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 <[email protected]>
Signed-off-by: Gustavo A. R. Silva <[email protected]>
Cc: Nick Terrell <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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 Jan 4, 2023, at 1:20 PM, Kees Cook <[email protected]> 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 <[email protected]>
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> Cc: Nick Terrell <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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 <[email protected]> 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
--
Kees Cook
> On Jan 13, 2023, at 4:48 PM, Kees Cook <[email protected]> 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 <[email protected]> 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 <[email protected] <mailto:[email protected]>>
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 <[email protected]> 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 <[email protected]> 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
--
Kees Cook