6 ms·

ZSTD 1.5.5 is released with a corruption fix found at Google

170 points by danlark 3y ago

vo_9812 3y ago
Congrats to Yann and the team.
pella 3y ago
Congrats ! If anyone is interested in the clearlinux optimized zstd build config ( imho it is useful for the 1.5.5 building ) https://github.com/clearlinux-pkgs/zstd/blob/main/zstd.spec https://github.com/clearlinux-pkgs/zstd/blob/main/zstd.spec ( CFLAGS, 4 patch )
- voltagex_ 3y ago
  What's stopping those 4 patches from being upstreamed? What advantages or disadvantages am I getting from them?
  - miffe 3y ago
    Looking at it it seems to require avx2. And use the "m" mode for fopen(), which I have no idea what it does, it's not in any documentation I can find.
    - aslatter 3y ago
      From my fopen(2) docs: m (since glibc 2.3) Attempt to access the file using mmap(2), rather than I/O system calls (read(2), write(2)). Currently, use of mmap(2) is attempted only for a file opened for reading.
      - eklitzke 3y ago
        I'm very skeptical that this actually makes things faster. For one, zstd is definitely going to be CPU bound rather than I/O bound. But even if it wasn't, there shouldn't be a performance benefit to reading a file sequentially using mmap compared to reading it sequentially using read, which you can easily verify yourself by writing a small program that just reads files both ways.
        dikei 3y ago
        I think for clear linux, it might be for memory saving in constraint devices rather than for speed optimization.
  - dikei 3y ago
    When a distro maintains patches, it's mostly down to either: * Change default value for some parameters, that can't be upstream due to compatibility reasons. * Change build flag / macro for optimization and/or workaround compiler bugs.
lucb1e 3y ago
> corruptions are detected thanks to the checksum That's good to know because, for important things, I test the archive before throwing the original data away. Not to single out zstd but it's a good opportunity to be reminded: if you make backups, test your backups! A bug like this can also be introduced, not only fixed. I'm not saying it's likely, or that you're likely to be affected by it, but for things you care about, just opening the backup once a year and spot checking some files is not a lot of work for a lot lower risk compared to never checking it. Most backup tools will also let you do checksum validation in an automated fashion, but I prefer to also manually open it and check that it truly works (not that a partial archive has no errors, for example). Anyway, details of the bug are here: https://github.com/facebook/zstd/pull/3517 https://github.com/facebook/zstd/pull/3517 > This [affects] the block splitter. Only levels using the optimal parser (levels 13 and above depending on the source size), or users who explicitly enable the block splitter, are affected. So if you use, for example, zstd -9 (I didn't know it went higher, at least not at somewhat reasonable speeds) or below, then you should always have been fine unless you called the block splitter yourself explicitly (or, perhaps, if your backup software does that for you? It sounds like something relevant for deduplication but I'm not sure what this feature is exactly). > The block splitter confuses sequences with literal length == 65536 that use a repeat offset code. It interprets this as literal length == 0 when deciding the meaning of the repeat offset, and corrupts the repeat offset history. This is benign, merely causing suboptimal compression performance, if the confused history is flushed before the end of the block, e.g. if there are 3 consecutive non-repeat code sequences after the mistake. It also is only triggered if the block splitter decided to split the block. If I understand it correctly, the bug triggers under circumstances where the data causes the splitter to split at exactly 2^16 and then doesn't flush the block, and one example where it doesn't do that is if any part of the next 2^17 bytes (128K) is compressible? Not sure what a "repeat code sequence" is, my lz77-geared brain thinks of a reference that points to an earlier repetition, aka a compressed part.
- koolba 3y ago
  > That's good to know because, for important things, I test the archive before throwing the original data away. Compression corruptions are worse then regular corruption due to the cascading impact. A corrupt sector can be replaced inline but a corrupt compressed file will generally destroy everything downstream from the error. Big +1 to actually verifying the round trip. Backups that aren’t tested through an actual restore are like theoretical war plans vs reality. No plan survives first contact with the enemy.
infogulch 3y ago
I wonder if this affects ZFS
- fcanesin 3y ago
  unlikely it affects high compression
  - rincebrain 3y ago
    OpenZFS uses 1.4.5 currently; the bug was introduced in 1.5.0. So no. OpenZFS does support up to level 19, though I'd generally advise not using it for many reasons, so if it were running the newer version, it could be affected.
marssaxman 3y ago
> due to the nb and complexity of simultaneous conditions Anybody willing to explain what "nb" stands for in this context?
- akx 3y ago
  Number. Common abbreviation around Francophone programmers (see https://fr.wikipedia.org/wiki/NB https://fr.wikipedia.org/wiki/NB: "L'abréviation Nb peut faire référence à : nombre")
  - marssaxman 3y ago
    Thank you! Never would have guessed that on my own.
- zinekeller 3y ago
  Not sure but Naïve Bayes? It might be pointing out that their inputs used in the fuzzing is statistically independent and random that it wouldn't catch some edge cases in real world?
azatom 3y ago
I don't know if it's related, but one time I had corruption too. I was playing with large sparse (ntfs) files and used zstd for temporarily store those. It was probably low compression settings and I couldn't reproduce, so I'd thought I messed up something.
- azatom 3y ago
  oh, I remembered, it was not all zstd fault. Somehow because zstad read not only the allocated space but the whole fole, the zeros too, windows tried to allocate the whole file in memory, which was not enough, and zstd did not give (outofmemory) eror message. (It was not ntfs compressed, only sparse!) Later (maybe windowsupdate happend) tried to reproduce, but worked with larger file with memory. I don't know zstd how could handle the outofmemory, but the sparse reading might not be perfect.

170 points by danlark 3y ago

Congrats to Yann and the team.

Congrats ! If anyone is interested in the clearlinux optimized zstd build config ( imho it is useful for the 1.5.5 building ) https://github.com/clearlinux-pkgs/zstd/blob/main/zstd.spec https://github.com/clearlinux-pkgs/zstd/blob/main/zstd.spec ( CFLAGS, 4 patch )

voltagex_ 3y ago
What's stopping those 4 patches from being upstreamed? What advantages or disadvantages am I getting from them?
- miffe 3y ago
  Looking at it it seems to require avx2. And use the "m" mode for fopen(), which I have no idea what it does, it's not in any documentation I can find.
  - aslatter 3y ago
    From my fopen(2) docs: m (since glibc 2.3) Attempt to access the file using mmap(2), rather than I/O system calls (read(2), write(2)). Currently, use of mmap(2) is attempted only for a file opened for reading.
    - eklitzke 3y ago
      I'm very skeptical that this actually makes things faster. For one, zstd is definitely going to be CPU bound rather than I/O bound. But even if it wasn't, there shouldn't be a performance benefit to reading a file sequentially using mmap compared to reading it sequentially using read, which you can easily verify yourself by writing a small program that just reads files both ways.
      - dikei 3y ago
        I think for clear linux, it might be for memory saving in constraint devices rather than for speed optimization.
- dikei 3y ago
  When a distro maintains patches, it's mostly down to either: * Change default value for some parameters, that can't be upstream due to compatibility reasons. * Change build flag / macro for optimization and/or workaround compiler bugs.

lucb1e 3y ago

> corruptions are detected thanks to the checksum That's good to know because, for important things, I test the archive before throwing the original data away. Not to single out zstd but it's a good opportunity to be reminded: if you make backups, test your backups! A bug like this can also be introduced, not only fixed. I'm not saying it's likely, or that you're likely to be affected by it, but for things you care about, just opening the backup once a year and spot checking some files is not a lot of work for a lot lower risk compared to never checking it. Most backup tools will also let you do checksum validation in an automated fashion, but I prefer to also manually open it and check that it truly works (not that a partial archive has no errors, for example). Anyway, details of the bug are here: https://github.com/facebook/zstd/pull/3517 https://github.com/facebook/zstd/pull/3517 > This [affects] the block splitter. Only levels using the optimal parser (levels 13 and above depending on the source size), or users who explicitly enable the block splitter, are affected. So if you use, for example, zstd -9 (I didn't know it went higher, at least not at somewhat reasonable speeds) or below, then you should always have been fine unless you called the block splitter yourself explicitly (or, perhaps, if your backup software does that for you? It sounds like something relevant for deduplication but I'm not sure what this feature is exactly). > The block splitter confuses sequences with literal length == 65536 that use a repeat offset code. It interprets this as literal length == 0 when deciding the meaning of the repeat offset, and corrupts the repeat offset history. This is benign, merely causing suboptimal compression performance, if the confused history is flushed before the end of the block, e.g. if there are 3 consecutive non-repeat code sequences after the mistake. It also is only triggered if the block splitter decided to split the block. If I understand it correctly, the bug triggers under circumstances where the data causes the splitter to split at exactly 2^16 and then doesn't flush the block, and one example where it doesn't do that is if any part of the next 2^17 bytes (128K) is compressible? Not sure what a "repeat code sequence" is, my lz77-geared brain thinks of a reference that points to an earlier repetition, aka a compressed part.

koolba 3y ago
> That's good to know because, for important things, I test the archive before throwing the original data away. Compression corruptions are worse then regular corruption due to the cascading impact. A corrupt sector can be replaced inline but a corrupt compressed file will generally destroy everything downstream from the error. Big +1 to actually verifying the round trip. Backups that aren’t tested through an actual restore are like theoretical war plans vs reality. No plan survives first contact with the enemy.

infogulch 3y ago

I wonder if this affects ZFS

fcanesin 3y ago
unlikely it affects high compression
- rincebrain 3y ago
  OpenZFS uses 1.4.5 currently; the bug was introduced in 1.5.0. So no. OpenZFS does support up to level 19, though I'd generally advise not using it for many reasons, so if it were running the newer version, it could be affected.

marssaxman 3y ago

> due to the nb and complexity of simultaneous conditions Anybody willing to explain what "nb" stands for in this context?

akx 3y ago
Number. Common abbreviation around Francophone programmers (see https://fr.wikipedia.org/wiki/NB https://fr.wikipedia.org/wiki/NB: "L'abréviation Nb peut faire référence à : nombre")
- marssaxman 3y ago
  Thank you! Never would have guessed that on my own.
zinekeller 3y ago
Not sure but Naïve Bayes? It might be pointing out that their inputs used in the fuzzing is statistically independent and random that it wouldn't catch some edge cases in real world?

azatom 3y ago

I don't know if it's related, but one time I had corruption too. I was playing with large sparse (ntfs) files and used zstd for temporarily store those. It was probably low compression settings and I couldn't reproduce, so I'd thought I messed up something.

azatom 3y ago
oh, I remembered, it was not all zstd fault. Somehow because zstad read not only the allocated space but the whole fole, the zeros too, windows tried to allocate the whole file in memory, which was not enough, and zstd did not give (outofmemory) eror message. (It was not ntfs compressed, only sparse!) Later (maybe windowsupdate happend) tried to reproduce, but worked with larger file with memory. I don't know zstd how could handle the outofmemory, but the sparse reading might not be perfect.