| Message ID | 20230104212048.gonna.331-kees@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 780f6a9afe8b0e303406a39f6968cf1daa6c3d51 |
| Headers | show |
| Series | [v2] lib: zstd: Fix -Wstringop-overflow warning | expand |
> 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? Best, Nick Terrell > Co-developed-by: Gustavo A. R. Silva <gustavoars@kernel.org> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > Cc: Nick Terrell <terrelln@fb.com> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > v2: use "rankValCol_t *" instead of U32 > v1: https://lore.kernel.org/lkml/20220330193352.GA119296@embeddedor/ > --- > lib/zstd/decompress/huf_decompress.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/zstd/decompress/huf_decompress.c b/lib/zstd/decompress/huf_decompress.c > index 89b269a641c7..60958afebc41 100644 > --- a/lib/zstd/decompress/huf_decompress.c > +++ b/lib/zstd/decompress/huf_decompress.c > @@ -985,7 +985,7 @@ static void HUF_fillDTableX2Level2(HUF_DEltX2* DTable, U32 targetLog, const U32 > > static void HUF_fillDTableX2(HUF_DEltX2* DTable, const U32 targetLog, > const sortedSymbol_t* sortedList, > - const U32* rankStart, rankVal_t rankValOrigin, const U32 maxWeight, > + const U32* rankStart, rankValCol_t *rankValOrigin, const U32 maxWeight, > const U32 nbBitsBaseline) > { > U32* const rankVal = rankValOrigin[0]; > -- > 2.34.1 >
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
> 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
On Sat, Jan 14, 2023 at 01:06:07AM +0000, Nick Terrell wrote: > > > > 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. Thanks! > If you would like to submit the same patch upstream yourself, I will accept the PR, otherwise I can submit an upstream PR. I don't know the process there, so if you could do it, I'd appreciate it. > Just to be certain, this patch is to work around a shortcoming in > -Wstringop-overflow, but the code was otherwise correct? It's the same result for the binary output. The types indicated in the function prototype meant there was cross-struct-member overflow (i.e. writing to the second array dimension when only 1 was specified), but the way to clear up intention isn't as clean here, so I'd kind of say half a code correctness issue, and half a work-around. *makes wavey hands gesture* -Kees
diff --git a/lib/zstd/decompress/huf_decompress.c b/lib/zstd/decompress/huf_decompress.c index 89b269a641c7..60958afebc41 100644 --- a/lib/zstd/decompress/huf_decompress.c +++ b/lib/zstd/decompress/huf_decompress.c @@ -985,7 +985,7 @@ static void HUF_fillDTableX2Level2(HUF_DEltX2* DTable, U32 targetLog, const U32 static void HUF_fillDTableX2(HUF_DEltX2* DTable, const U32 targetLog, const sortedSymbol_t* sortedList, - const U32* rankStart, rankVal_t rankValOrigin, const U32 maxWeight, + const U32* rankStart, rankValCol_t *rankValOrigin, const U32 maxWeight, const U32 nbBitsBaseline) { U32* const rankVal = rankValOrigin[0];
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. Co-developed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> Cc: Nick Terrell <terrelln@fb.com> Signed-off-by: Kees Cook <keescook@chromium.org> --- v2: use "rankValCol_t *" instead of U32 v1: https://lore.kernel.org/lkml/20220330193352.GA119296@embeddedor/ --- lib/zstd/decompress/huf_decompress.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)