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.

Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
---
 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

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@gmail.com>
> ---
>  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
> 
>

Em qui., 17 de jul. de 2025 às 13:39, Darrick J. Wong
<djwong@kernel.org> escreveu:
>
> 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.

This was s/strncpy/strscpy/ , no real bug.

-- 
Cheers,
Marcelo Moreira

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.

Yes, this prevents source buffer overrun in the event the corrupted
symlink data buffer is not correctly nul terminated or there is a
length mismatch between the symlink data and the inode metadata.

This patch removes the explicit source buffer overrun protection the
code currently provides.

> 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.

No, that can't happen, because sc->buf is initialised to contain
NULs and is large enough to hold a max length symlink plus the
trailing NUL termination.  Hence it will always be NUL-terminated,
even if the symlink length (nr) is capped at XFS_SYMLINK_MAXLEN.

> 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.

Please read the code more carefully. This is -explicitly- called out
in a comment in xrep_symlink_salvage() before it starts to process
the symlink data buffer that we just used strncpy() to place the
data in:

		/*
                 * NULL-terminate the buffer because the ondisk target does not
                 * do that for us.  If salvage didn't find the exact amount of
                 * data that we expected to find, don't salvage anything.
                 */
                target_buf[buflen] = 0;
                if (strlen(target_buf) != sc->ip->i_disk_size)
                        buflen = 0;

Also, have a look at the remote symlink data copy above the inline
salvage code you are changing (xrep_symlink_salvage_remote()).

It uses memcpy() to reconstruct the symlink data from multiple
source buffers. It does *not* explicitly NUL-terminate sc->buf after
using memcpy() to copy from the on disk structures to sc->buf. The
on-disk symlink data is *not* NUL-terminated, either.

IOWs, the salvage code that reconstructs the symlink data does not
guarantee NUL termination, so we do it explicitly before the data in
the returned buffer is used.

> Furthermore, `sc->buf` is allocated with
> `kvzalloc(XFS_SYMLINK_MAXLEN + 1, ...)`, explicitly reserving space for
> the NUL terminator.

.... and initialising the entire buffer to contain NULs.  IOWs, we
have multiple layers of defence against data extraction not doing
NUL-termination of the data it extracts.

> This change improves code safety and clarity by using a safer function for
> string copying.

I disagree. It is a bad change because it uses strscpy() incorrectly
for the context. i.e. it removes explicit source buffer overrun
protection whilst making the incorrect assumption that the callers
need to be protected from unterminated strings in the destination
buffer.

This code is dealing with *corrupt structures*, so it -must not-
make any assumptions about the validity of incoming data structures,
nor the validity of recovered data.  It has to treat them as is they
are always invalid, and explicitly protect against all types of
buffer overruns.

IOW, if you must replace strncpy() in xrep_symlink_salvage_inline(),
then the correct replacement memcpy().  Using some other strcpy()
variant is just as easy to get wrong as strncpy() if you don't
understand why strncpy() is safe to use in the first place.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Em qua., 16 de jul. de 2025 às 20:52, Dave Chinner
<david@fromorbit.com> escreveu:
>
> 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.
>
> Yes, this prevents source buffer overrun in the event the corrupted
> symlink data buffer is not correctly nul terminated or there is a
> length mismatch between the symlink data and the inode metadata.
>
> This patch removes the explicit source buffer overrun protection the
> code currently provides.
>
> > 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.
>
> No, that can't happen, because sc->buf is initialised to contain
> NULs and is large enough to hold a max length symlink plus the
> trailing NUL termination.  Hence it will always be NUL-terminated,
> even if the symlink length (nr) is capped at XFS_SYMLINK_MAXLEN.
>
> > 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.
>
> Please read the code more carefully. This is -explicitly- called out
> in a comment in xrep_symlink_salvage() before it starts to process
> the symlink data buffer that we just used strncpy() to place the
> data in:
>
>                 /*
>                  * NULL-terminate the buffer because the ondisk target does not
>                  * do that for us.  If salvage didn't find the exact amount of
>                  * data that we expected to find, don't salvage anything.
>                  */
>                 target_buf[buflen] = 0;
>                 if (strlen(target_buf) != sc->ip->i_disk_size)
>                         buflen = 0;
>
> Also, have a look at the remote symlink data copy above the inline
> salvage code you are changing (xrep_symlink_salvage_remote()).
>
> It uses memcpy() to reconstruct the symlink data from multiple
> source buffers. It does *not* explicitly NUL-terminate sc->buf after
> using memcpy() to copy from the on disk structures to sc->buf. The
> on-disk symlink data is *not* NUL-terminated, either.
>
> IOWs, the salvage code that reconstructs the symlink data does not
> guarantee NUL termination, so we do it explicitly before the data in
> the returned buffer is used.
>
> > Furthermore, `sc->buf` is allocated with
> > `kvzalloc(XFS_SYMLINK_MAXLEN + 1, ...)`, explicitly reserving space for
> > the NUL terminator.
>
> .... and initialising the entire buffer to contain NULs.  IOWs, we
> have multiple layers of defence against data extraction not doing
> NUL-termination of the data it extracts.
>
> > This change improves code safety and clarity by using a safer function for
> > string copying.
>
> I disagree. It is a bad change because it uses strscpy() incorrectly
> for the context. i.e. it removes explicit source buffer overrun
> protection whilst making the incorrect assumption that the callers
> need to be protected from unterminated strings in the destination
> buffer.
>
> This code is dealing with *corrupt structures*, so it -must not-
> make any assumptions about the validity of incoming data structures,
> nor the validity of recovered data.  It has to treat them as is they
> are always invalid, and explicitly protect against all types of
> buffer overruns.
>
> IOW, if you must replace strncpy() in xrep_symlink_salvage_inline(),
> then the correct replacement memcpy().  Using some other strcpy()
> variant is just as easy to get wrong as strncpy() if you don't
> understand why strncpy() is safe to use in the first place.
>
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com

got it, `kvzalloc` ensures that `sc->buf` is indeed NUL-terminated
from the start, and the explicit NUL termination (I hadn't seen that)
in `xrep_symlink_salvage()` (target_buf[buflen] = 0;) further
clarifies that the data on disk is treated as raw, non-NUL-terminated
bytes.

Thank you very much Dave for your detailed review and for taking the
time to explain the nuances of this code. I clearly misunderstood
several critical aspects of how `strncpy()` was being used here and
the protective mechanisms already in place.

My apologies for the incorrect assumptions in my commit message.

Given that the original `strncpy()` is safe and correctly implemented
for this context, and understanding that `memcpy()` would be the
correct replacement if a change were deemed necessary for
non-NUL-terminated raw data, I have a question:

Do you still think a replacement is necessary here, or would you
prefer to keep the existing `strncpy()` given its correct and safe
usage in this context? If a replacement is preferred, I will resubmit
a V2 using `memcpy()` instead.

Thank you again for teaching me these important details. I'm learning
a lot from your feedback :D

-- 
Cheers,
Marcelo Moreira

On Thu, Jul 17, 2025 at 02:34:25PM -0300, Marcelo Moreira wrote:
> Given that the original `strncpy()` is safe and correctly implemented
> for this context, and understanding that `memcpy()` would be the
> correct replacement if a change were deemed necessary for
> non-NUL-terminated raw data, I have a question:
> 
> Do you still think a replacement is necessary here, or would you
> prefer to keep the existing `strncpy()` given its correct and safe
> usage in this context? If a replacement is preferred, I will resubmit
> a V2 using `memcpy()` instead.

IMO: if it isn't broken, don't try to fix it. Hence I -personally-
wouldn't change it.

However, modernisation and cleaning up of the code base to be
consistent is a useful endeavour. So from this perspective replacing
strncpy with memcpy() would be something I'd consider an acceptible
change.

Largely it is up to the maintainer to decide.....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Em sex., 18 de jul. de 2025 às 08:16, Dave Chinner
<david@fromorbit.com> escreveu:
>
> On Thu, Jul 17, 2025 at 02:34:25PM -0300, Marcelo Moreira wrote:
> > Given that the original `strncpy()` is safe and correctly implemented
> > for this context, and understanding that `memcpy()` would be the
> > correct replacement if a change were deemed necessary for
> > non-NUL-terminated raw data, I have a question:
> >
> > Do you still think a replacement is necessary here, or would you
> > prefer to keep the existing `strncpy()` given its correct and safe
> > usage in this context? If a replacement is preferred, I will resubmit
> > a V2 using `memcpy()` instead.
>
> IMO: if it isn't broken, don't try to fix it. Hence I -personally-
> wouldn't change it.
>
> However, modernisation and cleaning up of the code base to be
> consistent is a useful endeavour. So from this perspective replacing
> strncpy with memcpy() would be something I'd consider an acceptible
> change.
>
> Largely it is up to the maintainer to decide.....

Hmm ok, thanks for the teaching :)

So, I'll prepare v2 replacing `strncpy` with `memcpy` aiming for that
modernization and cleanup you mentioned, but it's clear that the
intention is to focus on changes that cause real bugs.
Thanks!

-- 
Cheers,
Marcelo Moreira

On Fri, Jul 18, 2025 at 04:10:39PM -0300, Marcelo Moreira wrote:
> Em sex., 18 de jul. de 2025 às 08:16, Dave Chinner
> <david@fromorbit.com> escreveu:
> >
> > On Thu, Jul 17, 2025 at 02:34:25PM -0300, Marcelo Moreira wrote:
> > > Given that the original `strncpy()` is safe and correctly implemented
> > > for this context, and understanding that `memcpy()` would be the
> > > correct replacement if a change were deemed necessary for
> > > non-NUL-terminated raw data, I have a question:
> > >
> > > Do you still think a replacement is necessary here, or would you
> > > prefer to keep the existing `strncpy()` given its correct and safe
> > > usage in this context? If a replacement is preferred, I will resubmit
> > > a V2 using `memcpy()` instead.
> >
> > IMO: if it isn't broken, don't try to fix it. Hence I -personally-
> > wouldn't change it.
> >
> > However, modernisation and cleaning up of the code base to be
> > consistent is a useful endeavour. So from this perspective replacing
> > strncpy with memcpy() would be something I'd consider an acceptible
> > change.
> >
> > Largely it is up to the maintainer to decide.....
> 
> Hmm ok, thanks for the teaching :)
> 
> So, I'll prepare v2 replacing `strncpy` with `memcpy` aiming for that
> modernization and cleanup you mentioned, but it's clear that the
> intention is to focus on changes that cause real bugs.
> Thanks!
> 

I'm ok with cleanups, code refactoring, etc, when they are
aiming to improve something.

I'm not against the strncpy -> memcpy change itself, but my biggest
concern is that I'm almost sure you are not testing it. I really do
wish to be wrong here though, so...
Did you test this patch before sending?

I'm not talking about build and boot. Even for a 'small' change like
this, you should at least be running xfstests and ensure no tests are
failing because of this change, otherwise you are essentially sending
to the list an untested patch.

Even experienced developers falls into the "I'm sure this patch is
correct", just to get caught by some testing system - just to emphasize
how important it's to test the patches you craft.

Carlos.


> --
> Cheers,
> Marcelo Moreira
> 

Em dom., 20 de jul. de 2025 às 10:35, Carlos Maiolino <cem@kernel.org> escreveu:
>
> On Fri, Jul 18, 2025 at 04:10:39PM -0300, Marcelo Moreira wrote:
> > Em sex., 18 de jul. de 2025 às 08:16, Dave Chinner
> > <david@fromorbit.com> escreveu:
> > >
> > > On Thu, Jul 17, 2025 at 02:34:25PM -0300, Marcelo Moreira wrote:
> > > > Given that the original `strncpy()` is safe and correctly implemented
> > > > for this context, and understanding that `memcpy()` would be the
> > > > correct replacement if a change were deemed necessary for
> > > > non-NUL-terminated raw data, I have a question:
> > > >
> > > > Do you still think a replacement is necessary here, or would you
> > > > prefer to keep the existing `strncpy()` given its correct and safe
> > > > usage in this context? If a replacement is preferred, I will resubmit
> > > > a V2 using `memcpy()` instead.
> > >
> > > IMO: if it isn't broken, don't try to fix it. Hence I -personally-
> > > wouldn't change it.
> > >
> > > However, modernisation and cleaning up of the code base to be
> > > consistent is a useful endeavour. So from this perspective replacing
> > > strncpy with memcpy() would be something I'd consider an acceptible
> > > change.
> > >
> > > Largely it is up to the maintainer to decide.....
> >
> > Hmm ok, thanks for the teaching :)
> >
> > So, I'll prepare v2 replacing `strncpy` with `memcpy` aiming for that
> > modernization and cleanup you mentioned, but it's clear that the
> > intention is to focus on changes that cause real bugs.
> > Thanks!
> >
>
> I'm ok with cleanups, code refactoring, etc, when they are
> aiming to improve something.
>
> I'm not against the strncpy -> memcpy change itself, but my biggest
> concern is that I'm almost sure you are not testing it. I really do
> wish to be wrong here though, so...
> Did you test this patch before sending?
>
> I'm not talking about build and boot. Even for a 'small' change like
> this, you should at least be running xfstests and ensure no tests are
> failing because of this change, otherwise you are essentially sending
> to the list an untested patch.
>
> Even experienced developers falls into the "I'm sure this patch is
> correct", just to get caught by some testing system - just to emphasize
> how important it's to test the patches you craft.


I actually only tested build and boot, sorry. I wasn't aware of
xfstests, thanks for letting me know. I'll research how to use
xfstests. Thanks, Carlos :)

-- 
Cheers,
Marcelo Moreira