Fix usan runtime errors #983
Conversation
Majority of usan runtime errors are two types.
(1) Applying pointer arithmetic to NULL pointer. (undefined behaviour)
```
- const BYTE* p = x + 1; // usan runtime error when `x` is NULL.
+ const BYTE* p = (const BYTE*) ((uptrval) x + 1);
```
(2) Result of pointer arithmetic is overflowed. (undefined behaviour)
```
- const BYTE* p = x - 0x4000; // usan runtime error when `x` is less than 0x4000.
+ const BYTE* p = (const BYTE*) ((uptrval) x - 0x4000);
```
Patch for `LZ4_resetStreamHC_fast()` is just a bit cumbersome. For this line
```
LZ4_streamHCPtr->internal_donotuse.end -= (uptrval) LZ4_streamHCPtr->internal_donotuse.base;
```
`usan` claims
> applying non-zero offset to non-null pointer 0xXXXXXXXXXXXXXXXX produced null pointer
So, we need to cast both of pointers to `uptrval` and cast to `const LZ4_byte*`.
```
LZ4_streamHCPtr->internal_donotuse.end =
(const LZ4_byte*) ( (uptrval) LZ4_streamHCPtr->internal_donotuse.end
- (uptrval) LZ4_streamHCPtr->internal_donotuse.base );
```
|
FYI, here's an actual |
|
I'm a bit embarrassed by the proposed fix I'm fine with the end objective to try to stay clear from UB. Rather, let's try to improve both UB and readability, whenever it makes sense. |
| @@ -134,12 +134,14 @@ static int g_debuglog_enable = 1; | |||
| typedef uint32_t U32; | |||
| typedef int32_t S32; | |||
| typedef uint64_t U64; | |||
| typedef uintptr_t uptrval; | |||
There was a problem hiding this comment.
Unfortunately uintptr_t is not guaranteed to be present in <stdint.h>.
This could lead to portability issues for platforms that don't support it.
There was a problem hiding this comment.
You're right. In the standard, it's optional type. But I've copied this definition from lz4.c for consistency. And uptrval has been used in lz4.c for long time.
Therefore, if <stdint.h> doesn't have uintptr_t, they can't compile entire lz4 code at all. Also if I change this line, we must change counterpart in lz4.c.
But I don't say this is the best definition. So can we leave this in this PR, and can we create other PR to address uptrval issue?
Although possible solution may be
/* From n1256 (C99), "7.18.2 Limits of specified-width integer types"
> 1. The following object-like macros specify the minimum and
> maximum limits of the types declared in <stdint.h>.
> ...
> - maximum value of pointer-holding unsigned integer type
> UINTPTR_MAX
They say "the types declared in <stdint.h>". So if uintptr_t is
not declared in <stdint.h>, they must not have a UINTPTR_MAX. */
#if defined(UINTPTR_MAX) /* UINTPTR_MAX is optional macro constant */
typedef uintptr_t uptrval;
#else
typedef uintmax_t uptrval; /* choose the available "best" type */
#endif
void in_some_function() {
LZ4_STATIC_ASSERT(sizeof(uptrval) >= sizeof(void*));
}There was a problem hiding this comment.
arf, I thought that I had removed that definition a long time ago already.
I might have confused with zstd.
Anyway, that's still a goal : we should get rid of uintptr_t.
There was a problem hiding this comment.
Right. I'm also realized that lz4frame.c is the best candidate to start "get rid of uintptr_t" movement since it didn't have uptrval 😉
| @@ -886,8 +886,8 @@ LZ4_FORCE_INLINE int LZ4_compress_generic_validated( | |||
| /* the dictCtx currentOffset is indexed on the start of the dictionary, | |||
| * while a dictionary in the current context precedes the currentOffset */ | |||
| const BYTE* dictBase = !dictionary ? NULL : (dictDirective == usingDictCtx) ? | |||
| dictionary + dictSize - dictCtx->currentOffset : | |||
| dictionary + dictSize - startIndex; | |||
| (const BYTE*) ((uptrval) dictionary + dictSize - dictCtx->currentOffset) : | |||
There was a problem hiding this comment.
This one is strange.
We just checked !dictionary just before,
so by that point, we are certain than dictionary != NULL,
hence it shouldn't be necessary to fudge the calculation through uptrval.
There was a problem hiding this comment.
For this line, usan doesn't report about pointer arithmetic for NULL.
It indicates a pointer index expression overflow.
../lib/lz4.c:890:51: runtime error: pointer index expression with base 0x000000ed9910 overflowed to 0xfffffffffffe5156
../lib/lz4.c:890:51: runtime error: pointer index expression with base 0x000001d08480 overflowed to 0xffffffffffff8480
../lib/lz4.c:890:51: runtime error: pointer index expression with base 0x0000022ca440 overflowed to 0xfffffffffffca440
For instance, usan reports this error with the following variables:
dictionary=0x1d0e480, dictSize=65536, startIndex=30539776
dictionary + dictSize - startIndex
= 0x1d0e480 + 65536 - 30539776
= -7040
edit : put all errors for investigation
| @@ -865,7 +865,7 @@ LZ4_FORCE_INLINE int LZ4_compress_generic_validated( | |||
| const BYTE* ip = (const BYTE*) source; | |||
|
|
|||
| U32 const startIndex = cctx->currentOffset; | |||
| const BYTE* base = (const BYTE*) source - startIndex; | |||
| const BYTE* base = (const BYTE*) ((uptrval) source - startIndex); | |||
There was a problem hiding this comment.
Documentation states that, by this point, we should have source != NULL.
There was a problem hiding this comment.
usan reports an pointer arithmetic overflow.
../lib/lz4.c:868:45: runtime error: pointer index expression with base 0x000000001000 overflowed to 0xffffffffffffac2f
../lib/lz4.c:868:45: runtime error: pointer index expression with base 0x000000ed9910 overflowed to 0xfffffffffffe5156
../lib/lz4.c:868:45: runtime error: pointer index expression with base 0x000001cd3400 overflowed to 0xffffffffffff3400
edit : put all errors for future investigation.
| @@ -978,17 +978,17 @@ LZ4_FORCE_INLINE int LZ4_compress_generic_validated( | |||
| matchIndex += dictDelta; /* make dictCtx index comparable with current context */ | |||
| lowLimit = dictionary; | |||
| } else { | |||
| match = base + matchIndex; | |||
| match = (const BYTE*) ((uptrval) base + matchIndex); | |||
There was a problem hiding this comment.
we should rather ensure that, whenever this code is triggered, we have a guarantee that base != NULL.
asserting this condition would be fine.
|
Some of these runtime UB warnings seem to uncover root causes that should not happen. |
|
I'm really sorry. But these errors which you've commented are not caused by edit: I'll write instance of all |
|
Indeed, pointer overflow is a different type of UB. I would recommend to ignore it for the time being. |
| @@ -1554,7 +1556,7 @@ size_t LZ4F_decompress(LZ4F_dctx* dctx, | |||
| } } | |||
|
|
|||
| srcPtr += sizeToCopy; | |||
| dstPtr += sizeToCopy; | |||
| dstPtr = (BYTE*) ((uptrval) dstPtr + sizeToCopy); | |||
There was a problem hiding this comment.
usan-log:../lib/lz4frame.c:1557:24: runtime error: applying zero offset to null pointer
Since we have if (dstPtr == NULL) in the previous block, this should be fixed as the following
if (dstPtr == NULL) {
sizeToCopy = 0;
} else {
...
if (dctx->frameInfo.blockMode == LZ4F_blockLinked) {
...
- } }
+ }
+ dstPtr += sizeToCopy;
+ }
srcPtr += sizeToCopy;
- dstPtr += sizeToCopy; /* [usan] ../lib/lz4frame.c:1557:24: runtime error: applying zero offset to null pointer */
| lowLimit = (const BYTE*)source; | ||
| } | ||
| } else if (dictDirective==usingExtDict) { | ||
| if (matchIndex < startIndex) { | ||
| DEBUGLOG(7, "extDict candidate: matchIndex=%5u < startIndex=%5u", matchIndex, startIndex); | ||
| assert(startIndex - matchIndex >= MINMATCH); | ||
| match = dictBase + matchIndex; | ||
| match = (const BYTE*) ((uptrval)dictBase + matchIndex); |
There was a problem hiding this comment.
../lib/lz4.c:988:42: runtime error: applying non-zero offset 2096600 to null pointer
../lib/lz4.c:988:42: runtime error: pointer index expression with base 0xfffffffffffca440 overflowed to 0x0000022c9b93
../lib/lz4.c:988:42: runtime error: pointer index expression with base 0xffffffffffff8480 overflowed to 0x000001d05682
| lowLimit = dictionary; | ||
| } else { | ||
| match = base + matchIndex; | ||
| match = (const BYTE*) ((uptrval) base + matchIndex); |
There was a problem hiding this comment.
../lib/lz4.c:991:38: runtime error: pointer index expression with base 0xffffffffffff3400 overflowed to 0x000001cd3490
../lib/lz4.c:991:38: runtime error: pointer index expression with base 0xffffffffffffac2f overflowed to 0x000000001092
|
Since my inline comments for PR looks like spam, I'd like to just put a entire `make usan` (lib/lz4.c)`make usan` lib/lz4hc.c |
|
Thanks @t-mat , |
|
It's not an error of When I run the following test I've seen the following assertion. Here's a full test log. I also confirmed same assertion with all recent versions of clang from clang-3.5 to clang-12. Please let me know if this is not a valid test. I'm working to include this test scenario into GH-Actions CI. |
|
Here's a full log. |
I presume it's related to #991 ? |
|
closing, as I believe that using |
I also think so. But it seems they're (edit: GH-Actions team is) pulling out GCC11 from the repository. They will remove it from I apologize again for making many confusions by random comments and hasty introduction of |
|
No pb @t-mat , you are trying to make a great CI system, and finding many little details in the process. |
This is a follow up PR of #982.
This PR fixes
make usanruntime errors.Majority of usan runtime errors are two types.
(1) Applying pointer arithmetic to NULL pointer. (undefined behaviour)
(2) Result of pointer arithmetic is overflowed. (undefined behaviour)
Patch for
LZ4_resetStreamHC_fast()is just a bit cumbersome. For this lineusanclaimsSo, we need to cast both of pointers to
uptrvaland cast toconst LZ4_byte*.