SpringMT / zstd-ruby

Ruby binding for zstd(Zstandard - Fast real-time compression algorithm)
https://github.com/facebook/zstd
BSD 3-Clause "New" or "Revised" License
69 stars 16 forks source link

Regression when decompressing an empty string in `zstd` 1.5.2.3 #51

Closed agalloch closed 8 months ago

agalloch commented 2 years ago

Hello, thank you for maintaining this awesome gem! 👏

When upgrading zstd-ruby from 1.5.2.2 to 1.5.2.3 I've encountered the following difference in decompress behaviour:

# Expected behaviour (as it was in versions < v1.5.2.3)

irb> Zstd.decompress ''
=> ""

vs.

# Actual behaviour

irb> Zstd.decompress ''
RuntimeError: not compressed by zstd: Unspecified error code

Since it's the same upstream version of zstd library (https://github.com/facebook/zstd/releases/tag/v1.5.2), this seems like a regression in the gem.

Am I missing something?

SpringMT commented 2 years ago

In v1.5.2.3, zstd decompression frame check is strictly. https://github.com/SpringMT/zstd-ruby/blob/main/ext/zstdruby/zstdruby.c#L121-L124

Previously using ZSTD_getDecompressedSize when check decompression string, but this method is obsolete. So, v1.5.2.3 zstd-ruby gem uses ZSTD_getFrameContentSize. https://raw.githack.com/facebook/zstd/release/doc/zstd_manual.html#Chapter3

In ZSTD_getFrameContentSize, an empty string with decompression is invalid, so in the spec, an empty string decompression should have been treated as an error. The example from the zstd sample. https://github.com/facebook/zstd/blob/dev/examples/simple_decompression.c#L27

It is not regression but breaking backward compatibility.

Is there any inconvenience?

agalloch commented 2 years ago

Hello @SpringMT, I'm not sure whether zstd-ruby gem uses semantic versioning but if it does, it's unusual to introduce backward incompatibility with a patch update such as 1.5.2.2 -> 1.5.2.3.

Do you recommend I use Zstd::StreamingDecompress when the source could be a blank string? 🙂

SpringMT commented 2 years ago

I'm considering again whether it is correct that Zstd::StreamingDecompress and Zstd.decompress is different behavior.

SpringMT commented 2 years ago

First of all, I added the breaking change about an empty string decompress to the release note. https://github.com/SpringMT/zstd-ruby/releases/tag/v1.5.2.3