Messages in this thread | | | From | Nick Terrell <> | Subject | Re: [PATCH][next] lib: zstd: Fix Wstringop-overflow warning | Date | Thu, 31 Mar 2022 19:55:04 +0000 |
| |
> On Mar 30, 2022, at 2:43 PM, Kees Cook <keescook@chromium.org> wrote: > > On Wed, Mar 30, 2022 at 02:33:52PM -0500, Gustavo A. R. Silva wrote: >> 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, >> | ^~~~~~~~~~~~~~~~ > > Reviewing this changes would be easier if the reason for the warning > could be explained. i.e. why has GCC decided that the region is 52 > bytes, and how did it calculate the 624 bytes? > > rankVal_t is HUF_TABLELOG_MAX-many rankValCol_t, which itself is > HUF_TABLELOG_MAX + 1 many U32s. So, basically: > > U32 array[HUF_TABLELOG_MAX + 1][HUF_TABLELOG_MAX] > > sizeof(rankValCol_t) == 52 > sizeof(rankVal_t) == 624
Yeah, I'm not quite sure what this warning is saying. Clarification would be useful. It seems like a false positive to me, but I could be mistaken.
>> >> by using pointer notation instead of array notation. >> >> This helps with the ongoing efforts to globally enable >> -Wstringop-overflow. >> >> Link: https://github.com/KSPP/linux/issues/181 >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> --- >> lib/zstd/decompress/huf_decompress.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/lib/zstd/decompress/huf_decompress.c b/lib/zstd/decompress/huf_decompress.c >> index 5105e59ac04a..0ea34621253a 100644 >> --- a/lib/zstd/decompress/huf_decompress.c >> +++ b/lib/zstd/decompress/huf_decompress.c >> @@ -570,7 +570,7 @@ static void HUF_fillDTableX2Level2(HUF_DEltX2* DTable, U32 sizeLog, const U32 co >> >> static void HUF_fillDTableX2(HUF_DEltX2* DTable, const U32 targetLog, >> const sortedSymbol_t* sortedList, const U32 sortedListSize, >> - const U32* rankStart, rankVal_t rankValOrigin, const U32 maxWeight, >> + const U32* rankStart, const U32* rankValOrigin, const U32 maxWeight, >> const U32 nbBitsBaseline, U32* wksp, size_t wkspSize) > > This really feels like we're papering over the warning. This removes the > type information and makes it a U32 * instead, and then later makes a > cast? > > Can this be fixed in a way that retains the type information? > > On the other hand, all the arguments are also U32 *. > > I see stuff like: > > ZSTD_memcpy(rankVal, rankValOrigin, sizeof(U32) * (HUF_TABLELOG_MAX + 1)); > > That looks like it's ignoring type information as well. i.e. why isn't > this sizeof(rankValOrigin)? (The length above is 52 bytes.) > > I'm especially curious here since rankValOrigin is rankVal_t, which is > 624 bytes, not 52 bytes (i.e. the above is copying a single rankValCol_t > from rankValOrigin. I'd expect this to be: > > ZSTD_memcpy(rankVal, &rankValOrigin[0], sizeof(rankValOrigin[0]));
Yes, this is definitely a better way to write it. But, I think the `&` is unnecessary. Because of the way C arrays work, rankValOrigin == rankValOrigin[0].
Upstream, this memcpy is gone, and this code has been optimized & refactored [1]. And that will get pulled in in the next update. But, in the meantime, I'd be happy to accept a refactoring diff.
[0] https://gcc.godbolt.org/z/orodoK818 [1] https://github.com/facebook/zstd/blob/3e6bbdd8473a753d2047969ac0053fb2cb4dda23/lib/decompress/huf_decompress.c#L992-L1036
>> { >> U32* rankVal = wksp; >> @@ -598,7 +598,7 @@ static void HUF_fillDTableX2(HUF_DEltX2* DTable, const U32 targetLog, >> if (minWeight < 1) minWeight = 1; >> sortedRank = rankStart[minWeight]; >> HUF_fillDTableX2Level2(DTable+start, targetLog-nbBits, nbBits, >> - rankValOrigin[nbBits], minWeight, >> + rankValOrigin + nbBits, minWeight, > > And here I'd expect to pass &rankValOrigin[nbBits] > since HUF_fillDTableX2Level2 is doing another rankValCol_t-sized copy: > > ZSTD_memcpy(rankVal, rankValOrigin, sizeof(U32) * (HUF_TABLELOG_MAX + 1));
In HUF_fillDTableX2Level2, rankValOrigin is a rankValCol_t. It is confusing, and this is improved upstream, though rankVal is passed as a U32 const*, where it should really Be a rankValCol_t. So I'll put up a small PR to improve that upstream.
>> sortedList+sortedRank, sortedListSize-sortedRank, >> nbBitsBaseline, symbol, wksp, wkspSize); >> } else { >> @@ -699,7 +699,7 @@ size_t HUF_readDTableX2_wksp(HUF_DTable* DTable, >> >> HUF_fillDTableX2(dt, maxTableLog, >> wksp->sortedSymbol, sizeOfSort, >> - wksp->rankStart0, wksp->rankVal, maxW, >> + wksp->rankStart0, (U32 *)wksp->rankVal, maxW, > > It's possible the problem is with this structure: > > typedef struct { > rankValCol_t rankVal[HUF_TABLELOG_MAX]; > U32 rankStats[HUF_TABLELOG_MAX + 1]; > U32 rankStart0[HUF_TABLELOG_MAX + 2]; > sortedSymbol_t sortedSymbol[HUF_SYMBOLVALUE_MAX + 1]; > BYTE weightList[HUF_SYMBOLVALUE_MAX + 1]; > U32 calleeWksp[HUF_READ_STATS_WORKSPACE_SIZE_U32]; > } HUF_ReadDTableX2_Workspace; > > it's not using the rankVal_t type for rankVal for some reason? > > i.e. what's passed to HUF_fillDTableX2 is a rankValCol_t (52 bytes), but then it > gets passed against later as rankVal_t (624 bytes). > > Does changing the type definition above solve this more cleanly?
It would make sense to refactor it as a `rankVal_t` to me, since that is consistent with the function signature. However, after removing typedefs, `wksp->rankVal` should be exactly the same type as `rankVal_t`. So the warning seems too noisy here.
Not saying that the code can't be improved, it certainly can be, and I will put up a PR upstream by tomorrow, and link it here.
Best, Nick Terrell
> -Kees > >> tableLog+1, >> wksp->calleeWksp, sizeof(wksp->calleeWksp) / sizeof(U32)); >> >> -- >> 2.27.0 >> > > -- > Kees Cook
| |