Messages in this thread Patch in this message | | | Date | Thu, 28 Mar 2024 23:54:37 +0000 | From | Justin Stitt <> | Subject | Re: [PATCH 05/11] test_hexdump: avoid string truncation warning |
| |
Hi,
On Thu, Mar 28, 2024 at 03:04:49PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > gcc can warn when a string is too long to fit into the strncpy() > destination buffer, as it is here depending on the function > arguments: > > inlined from 'test_hexdump_prepare_test.constprop' at /home/arnd/arm-soc/lib/test_hexdump.c:116:3: > include/linux/fortify-string.h:108:33: error: '__builtin_strncpy' output truncated copying between 0 and 32 bytes from a string of length 32 [-Werror=stringop-truncation] > 108 | #define __underlying_strncpy __builtin_strncpy > | ^ > include/linux/fortify-string.h:187:16: note: in expansion of macro '__underlying_strncpy' > 187 | return __underlying_strncpy(p, q, size); > | ^~~~~~~~~~~~~~~~~~~~ > > As far as I can tell, this is harmless here because the truncation is > intentional, but using strscpy_pad() will avoid the warning since gcc > does not (yet) know about it. >
We need to be careful. strscpy() or strscpy_pad() are not drop-in replacements for strncpy().
if @l is less than the length of @data_a we might have a problem because strscpy_pad() will eagerly assign a NUL-byte to dest[l-1].
It looks like @l could be less than the length of @data_a judging from: size_t len = get_random_u32_inclusive(1, d);
Let me model the potential behavior before and after, understanding that data_a is defined as:
static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C...";
Before (using strncpy): p = ['j', 'u', 'n', 'k'] // example destination buffer assume @l = 3 then we are trying to copy ".2." from @data_a, resulting in p = ['.', '2', '.', 'k']
After (using strscpy_pad()): p = ['j', 'u', 'n', 'k'] // example destination buffer assume @l = 3 then we are trying to copy ".2." from @data_a, resulting in p = ['.', '2', '\0', 'k']
because strscpy got to the end of its allowed size and didn't find a NUL-term from its source string, so it eagerly assigns a NUL-byte to the end, essentially truncating our string.
Here's the responsible code from strscpy's implementation: if (res) dest[res-1] = '\0';
https://elixir.bootlin.com/linux/latest/source/lib/string.c#L107
It is possible I haven't fully considered the context of this change but I think using strscpy_pad() will cause these tests to fail, if they aren't failing I think we're getting lucky.
Also, maybe this godbolt example can help demonstrate: https://godbolt.org/z/nWGKraTvT
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > lib/test_hexdump.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c > index b916801f23a8..c9820122af56 100644 > --- a/lib/test_hexdump.c > +++ b/lib/test_hexdump.c > @@ -113,7 +113,7 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize, > *p++ = ' '; > } while (p < test + rs * 2 + rs / gs + 1); > > - strncpy(p, data_a, l); > + strscpy_pad(p, data_a, l); > p += l; > } > > -- > 2.39.2 >
Thanks Justin
| |