Skip to content

Add error checking to zstd hdf5 plugin #280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pimlu wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add error checking to zstd hdf5 plugin #280

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ZSTD/src/H5Zzstd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

@pimlu pimlu Jul 30, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mkitti mkitti Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pimlu pimlu Aug 3, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mkitti mkitti Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pimlu pimlu Aug 4, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if (decompSize == 0 || decompSize == ZSTD_CONTENTSIZE_UNKNOWN ||
decompSize == ZSTD_CONTENTSIZE_ERROR)
goto error;

if (NULL == (outbuf = malloc(decompSize)))
goto error;

decompSize = ZSTD_decompress(outbuf, decompSize, inbuf, origSize);
if (ZSTD_isError(decompSize))
goto error;

#ifdef ZSTD_DEBUG
fprintf(stderr, " decompressing nbytes: %ld\n", decompSize);
Expand Down Expand Up @@ -104,6 +110,8 @@ H5Z_filter_zstd(unsigned int flags, size_t cd_nelmts, const unsigned int cd_valu
goto error;

compSize = ZSTD_compress(outbuf, compSize, inbuf, origSize, aggression);
if (ZSTD_isError(compSize))
goto error;

#ifdef ZSTD_DEBUG
fprintf(stderr, " compressing nbytes: %ld\n", compSize);
Expand Down
Loading