linux-kernel - Re: [PATCH] xfs: Replace strncpy with strscpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <w5zac6jzwncscbhnihlfonbp7ac4jsf7d4ge7uo4fpsqcloeil@ylxill2zypfn>
Date: Sun, 20 Jul 2025 15:35:50 +0200
From: Carlos Maiolino <cem@...nel.org>
To: Marcelo Moreira <marcelomoreira1905@...il.com>
Cc: Dave Chinner <david@...morbit.com>, linux-xfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, 
	linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH] xfs: Replace strncpy with strscpy

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