Messages in this thread | | | From | Nick Terrell <> | Subject | Re: [PATCH v2] lib: zstd: Fix -Wstringop-overflow warning | Date | Sat, 14 Jan 2023 01:06:07 +0000 |
| |
> On Jan 13, 2023, at 4:48 PM, Kees Cook <keescook@chromium.org> wrote: > > !-------------------------------------------------------------------| > This Message Is From an External Sender > > |-------------------------------------------------------------------! > > On Tue, Jan 10, 2023 at 11:10:08PM +0000, Nick Terrell wrote: >> >> >>> On Jan 4, 2023, at 1:20 PM, Kees Cook <keescook@chromium.org> wrote: >>> >>> !-------------------------------------------------------------------| >>> This Message Is From an External Sender >>> >>> |-------------------------------------------------------------------! >>> >>> Fix the following -Wstringop-overflow warning when building with GCC 11+: >>> >>> lib/zstd/decompress/huf_decompress.c: In function ‘HUF_readDTableX2_wksp’: >>> lib/zstd/decompress/huf_decompress.c:700:5: warning: ‘HUF_fillDTableX2.constprop’ accessing 624 bytes in a region of size 52 [-Wstringop-overflow=] >>> 700 | HUF_fillDTableX2(dt, maxTableLog, >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> 701 | wksp->sortedSymbol, sizeOfSort, >>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> 702 | wksp->rankStart0, wksp->rankVal, maxW, >>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> 703 | tableLog+1, >>> | ~~~~~~~~~~~ >>> 704 | wksp->calleeWksp, sizeof(wksp->calleeWksp) / sizeof(U32)); >>> | >>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> lib/zstd/decompress/huf_decompress.c:700:5: note: referencing argument 6 of type ‘U32 (*)[13]’ {aka ‘unsigned int (*)[13]’} >>> lib/zstd/decompress/huf_decompress.c:571:13: note: in a call to function ‘HUF_fillDTableX2.constprop’ >>> 571 | static void HUF_fillDTableX2(HUF_DEltX2* DTable, const U32 targetLog, >>> | ^~~~~~~~~~~~~~~~ >>> >>> by using pointer notation instead of array notation. >>> >>> This is one of the last remaining warnings to be fixed before globally >>> enabling -Wstringop-overflow. >> >> The patch looks correct to me, thanks for reviving it. But, I was attempting to reproduce the issue, >> so I could better understand what's going on, and I wasn't able to reproduce it myself. >> >> To attempt to reproduce, I applied this patch >> >> --- >> diff --git a/lib/zstd/Makefile b/lib/zstd/Makefile >> index 20f08c644b71..190d3d5ab4be 100644 >> --- a/lib/zstd/Makefile >> +++ b/lib/zstd/Makefile >> @@ -12,6 +12,8 @@ obj-$(CONFIG_ZSTD_COMPRESS) += zstd_compress.o >> obj-$(CONFIG_ZSTD_DECOMPRESS) += zstd_decompress.o >> obj-$(CONFIG_ZSTD_COMMON) += zstd_common.o >> >> +ccflags-y := -Wstringop-overflow=4 -Werror >> + >> zstd_compress-y := \ >> zstd_compress_module.o \ >> compress/fse_compress.o \ >> --- >> >> Then compiled on x86-64 with gcc 12.2.0 on tag v6.2-rc3. I saw no errors. >> I also tried with just `-Wstringop-overflow`, and on upstream zstd. I tried to >> make a minimal reproducer on godbolt, so I could see if it was the gcc version, >> but wasn't able to make it fail with any of them https://gcc.godbolt.org/z/Exzq9arMr . >> >> Could you please tell me how to reproduce this warning? > > I saw it like so with next-20230113 on x86_64: > > $ gcc --version > gcc (Ubuntu 12.2.0-3ubuntu1) 12.2.0 > ... > $ make KCFLAGS=-Wstringop-overflow allmodconfig lib/zstd/decompress/huf_decompress.o
Thanks, I was able to repro it! I will merge this patch into my tree.
If you would like to submit the same patch upstream yourself, I will accept the PR, otherwise I can submit an upstream PR.
Just to be certain, this patch is to work around a shortcoming in -Wstringop-overflow, but the code was otherwise correct?
Reviewed-by: Nick Terrell <terrelln@fb.com <mailto:terrelln@fb.com>>
Best, Nick Terrell
> -- > Kees Cook
| |