On Wed, Jul 16, 2025 at 03:20:37PM -0300, Marcelo Moreira wrote: > The `strncpy` function is deprecated for NUL-terminated strings as > explained in the "strncpy() on NUL-terminated strings" section of > Documentation/process/deprecated.rst. > > In `xrep_symlink_salvage_inline()`, the `target_buf` (which is `sc->buf`) > is intended to hold a NUL-terminated symlink path. The original code > used `strncpy(target_buf, ifp->if_data, nr)`, where `nr` is the maximum > number of bytes to copy. This approach is problematic because `strncpy()` > does not guarantee NUL-termination if the source string is truncated > exactly at `nr` bytes, which can lead to out-of-bounds read issues > if the buffer is later treated as a NUL-terminated string. > Evidence from `fs/xfs/scrub/symlink.c` (e.g., `strnlen(sc->buf, > XFS_SYMLINK_MAXLEN)`) confirms that `sc->buf` is indeed expected to be > NUL-terminated. Furthermore, `sc->buf` is allocated with > `kvzalloc(XFS_SYMLINK_MAXLEN + 1, ...)`, explicitly reserving space for > the NUL terminator. > > `strscpy()` is the proper replacement because it guarantees NUL-termination > of the destination buffer, correctly handles the copy limit, and aligns > with current kernel string-copying best practices. > Other recommended functions like `strscpy_pad()`, `memcpy()`, or > `memcpy_and_pad()` were not used because: > - `strscpy_pad()` would unnecessarily zero-pad the entire buffer beyond the > NUL terminator, which is not required as the function returns `nr` bytes. > - `memcpy()` and `memcpy_and_pad()` do not guarantee NUL-termination, which > is critical given `target_buf` is used as a NUL-terminated string. > > This change improves code safety and clarity by using a safer function for > string copying. Did you find an actual bug in online fsck, or is this just s/strncpy/strscpy/ ? If the code already works correctly, please leave it alone. Unless you want to take on all the online fsck and fuzz testing to make sure the changes don't break anything. --D > Signed-off-by: Marcelo Moreira <marcelomoreira1905@xxxxxxxxx> > --- > fs/xfs/scrub/symlink_repair.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/scrub/symlink_repair.c b/fs/xfs/scrub/symlink_repair.c > index 953ce7be78dc..ce21c7f0ef54 100644 > --- a/fs/xfs/scrub/symlink_repair.c > +++ b/fs/xfs/scrub/symlink_repair.c > @@ -185,7 +185,7 @@ xrep_symlink_salvage_inline( > return 0; > > nr = min(XFS_SYMLINK_MAXLEN, xfs_inode_data_fork_size(ip)); > - strncpy(target_buf, ifp->if_data, nr); > + strscpy(target_buf, ifp->if_data, XFS_SYMLINK_MAXLEN + 1); > return nr; > } > > -- > 2.50.0 > >
