Add error checking to zstd hdf5 plugin - #280
Conversation
This PR adds error checking to the zstd hdf5 plugin. Previously it would return uninitialized memory from malloc when any zstd error occurred. It also prevents proceeding with the result of malloc(0) in the abnormal case where decompSize == 0.
| @@ -66,10 +66,16 @@ H5Z_filter_zstd(unsigned int flags, size_t cd_nelmts, const unsigned int cd_valu | |||
| if (flags & H5Z_FLAG_REVERSE) { | |||
| /* We're decompressing */ | |||
| size_t decompSize = ZSTD_getFrameContentSize(*buf, origSize); | |||
There was a problem hiding this comment.
Technically decompSize should be an unsigned long long but on 64-bit systems the outcome is the same. Let me know if I should update this in the same PR.
There was a problem hiding this comment.
Please fix that here.
Below, an unknown content size should not be treated as an error. That is a perfectly valid situation for Zstandard compressed data.
We may need to use the streaming API to complete decompression, however.
There was a problem hiding this comment.
Is that to support if some other program compressed hdf5 data via the streaming API? Since currently ZSTD_CONTENTSIZE_UNKNOWN is unreachable right, at least if hdf5_plugins is doing the compression. ZSTD_compress will always set the decompressed size
There was a problem hiding this comment.
As you have seen, there are multiple implementations of the HDF5 Zstandard plugin. We cannot guarantee that the streaming API will not be used.
I've added support for streaming decompression in the past in a couple of libraries:
https://github.com/zarr-developers/numcodecs/blob/82c36354abf7f1c1999bc1c3cd11571215a6f364/src/numcodecs/zstd.pyx#L228
https://github.com/zarr-developers/numcodecs/blob/82c36354abf7f1c1999bc1c3cd11571215a6f364/src/numcodecs/zstd.pyx#L279-L369
https://github.com/manzt/numcodecs.js/blob/main/codecs/zstd/zstd_codec.cpp#L56
If you do not want to do right now, that's fine. However, the "error message" should be appropriate. The stream is not corrupt, but streaming decompression is not enabled here.
There was a problem hiding this comment.
However, the "error message" should be appropriate.
Oh whoah, we can report error messages now (since HDFGroup/hdf5#6539, two weeks ago)?
We would love to report error messages (in general, not just for streaming decompression). My original PR used H5Epush2 until I found that errors didn't bubble up to python because of the H5E_PAUSE_ERRORS. Is it okay if I call H5Epush2(H5E_DEFAULT, ...) for every error case with a message?
|
Hi @pimlu, could you also rebase this branch off of the current master? The CI workflows needed a couple fixes to make testing work again after changes in HDF5. |
3 participants
This PR adds error checking to the zstd hdf5 plugin. Previously it would return uninitialized memory from malloc when any zstd error occurred.
It also prevents proceeding with the result of
malloc(0)in the abnormal case wheredecompSize == 0.