Check that dest is valid for decompression

[daniellerozenblit]

This PR is a response to issues #3506 and #3507.

It adds some checks to ensure that the dest buffer passed to ZSTD_decompressBlock are valid. In particular, these check that if the dstCapacity == 0 then the number of sequences is 0 and that the pointer passed to dest is sufficiently small that any permitted match + literal length can be added to it without overflowing.

This PR additionally adds some fuzzing to target these issues. Specifically, we pass a random dest value when fuzzing decompression on a buffer of size 0.

[yoniko]

Was this file included on purpose? (same question for contrib/linux-kernel/test/test)

[daniellerozenblit]

oops, nope!

[terrelln]

Generally looks good, just want to see Yann's opinion on the uintptr_t.

[terrelln]

nit: We can combine this check with the line 2127. dst may only be NULL if dstCapacity == 0.

[terrelln]

@Cyan4973 what do you think of requiring uintptr_t here? We already do the same in LZ4.

[Cyan4973]

I'm generally not a fan of uintptr_t, because it's an optional type,
and we learned the hard way that this type breaks on some platforms.

What about the following alternative test ?

#define MAX_ADDRESS (const char*)(size_t)(-1)
#define SAFE_DISTANCE  (1 << 20)

if (MAX_ADDRESS - (const char*)dst < SAFE_DISTANCE) { ... } 

[Cyan4973]

I'll take it,

but note that this formulation essentially replaces uintptr_t by size_t,
which implies that sizeof(size_t) == sizeof(void*).
This is often true, for "common" platforms, but not always.
See this comment for an example.

In this specific example, where sizeof(void*)==16 while sizeof(size_t)==4,
the proposed test will trigger for any address close enough to a multiple of 4 GB.

To be fair, this is a hard issue to fix "properly" (i.e. without UB), so that's why I accept this fix proposal.
Evaluating the difference between 2 addresses which are not part of the same memory segment is UB to begin with (even if it works fine in a single-plane memory addressing architecture). That makes it hard to check the distance of an address from the end of its address space.

For the same reason, a dangling pointer that randomly points anywhere in memory is also UB, even if there is a second variable that states size == 0. So we are trying to fix at runtime a user-side UB. No wonder it's difficult to fix without triggering UB.

[daniellerozenblit]

Yeah I'm not sure of the fully correct way to solve this.

As you said, checking the distance between these two addresses is UB regardless.

I guess the major concern with my solution is that on some unusual platforms where sizeof(size_t) == 8 but sizeof(void*) != 8 we may produce a lot of false positives. Would it be better to not check at all?

[Cyan4973]

I guess the major concern with my solution is that on some unusual platforms where sizeof(size_t) == 8 but sizeof(void*) != 8 we may produce a lot of false positives. Would it be better to not check at all?

That's indeed the question.
It seems that your check dstCapacity==0 should catch the issues described in #3507, and possibly #3506.
In which case, maybe that's enough ?

[daniellerozenblit]

Correct, the first check catches all of the cases in #3507 and #3506.

[Cyan4973]

So maybe the first check is enough to fix the situation.

The second test is fine as long as sizeof(size_t) == sizeof(void*),
maybe that condition could be checked before triggering the second test.

[Cyan4973]

I'll merge it now, so that there is enough time testing before the release