[zstd] remove src minimal size limit on decompressSizeHint

[XiaochenCui]

We want to mandate the 'zstd' decompressed data to the buffer pre-allocated by us in some circumstances. (No matter whether its size is smaller than zstdFrameHeaderSizeMax.)

For example, in our OLAP system kernel, we have a unified buffer pool to manage all bytes allocated for queries. An accidentally created large array will have a fatal impact on the correctness and performance of the system. (In this case, any compressed data which have a size smaller than 18 would result in a newly constructed byte slice with a size of 1MB, which is intolerable.)

I had also considered other workaround:

• provide a global option to control the manner of binary allocation, the pros are it will make our condition branches simpler, and the cons are the difficulty of this library will get increased, and it will be tricky when different components of a system want different strategies.
• provide a new decompression API, which will break the consistence of our library.
• provide a new arg in the old API, same as point 2.

[XiaochenCui]

Closed it for not passed tests

[XiaochenCui]

Bug fixed

[XiaochenCui]

@Viq111
Request review or better solution on this issue sincerely.

[Viq111]

Hello @XiaochenCui , thanks for the PR.
Would you have a example payload that would be smaller than 18 bytes and valid zstd ?
Indeed the 18 bytes for the frame is the max, we could change it to the min with is 2 bytes

[XiaochenCui]

Hello @XiaochenCui , thanks for the PR. Would you have a example payload that would be smaller than 18 bytes and valid zstd ? Indeed the 18 bytes for the frame is the max, we could change it to the min with is 2 bytes

Yeah, we have a compressed column in our OLAP system with a length of 16. It's generated by a binary with 52 zeros.

Here is the code:

originalData = make([]byte, 52)
logging.Printf("len of original data: %d", len(originalData))

compressedData = Compress(CompressZSTD, originalData)
logging.Printf("len of compressed data: %d, content: %v", len(compressedData), compressedData)

The Compress function is just a wrapper for zstd2.CompressLevel(...)

And here is the output:

2022-05-29 16:56:50 | INFO  | compress_test.go:90 > len of original data: 52
2022-05-29 16:56:50 | INFO  | compress_test.go:93 > len of compress data: 16, content: [40 181 47 253 32 52 61 0 0 8 0 1 0 224 116 66]

[XiaochenCui]

In zstd1.50, the zstd2.CompressLevel will always use our buffer for decompressed data.

However, in zstd1.52, it will generate a huge binary with size=1 million when we have compressed data with a size < 18, which is a critical problem for both performance and consistency. We downgrade the library to 1.50 to erase this trouble. But I really want to solve it from the root. (I thought the root cause is decompressSizeHint, tell me if it isn't)

[XiaochenCui]

Here is the code that is more convenient for you:

        originalData := make([]byte, 52)
	logging.Printf("len of original data: %d", len(originalData))

	// compressedData = Compress(CompressZSTD, originalData)
	compressedData, err := zstd2.CompressLevel(make([]byte, 0, len(originalData)), originalData, zstd2.DefaultCompression)
	if err != nil {
		panic(err)
	}
	logging.Printf("len of compressed data: %d, content: %v", len(compressedData), compressedData)`

[XiaochenCui]

@Viq111
I have updated the minimal size of hint to zstdFrameHeaderSizeMin, which is 2.

Btw, why do we have to "prevent DOS from maliciously-created payloads"? As I'm not good at application cryptography.

[Viq111]

Hello!

Ah I've been working on the code in parallel yesterday, here is the PR with a test: #120 that should fix your issue (thanks for reporting!)

Btw, why do we have to "prevent DOS from maliciously-created payloads"? As I'm not good at application cryptography.
See the original PR that introduced the change: #115 (and related issue #60) for the explanation.

If that's ok, I will merge #120 instead of this one as it has the added test ?

[XiaochenCui]

Hello!

Ah I've been working on the code in parallel yesterday, here is the PR with a test: #120 that should fix your issue (thanks for reporting!)

Btw, why do we have to "prevent DOS from maliciously-created payloads"? As I'm not good at application cryptography.
See the original PR that introduced the change: #115 (and related issue #60) for the explanation.

If that's ok, I will merge #120 instead of this one as it has the added test ?

Sure, I've reviewed that pr and it's great! Thanks for tracking this issue.