Make an installed SZ3 usable through find_package, stop a crash in the HDF5 filter, and vendor Zstd - #150
Open
ayzk wants to merge 65 commits into
Open
Make an installed SZ3 usable through find_package, stop a crash in the HDF5 filter, and vendor Zstd#150ayzk wants to merge 65 commits into
ayzk wants to merge 65 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The bundled Zstd target publicly propagates private visibility overrides that can interfere with consumers’ own Zstd headers and linking.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR makes installed SZ3 packages consumable via find_package(SZ3) and fixes HDF5 filter property-list handling to prevent crashes.
Changes:
- Reworks exported dependencies, target aliases, ABI checks, and RPATH handling.
- Bundles Zstd privately at version 1.5.7.
- Adds regression coverage for installed-package consumption and HDF5 behavior.
File summaries
| File | Description |
|---|---|
CMakeLists.txt |
Configures packaging metadata and install RPATH behavior. |
tools/zstd/CMakeLists.txt |
Builds and installs private bundled Zstd. |
tools/H5Z-SZ3/CMakeLists.txt |
Updates HDF5 detection, ABI checks, and exports. |
SZ3Config.cmake.in |
Resolves dependencies and exposes compatible targets. |
tools/H5Z-SZ3/src/H5Z_SZ3.cpp |
Detects filters on individual property lists safely. |
include/SZ3/lossless/Lossless_zstd.hpp |
Selects bundled or system Zstd headers. |
.github/workflows/cmake.yml |
Adds installed-package and bundling regression checks. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+102
to
+111
| target_compile_definitions(zstd | ||
| PUBLIC | ||
| SZ3_BUNDLED_ZSTD=1 | ||
| ZSTDLIB_VISIBILITY= | ||
| ZSTDERRORLIB_VISIBILITY= | ||
| PRIVATE | ||
| # The x86-64 assembly Huffman decoder arrived in 1.5.x as a .S that would need | ||
| # enable_language(ASM) and an architecture guard, and that MSVC -- the one platform where | ||
| # this bundled copy is the default -- never builds. Take the C path everywhere. | ||
| ZSTD_DISABLE_ASM=1) |
set_SZ3_conf_to_H5 and get_SZ3_conf_from_H5 both branched on H5Zfilter_avail(), which reports whether the filter is registered with the library, not whether it is on this property list. Once H5Zregister() has run or the plugin has been loaded it answers yes for every list, so a freshly created dataset creation property list took the modify path in set_ and faulted inside HDF5, while get_ read from a list with no filter on it. get_ also ignored H5Pget_filter_by_id's return. A failed read leaves cd_nelmts at its input value, which is not zero, so the caller's Config was overwritten from the zero-filled buffer instead of being left alone. Co-Authored-By: Claude Opus 5 <[email protected]>
find_package(SZ3) on an installed SZ3 did not work. The export named imported targets that exist only inside the build that made them (PkgConfig::ZSTD, MPI::MPI_CXX), it offered only the namespaced names, its config file ended the consumer's configure from inside an optional probe, it baked in this machine's HDF5 include path, and the filter it installed recorded no RUNPATH. The bundled Zstd installed a libzstd and a zstd.h over the ones already in the prefix and exported its symbols into the consumer's process. CI now installs SZ3 and builds a separate project against it, which is how all of the above were found. Co-Authored-By: Claude Opus 5 <[email protected]>
find_package(GSL) defined SZ3_ENABLE_GSL and linked GSL::gsl for one header, preprocessor/Wavelet.hpp, which nothing in the tree includes -- no ALGO uses it, no pipeline composes it, no test builds it, and no CI job enables GSL, so it has never been compiled. Its cost was a fatal find_package(GSL REQUIRED) inside every consumer's optional find_package(SZ3) probe, because libgsl-dev is common enough on a build host that a packaged SZ3 records GSL_FOUND=TRUE. The header goes with the dependency rather than staying behind: with nothing defining SZ3_ENABLE_GSL, every line of it is inside a block no supported build can enter. It also discards the transform coefficients past index n, so it is not a round trip for any length that is not a power of two. git has it if someone wants to bring it back with a composition and a test. Co-Authored-By: Claude Opus 5 <[email protected]>
ayzk
force-pushed
the
zstd-export-fix
branch
from
September 18, 2026 02:34
efe6ed5 to
9074dc0
Compare
…and a version Three things an application that links the filter, rather than reaching it through HDF5_PLUGIN_PATH, could not do. H5Z_SZ3_initialize()/H5Z_SZ3_finalize() follow H5Z-ZFP's shape: an H5Zfilter_avail guard, H5Zregister, and an unregister that only takes back what this library put there. Until now nothing in the public header was a function a consumer had to call, so GROMACS linked SZ3::hdf5sz3 without referencing a single symbol from it, and on a compiler driver that passes --as-needed the dependency was dropped outright. initialize() separates the two ways it can succeed: 1 when it registered the filter, 0 when one was already there and it deferred. Zero is worth logging. HDF5 holds one filter per id, and a different build of this same filter -- the copy hdf5plugin ships under id 32024, say -- writes cd_values this one rejects and files this one cannot read. The install now also drops a copy of the library in lib/plugin, which HDF5 can be pointed at on its own. Pointing HDF5_PLUGIN_PATH at the whole install lib/ instead is destructive -- the variable replaces HDF5's compiled-in plugin directory rather than extending it -- and expensive, because HDF5 dlopens every lib*.so in each directory on the path. H5Z_SZ3_PLUGIN_INSTALL_DIR follows H5Z-ZFP's cache-variable precedent. The H5Z_class2_t name now carries the version, because that string is what lands in the file, what h5dump and h5ls report back with no plugin present at all, and what HDF5 names in "required filter '...' is not registered". Co-Authored-By: Claude Opus 5 <[email protected]>
h5repack is how most users apply a filter without writing code, and it drives H5Z_sz3_set_local through cd_values typed on a command line, which set_SZ3_conf_to_H5 never does. h5dump and h5ls are what they reach for when it goes wrong. Neither was covered, nor were the three shapes an application can take: linking and calling H5Z_SZ3_initialize(), linking but relying on HDF5_PLUGIN_PATH, and doing both. The negative cases are the point. h5repack applied with the filter unreachable exits 0 and writes an unfiltered copy; h5repack -f NONE in the same state exits 0 and drops the dataset outright. Both are asserted here so that a change making them worse is visible. The script asserts which path was taken rather than only an exit code, and the DT_NEEDED check reports itself inconclusive rather than passing when the compiler driver keeps unreferenced libraries anyway. Co-Authored-By: Claude Opus 5 <[email protected]>
The bundled copy is not a fallback for rare platforms; it is what most people who build SZ3
from source get. pkg_search_module(ZSTD) needs libzstd.pc, which ships in libzstd-dev --
Priority optional on Debian and Ubuntu, not installed by default -- while the libzstd1
runtime it would link is Priority required and always present (checked on Ubuntu 24.04:
`apt-cache show libzstd1 libzstd-dev`). macOS carries no zstd.h in /usr/include or the Xcode
SDK, and Windows carries neither. So `git clone && cmake && make` reaches the bundled copy on
all three unless the builder installed a development package on purpose. It was 1.4.5, from
2020.
1.5.6 rather than the current 1.5.7, on compression THROUGHPUT rather than output size.
First, Zstd on its own -- Lossless_zstd timed directly, no file I/O in the timed region, on lab1 (x86-64, BMI2),
pinned to one core, variants interleaved, best of 5 over 4 passes:
92,989,003 B payload -- the exact bytes ALGO_INTERP hands Zstd for a 1.07 GB HACC field
zstd output bytes zstd ratio compress MB/s decompress MB/s
1.4.5 90,125,005 1.0318 568.6 1248.0
1.5.6 90,125,789 1.0318 672.3 1496.7
1.5.7 90,641,684 1.0259 420.7 1689.3
167,865,204 B of raw MD floats -- what ALGO_LOSSLESS hands Zstd
1.4.5 155,106,115 1.0823 774.8 1222.4
1.5.6 155,108,273 1.0822 940.9 1485.7
1.5.7 155,100,676 1.0823 565.1 1238.8
Read those as two separate axes, because only one of them moves much, and then read the
third table below, because throughput measured on Zstd alone is not what a user feels.
Throughput, Zstd alone (MB/s): over four payloads 1.5.6 compresses 15-38% faster and
decompresses 2-27% faster than 1.4.5, while 1.5.7 compresses 26-30% SLOWER than 1.4.5.
Output size (bytes): all three sit within about half a percent of each other. 1.5.6 is
within 0.002% of 1.4.5 on every payload. 1.5.7 ranges from 0.573% more bytes on the first
payload above (90,125,005 B -> 90,641,684 B, ratio 1.0318 -> 1.0259) to 0.004% fewer on
the second.
What that is worth end to end depends entirely on the algorithm, because SZ3 interpolates,
quantises and Huffman-codes before Zstd sees anything. Measured, not inferred: a Timer around
the one lossless call in SZGenericCompressor, against the SZ_compress/SZ_decompress time the
sz3 CLI already reports (which excludes file I/O). SZ3_DEBUG_TIMINGS could not answer this --
built with it ON, the default path prints no stage timings at all, every timer.stop() on that
path being absent or commented out. Seconds, best of 5, one quiet machine:
ALGO_INTERP_LORENZO, the default path -- Zstd is a few percent of the work
dataset zstd SZ_compress Zstd share SZ_decompress Zstd share
md 1.4.5 0.7826 s 4.2 % 0.7128 s 2.2 %
md 1.5.6 0.7742 s 3.4 % 0.7015 s 2.2 %
md 1.5.7 0.8073 s 7.0 % 0.7011 s 2.2 %
hacc 1.4.5 1.3432 s 4.5 % 1.0896 s 1.2 %
hacc 1.5.6 1.3391 s 3.7 % 1.0773 s 1.2 %
hacc 1.5.7 1.3808 s 6.9 % 1.0839 s 1.8 %
ALGO_LOSSLESS -- Zstd is the whole algorithm (SZDispatcher calls Lossless_zstd directly)
md 1.4.5 0.2861 s 0.1722 s
md 1.5.6 0.2500 s (-12.6 %) 0.1604 s (-6.9 %)
md 1.5.7 0.3701 s (+29.4 %) 0.1852 s (+7.5 %)
hacc 1.4.5 0.7264 s 0.2661 s
hacc 1.5.6 0.6485 s (-10.7 %) 0.2544 s (-4.4 %)
hacc 1.5.7 0.7903 s (+8.8 %) 0.2681 s (+0.8 %)
So 1.5.7's Zstd regression costs only 2.8-3.2% of SZ_compress on the default algorithm, and
nothing on decompression; it costs 8.8-29.4% on ALGO_LOSSLESS, where Zstd is all there is.
1.5.6 is faster than 1.4.5 in every cell of both tables. That, rather than any disqualifying
fault in 1.5.7, is why 1.5.6: it is the fastest of the three everywhere measured and matches
1.4.5's output size. If 1.5.7's one listed fix -- "compression bug in 32-bit mode associated
with long-lasting sessions" -- matters for 32-bit builds, the cost of taking 1.5.7 instead is
about 3% of SZ_compress on the default path and up to 29% on ALGO_LOSSLESS, and that is a
judgement call rather than a measurement.
Where the throughput went: 1.5.7's changelog lists "compression ratio improvement for dfast,
aka levels 3 and 4" and "better block boundaries", and level 3 is the level Lossless_zstd
uses. Building 1.4.5, 1.5.0, 1.5.2, 1.5.4, 1.5.5, 1.5.6 and 1.5.7 with Zstd's own makefile
puts the drop at 1.5.7 and nowhere earlier, and Zstd's own build of 1.5.7 is as slow as this
one, so it is upstream's trade rather than a mistake in how this file builds it.
Streams cross between the versions unchanged. Every ALGO_* against 1D, 2D and 3D framings of
a 4187x3341x3 MD trajectory and a 256 MiB HACC field, at three error bounds -- 126
combinations -- compressed with each build and decompressed with both: all 252 reconstructions
byte-identical to the one from the build that wrote the stream. Compressed SZ3 file size over
those 126, in bytes against the same 1.4.5 baseline: 1.5.6 mean -0.027%, best -0.770%, worst
+0.045%; 1.5.7 mean -0.155%, best -1.855%, worst +1.041%. Neither is a throughput figure, and
neither is large: 1.5.6's worst case is md 3D ALGO_INTERP_LORENZO at eb 1e-2, 19,115,368 B ->
19,123,929 B, a ratio of 8.782 -> 8.778.
zstd_decompress.c was listed twice; it is listed once now.
ZSTD_DISABLE_ASM is new and required: 1.5.x added decompress/huf_decompress_amd64.S, and
without it huf_decompress.c leaves two HUF_decompress4X*_fast_asm_loop symbols undefined on
x86-64. Assembling that file would need enable_language(ASM), for a language MSVC does not
assemble. The cost was measured, not assumed: built both ways on a quiet machine, twelve
samples per case, the assembly loop decompresses 0.1-2.0% faster on three payloads and 10.6%
faster on the fourth -- and the C loop still leaves 1.5.6 faster than 1.4.5 everywhere.
The CI half is that nothing said which Zstd was under test. The Linux job installed no
development package, so whether it exercised the system path or the bundled one was whatever
the runner image happened to carry -- and either way it never said so. It now installs
libzstd-dev and asserts sz3 links a system libzstd with no bundled archive built; the bundled
step, which already existed to keep that copy out of the install prefix, now asserts the
converse and round-trips testfloat_8_8_128.dat through it, because an archive that exists is
not an archive that works. Both assertions were run against real builds of each kind, so each
is known to fail on the other path rather than passing regardless.
Two checks in that job were written `! grep ...`. Bash exempts a !-inverted command from
set -e, so unless it is the last line of the step its failure is dropped: neither could ever
have failed. They are `if grep ...; then exit 1; fi` now.
Co-Authored-By: Claude Opus 5 <[email protected]>
A consumer of an installed SZ3 could not compile unless zstd.h was on their include path, and
outside /usr/include -- Homebrew, conda, Spack -- it is not, because the header lives in a
development package they have no reason to have installed. The export then made it worse by
naming the build machine's libzstd, which stops existing the moment that prefix does. And the
bundled copy was downloaded at configure time from a URL with no hash, so an offline build
failed outright and no build was reproducible.
Five changes, because they are one problem seen from five sides.
Lossless_zstd.hpp declares the four Simple-API functions SZ3 calls instead of including
<zstd.h>. They are byte-identical in every release from 1.4.5 to 1.5.7 and covered by Zstd's
stable-API guarantee. An installed SZ3 now needs Zstd's library and never its header, which
fixes the consumer compile however SZ3 was built. SZ3_USE_ZSTD_HEADER takes the real header
instead. SZImpl.hpp, SZImplOMP.hpp and SZDispatcher.hpp called ZSTD_compressBound directly,
reaching a declaration they never made through whatever chain of headers happened to include
this one; they call Lossless_zstd::compress_bound now, so every Zstd symbol is in one file.
Not a virtual on LosslessInterface: a new pure virtual breaks every out-of-tree implementer.
test_zstd_decl compiles those declarations and the real zstd.h into one translation unit, and
zstd_decl_header_first.cpp does it in the opposite order, so a future divergence is a build
failure rather than a silent ABI mismatch. Verified by breaking it on purpose: changing
ZSTD_compressBound's parameter to int fails with "conflicting types for 'ZSTD_compressBound'".
Both include orders sit behind clang-format off/on, because clang-format sorts includes and
sorting them made both files test the same order -- caught by re-running it, not by luck.
tools/zstd holds the source now. FetchContent downloaded zstd-1.5.6.tar.gz at configure time
with no URL_HASH: offline builds failed, nothing verified what arrived, and a distro packager
had to patch the mechanism out. lib/common, lib/compress and lib/decompress are copied byte for
byte from the release tarball (SHA256 8c29e06c...), so diff -r against it proves the copy is
unmodified -- which is why whole directories were copied rather than files picked out of them.
lib/deprecated, lib/legacy and lib/dictBuilder are not here: SZ3 calls none of them and they
were 14 of the 39 files this used to compile. ZSTD_LEGACY_SUPPORT=0 is now stated rather than
left to the default, because lib/legacy is gone and two decompress files include it when it is
set. The library's form is unchanged: STATIC, OUTPUT_NAME sz3_zstd, hidden visibility, both
visibility macros, position-independent. That is what keeps its symbols from interposing on a
consumer's own Zstd, and it was not touched.
Nothing includes zstd.h any more, so the SZ3/bundled_zstd/ staging and its install(FILES) are
gone. They existed only to keep the bundled header off a consumer's include path; with no
header installed at all there is nothing to keep off it.
Finding Zstd is now a cascade -- find_package(zstd CONFIG), then pkg-config, then find_library,
then the vendored copy -- because no one of them is everywhere. zstdConfig.cmake is absent on
RHEL/Rocky/Alma 8-10, Fedora <=43, Ubuntu 20.04 and 22.04, Debian bullseye and default Spack;
libzstd.pc needs libzstd-dev, which Debian and Ubuntu do not install by default. find_library
needs neither, and since SZ3 no longer needs a header it is now a sufficient answer.
tools/zstd can be deleted outright and a system build still works.
The export carries only what a consumer can resolve. A system Zstd goes in under
$<BUILD_INTERFACE:> -- the link as well as the includes, which is the half that was missing --
and SZ3Config.cmake runs the same cascade again on the consumer's machine, soft-failing with
SZ3_NOT_FOUND_MESSAGE and SZ3_FOUND=FALSE rather than ending a configure that may well be an
optional QUIET probe. The vendored Zstd needs none of that: it exports as SZ3::zstd under
${_IMPORT_PREFIX}, so it relocates with the prefix.
1.5.6 rather than 1.5.7 still, and after vendoring there is no URL left to carry why, so the
reasoning and its expiry condition are in tools/zstd/CMakeLists.txt. In short: 1.5.7's only
listed correctness fix caps a 32-bit window index that SZ3 cannot drive past its limit, because
SZ3 calls one-shot ZSTD_compress, which builds a fresh ZSTD_CCtx per call. Measured: 5 GB
through one-shot calls reaches max curr = srcSize + 2 at 64 MiB, 256 MiB and 1 GiB per call,
while the same 5 GB through one reused ZSTD_CCtx reaches ZSTD_CURRENT_MAX. The same defect is
in the 1.4.5 this replaced. It expires if SZ3 ever holds a ZSTD_CCtx across calls.
CI asserts which Zstd each leg took from SZ3_ZSTD_PROVIDER rather than inferring it from ldd,
checks that no zstd.h is installed under any name, that the generated export names no libzstd
at all, that a consumer still builds after the build tree is deleted, and that a packager who
deletes tools/zstd can still build against the system Zstd.
Co-Authored-By: Claude Opus 5 <[email protected]>
h5repack, h5dump and h5ls cover how a user reaches this filter without writing any code, so the suite cannot report a result without them. libhdf5-dev does not carry them, hdf5-tools does, and the runner had neither: the suite answered with twelve `command not found` failures rather than one actionable line. Co-Authored-By: Claude Opus 5 <[email protected]>
char's signedness is implementation-defined. Where it is signed, a cd_values byte of 0xF3 reaches N as -13 and passes `N > 4`. The later guards do not stop it either: with bitWidth 0 the dim_bytes computation wraps to 0, so a negative N reaches bytes2vector, which asks for 1.8e19 elements and throws a bare std::length_error with no diagnosis of what was wrong with the input. Co-Authored-By: Claude Opus 5 <[email protected]>
The comment claimed the array was the layout hdf5plugin ships, nine elements against this filter's eight. Neither number holds: hdf5plugin ships v3.1.7, which writes fourteen for that dataset, and this filter writes fifteen. The count is dataset-dependent and has been 9, 11, 13, 14 and 15 across releases, so it identifies nothing and the check does not rest on it. Co-Authored-By: Claude Opus 5 <[email protected]>
Proving the export outlives the build tree meant deleting build/, but the steps that run sz3 on the downloaded dataset have working-directory: build. The job had never reached them before, because it stopped earlier on missing HDF5 tools. Moving the directory aside proves the same thing: it genuinely does not exist while the consumer configures, builds and links. Co-Authored-By: Claude Opus 5 <[email protected]>
The build resolved Zstd through three routes and the consumer re-ran all three plus a build-time search hint. One route each is enough. Build side: pkg-config only, as on master. find_package(zstd CONFIG) was never the only route to a system Zstd -- zstd's own Makefile and CMake installs both write libzstd.pc -- and the find_library rung never won in any of the nine environments measured, because the one packaging shape that has a libzstd without a .pc (Debian's libzstd1 with no libzstd-dev) has no dev symlink for find_library to find either. Consumer side: find_library only. Its suffix order prefers a shared Zstd without the BUILD_SHARED_LIBS reasoning the imported-target route needed, and it searches exactly the prefixes find_package(SZ3) itself came from. The build-time libdir hint goes with it, which takes the last build-machine path out of the installed package. CMakeLists.txt 252 -> 151, SZ3Config.cmake.in 84 -> 46, tools/zstd/CMakeLists.txt 133 -> 88. Co-Authored-By: Claude Opus 5 <[email protected]>
The path emitted the same error three times: a printf to stdout, a std::cerr, and the throw. The stdout write is the damaging one -- inside the HDF5 filter it lands in whatever the user redirected, so `h5dump -d /ds f.h5 > out.txt` gets SZ3's diagnostic written into the data file. The other two are duplicates: the filter already turns what() into an entry in HDF5's error stack, and the CLI prints it. The message keeps what the printf carried -- the program version, the data version it reads, and the version found -- so nothing diagnostic is lost. Co-Authored-By: Claude Opus 5 <[email protected]>
The version check in SZImplOMP.hpp is a copy of the serial one, so the OpenMP decompression path wrote the same diagnostic to the caller's stdout. The iterator's dimension check did it with std::cout, and threw away the two counts it printed rather than putting them in the message. Iterator.hpp gains <sstream>, which it now uses directly. Its print() keeps std::cout: printing is what the caller asks it for. Co-Authored-By: Claude Opus 5 <[email protected]>
H5Dread's status was stored and then ignored: on failure the tool printed twenty uninitialized floats as "reconstructed data", wrote them into a new HDF5 file, and reached a trailing check that reported the failure and called exit(0). A caller saw a complete output file and a success status. The status is checked where it is produced, so nothing downstream runs on a buffer that was never filled, and ERROR() returns non-zero like every other failure in this file. The trailing check is unreachable once the reads bail out. Co-Authored-By: Claude Opus 5 <[email protected]>
cd_values carries neither a magic number nor a version, and its layout has changed five times across released versions, so on read it was parsed under this build's assumed layout before anything established the data came from a compatible SZ3. What refused a v3.2.0, v3.2.1 or v3.3.0 file was Config::load's own guards -- one of which, the dimension count, did not catch a negative value until this branch fixed it -- and the message they produce, "invalid number of dimensions", reads as a corrupt file rather than a version mismatch. Consulting the payload header first refuses those three with the version that wrote them and the version this build reads. The magic number is a precondition rather than a requirement: a chunk the conf.num < 20 path stores raw has no header, and conf.num is not known until cd_values has been parsed, so a missing magic cannot be told apart from a pass-through chunk here. Files older than v3.2.0 carry no magic and so are still refused by Config::load, as before. Co-Authored-By: Claude Opus 5 <[email protected]>
A reader compares SZ3_DATA_VERSION for exact equality, so a format change that does not move it produces files an older build accepts and then misreads. v3.3.0 and v3.3.1 shipped different layouts under the same declared 3.3.0 and nothing caught it: the only digest CI had asks whether Linux and macOS agree with each other, and a format change moves both together. The recorded digest is of a bundled-Zstd build, reusing the tree the step above already produces. The bytes depend on which Zstd compressed them -- system Zstd 1.5.5 and 1.5.7 differ by two bytes on this input -- and only the bundled one is pinned by this repository, so anything else would make the check depend on the runner image. Demonstrated in all four states: unchanged passes, a serialized field added without a version bump fails, a version bumped without a row fails, and reverting passes again. Co-Authored-By: Claude Opus 5 <[email protected]>
A third of this branch's added lines were comments. Most restated their own code or recounted how a defect was found. What is kept is the set a later edit would trip over: that Lossless_zstd.hpp declares Zstd rather than including it, that H5Zfilter_avail() registers rather than reports, that the magic number is a precondition and not a requirement, that char's signedness is why a dimension count is compared as unsigned, why the bundled Zstd is named libsz3_zstd with hidden symbols, and the two shell forms in CI that look like checks and cannot fail. Two clang-format reflows of untouched lines are reverted with them. Co-Authored-By: Claude Opus 5 <[email protected]>
The comments said what happens. A comment on a guard is read by someone about to remove the guard, so it has to say what not to write: not `N > 4`, not #include <zstd.h>, not H5Zfilter_avail(), and no printf inside a filter whose stdout is the user's output file. Co-Authored-By: Claude Opus 5 <[email protected]>
Two of these stacked negatives -- "never needs ... would not have", "which they do not ... not its header" -- and trailed into references a reader has no way to resolve at that point. The instruction is the first sentence; the rest is one reason, stated positively. Co-Authored-By: Claude Opus 5 <[email protected]>
ayzk
force-pushed
the
zstd-export-fix
branch
from
September 18, 2026 22:20
a89170e to
b109e43
Compare
Co-Authored-By: Claude Opus 5 <[email protected]>
It guarded against Zstd changing the signatures of its four Simple-API functions. Those are ABI-stable across 1.x and the assumption is taken as given, which removes the only thing in the tree that needed a real zstd.h and with it the SZ3_ZSTD_HEADER_DIRS search and its can-be-empty branch. Co-Authored-By: Claude Opus 5 <[email protected]>
SZ3_ZSTD_PROVIDER was a string saying what SZ3Targets.cmake already shows: the vendored build exports SZ3::zstd carrying its own location under the install prefix, and the system build exports nothing for Zstd at all. Two records of one fact can disagree; one cannot. SZ3Config.cmake now asks whether SZ3::zstd exists, and the three CI checks ask the export instead of grepping for the string. Co-Authored-By: Claude Opus 5 <[email protected]>
Two option() calls differing only in their default value. option() leaves an existing cache entry alone, so -DSZ3_USE_BUNDLED_ZSTD still wins either way. Co-Authored-By: Claude Opus 5 <[email protected]>
Co-Authored-By: Claude Opus 5 <[email protected]>
sz3ToHDF5, dsz3FromHDF5, convertBinToHDF5 and cdvalueHelper all print a usage line and assert nothing. They are command-line tools that happened to live in a directory called test, which left the one thing that does assert -- filterAccessModes.sh -- indistinguishable from them. The four move to tools/H5Z-SZ3/tools. test/ keeps the suite and the ctest wrapper that installs a prefix for it to run against. Co-Authored-By: Claude Opus 5 <[email protected]>
…atic tools/H5Z-SZ3/CMakeLists.txt added test/ unconditionally, so the filter's tests were configured whenever the filter was, while the rest of the tree's tests sit behind BUILD_TESTING. The guard inside the directory was doing that job instead. Now the directory is gated and the guard asks only what it should: whether h5repack, h5dump and h5ls are there. sz3_filter_on_plist sat in an anonymous namespace while everything else file-local in that file is static. Co-Authored-By: Claude Opus 5 <[email protected]>
The pxd mirrored a one-argument load; the real one takes the remaining buffer length as well. Cython trusts a cdef extern declaration rather than checking it, so this only shows up as a compile failure the first time anything calls it -- nothing in sz.pyx does. Checked the rest of the mirror while there: every other signature and field type matches. Co-Authored-By: Claude Opus 5 <[email protected]>
build-linux had grown to fourteen steps and three hundred lines: a build, a
consumer, an export audit, a packager's deletion, the bundled Zstd's promises, a
format digest, and a round trip. A red X on it said nothing about which.
cmake.yml one full-feature build per platform: configure everything on,
build, ctest, round-trip the smoke dataset, compare digests
packaging.yml what an installed SZ3 hands someone else -- the export, the
consumer shapes, the bundled Zstd, the format digest
hdf5.yml the filter against more than one HDF5: three versions, a
parallel one, and two installed side by side
No step was dropped. Three are new, all in hdf5.yml: a second HDF5 the build
does not see, and a consumer that has to compile against its own rather than
ours -- which is the case the BUILD_INTERFACE guard exists for, and which no
single-HDF5 machine can show.
Co-Authored-By: Claude Opus 5 <[email protected]>
The name is written into every file's object header, so it costs bytes there and makes two patch releases produce different headers for identical data. The library version already implies the format version, and cd_values carries the format version where a decoder can act on it. Of the filters registered with HDF5, one puts any version in this field and none puts a format version. Co-Authored-By: Claude Opus 5 <[email protected]>
A header-only library sees more of a compiler than most: every template it has is instantiated in the consumer's translation unit, so a warning or a language difference shows up there rather than here. Everything until now built under whichever compiler the runner shipped. gcc-12, gcc-14, clang-16 and clang-18, each with BUILD_SHARED_LIBS on and off, and an assertion that the leg produced the shape it asked for. Quoted in the matrix because YAML reads a bare ON as a boolean, which would have made that assertion compare against "true" and never fire. Co-Authored-By: Claude Opus 5 <[email protected]>
HDF5 writes this string into every dataset that uses the filter and quotes it back when the filter is missing, which is the moment someone needs to know where to get it. Measured: at 100 and at 1000 datasets, lengthening it costs zero bytes -- the object header absorbs it. Co-Authored-By: Claude Opus 5 <[email protected]>
Every template SZ3 has is instantiated in the consumer's translation unit, so its warnings land in their build. GROMACS carries 24 suppression lines for them. Renames constructor parameters that shadowed the member they initialise, following the trailing-underscore spelling already used in BlockwiseIterator and Iterator; drops the names of parameters nothing reads; gives six files the newline they were missing; marks the xdr helpers inline, since a static in a header that only a template calls is one clang flags as unneeded; parenthesises the && inside || in HuffmanEncoderV2, which already meant what it looked like; and moves the H5Z filter callbacks out of the public header, where every consumer saw them declared and never defined. Co-Authored-By: Claude Opus 5 <[email protected]>
…or it Splitting build-linux into three left packaging.yml with only BUILD_TYPE while the steps that moved into it still read DIMS, MODE and TOL. GitHub interpolates an undeclared key to the empty string, so the compressor ran as sz3 -f -i tools/sz3/testfloat_8_8_128.dat -z _n.sz3 -3 -M and answered with its usage text and a non-zero exit -- which reads as a compression failure rather than a workflow that lost its arguments. An interpolation cannot be made to fail, so the step that feeds those values to a program now names a key that is missing before running it, and a check over every workflow catches one that was never declared or is declared empty. Run against the three files as the split left them it reports the eight references that broke this job. Co-Authored-By: Claude Opus 5 <[email protected]>
SZ3Targets named OpenMP::OpenMP_CXX, and SZ3Config asked FindOpenMP for the CXX component to supply it. FindOpenMP can only produce a target for a language the consumer enabled, so a C-only project got Could NOT find OpenMP (missing: OpenMP_CXX_FOUND CXX) SZ3Config.cmake:51 (find_dependency) and its configure ended there. That is what the parallel-HDF5 job hit: its consumer is C, for mpicc, and MPI had nothing to do with it. The export no longer names the target. SZ3Config asks for whichever of C and C++ the consumer enabled -- one runtime serves both on every compiler SZ3 builds under -- and appends it to SZ3::SZ3, the way the Zstd dependency above it is already handled. The check that should have caught this was written as find_package(SZ3 QUIET) with the executable inside if (SZ3_FOUND), so it built nothing in exactly the case where a C-only consumer is broken, and passed. It is REQUIRED now, and the binary it names has to exist. Co-Authored-By: Claude Opus 5 <[email protected]>
The new shared=OFF matrix legs turned 16 of the 31 access-mode checks red on their first run, and they were right to: BUILD_SHARED_LIBS=OFF installs libhdf5sz3.a, there is no shared object for HDF5 to dlopen, and every mode that goes through HDF5_PLUGIN_PATH has nothing to reach. Not a configure-time error for that combination: the archive is what an application links to call H5Z_SZ3_initialize() itself, which is a whole access mode and still passes, and the matrix leg asserts that archive exists. Not a plugin forced shared regardless either: that overrides what the flag was set for, and H5Z_SZ3_PLUGIN_INSTALL_DIR already offers an install with no plugin, so this suite has to describe that tree whichever way the question is settled. So the modes are skipped, each one named, each one counted, and the total asserted at the bottom, because a suite that quietly reports fewer checks is what this file exists to prevent. What decides the skip is the shape of the installed library rather than the plugin's absence: a shared hdf5sz3 with an empty plugin directory is the install rule being wrong and now says so. Under MSYS2 the prefix reached cmake as /d/a/... , which a native Windows cmake does not read, so it found SZ3 through the build tree on PATH instead and failed on an SZ3Targets.cmake that only exists once installed. Paths handed to cmake and to HDF5 go through cygpath where there is one. Co-Authored-By: Claude Opus 5 <[email protected]>
integration_test.yml has had this since it was written; cmake.yml, packaging.yml and hdf5.yml never got it. Three pushes to this branch inside 24 minutes each started a full set, and all three ran to completion, around 40 jobs spent on two revisions nobody was waiting for. Co-Authored-By: Claude Opus 5 <[email protected]>
…ey are absent ::set-output is deprecated and warned about on every run. When it stops working the two jobs will still finish, the compare job will read two empty strings, and "" != "" is false -- it would print that Linux and macOS produced identical output having compared nothing at all. The digests go through $GITHUB_OUTPUT instead, each job refuses to publish an empty one, and the compare job refuses to accept one. Co-Authored-By: Claude Opus 5 <[email protected]>
An installed SZ3 is reached two ways by GROMACS: mainline finds it with find_package(SZ3) behind GMX_USE_SZ3, and the H5MD prototype vendors it and writes SZ3-compressed trajectories through it. Neither was covered. gromacsConsumerMatrix.sh configures GROMACS at each value of GMX_USE_SZ3 and asserts which library was wired in, including that AUTO falls back when SZ3 declines rather than the configure ending. gromacsH5mdRoundtrip.sh writes a real trajectory through the filter, repacks it, reads it back with a reader that links no GROMACS and no SZ3, and compares every coordinate with the float32 the compressor was given. The workflow runs them weekly and on demand, not on push: it is 1.5 hours of runner time and it depends on gitlab.com and a 70 MB download. Co-Authored-By: Claude Opus 5 <[email protected]>
Co-Authored-By: Claude Opus 5 <[email protected]>
SZGenericCompressor::decompress reads quant_inds_size straight from the decompressed frame and hands it to encoder.decode as targetLength, which allocated out(targetLength) before any check. A 1736-byte file with quant_inds_size patched to 2^31 committed ~8 GB (measured) before the decode walk failed; 2^50 only then hit bad_alloc. Reachable from h5dump, h5repack and h5py, which run the same decompress. Two bounds, because the count has two ceilings: - HuffmanEncoder::decode: below the root every code is at least one bit, so encodedLength * 8 is the most symbols the stream can hold. Reject a larger targetLength (guarding the * 8 against overflow) before sizing out. This covers every decode caller for a multi-leaf tree. - SZGenericCompressor::decompress: a one-leaf tree stores its symbols in zero bits, so decode has no stream length to check against; cap quant_inds_size at conf.num, the element count decData already holds, which no valid frame exceeds. Co-Authored-By: Claude Opus 5 <[email protected]>
usage() ends in exit(0), and the argument path called it for every parse error, so a missing dimension, unknown -M mode, or negative/zero/ absurd dimension printed usage to stdout and reported success. A script checking the exit code saw a bad command succeed. Route argument errors through main()'s existing catch instead: they now print "sz3: <what was wrong>" to stderr and exit 1, matching the fix in 2e908f3 for algorithm exceptions. Dimensions are parsed and validated in one place (parse_dim), rejecting signs, non-digits, trailing junk, zero, and overflow. Unknown -M modes are rejected at parse time rather than silently falling back to ABS. No arguments, -h, --help, -h2, and -v still print help to stdout and exit 0. Co-Authored-By: Claude Opus 5 <[email protected]>
Each of these reported success in a state it was written to catch. cmake.yml: build-windows configured without BUILD_TESTING, so its "Run CTest" step found no tests and ctest exited 0. Set BUILD_TESTING there, and pass --no-tests=error everywhere so an empty ctest can never be green again. packaging.yml: the macOS step grepped otool -L for '@rpath/libhdf5', which matches the library's own LC_ID_DYLIB (@rpath/libhdf5sz3.dylib) on every shared build, so the step reduced to "the dylib carries some LC_RPATH". A dylib whose libhdf5 comes through an @rpath that no LC_RPATH resolves passed it. dlopen answers the question the step is named after. packaging.yml: two export checks globbed Export/*/SZ3Targets.cmake straight into grep. When the glob matches nothing it reaches grep as a literal, grep exits 2, and the `if` reads that as "clean" -- with SZ3::zstd sitting in the file. packaging.yml, hdf5.yml: BUILD_TESTING=ON in two jobs that never run ctest. It also makes FetchContent install googletest into the prefix those jobs go on to inspect as an installed SZ3. gromacs.yml: sed exits 0 when it matches nothing, so an unreadable project() line left SZ3_EXPECT_VERSION empty and the round trip asserted "H5Z-SZ3-", which the vendored 3.1.8 satisfies. Fail instead, in the workflow and in the script. hdf5.yml: restore the line continuations in the _mpi consumer's cmake call. Co-Authored-By: Claude Opus 5 <[email protected]>
RegressionPredictor::load reads coeff_size out of the decompressed frame and hands it to HuffmanEncoder::decode as targetLength. Reachable through ALGO_LORENZO_REG, whose default predictor set is Lorenzo + Regression. The encoder-side ceiling added in 98e05f4 does not cover this call. Below the root every Huffman code is at least one bit, so encodedLength * 8 bounds a multi-leaf stream -- but a single-leaf tree spends zero bits per symbol, and that is exactly the tree a smooth field produces here: once the first block is in, every later block's coefficients predict from the previous block's, so the deltas are all the zero bin. A 143-byte ALGO_LORENZO_REG file with coeff_size patched to 2^28 committed 1.07 GB (measured) and then decompressed without complaint, because nothing downstream reads past the coefficients it needs. The count's own ceiling comes from the block grid, not from the stream: pred_and_quantize_coefficients() appends N linear terms plus one independent term per block it is committed on, and the caller walks prod ceil(dims[i] / blockSize) blocks, so (N + 1) * that product is the most a stream this predictor wrote can carry. A predictor cannot work that out on its own -- load() gets a buffer, not a Config -- so the two decompositions that drive predictors now pass the count down, which is also how the time-series case gets its own (spatial-only) grid rather than an over-estimate. Co-Authored-By: Claude Opus 5 <[email protected]>
ComposedPredictor::load reads selection_size out of the decompressed frame and hands it to HuffmanEncoder::decode, the same shape as the regression count fixed in the previous commit and reached through the same ALGO_LORENZO_REG path. The single-leaf case is if anything more likely here: one predictor usually wins every block, so the selection vector is constant and its tree holds one leaf, which spends zero bits per symbol and leaves the encoder's encodedLength * 8 ceiling nothing to measure. A 143-byte file with selection_size patched to 2^28 committed 1.07 GB (measured). Its ceiling is the block count itself, with no multiplier: precompress_block_ commit() appends exactly one selection per block, and predecompress() consumes one per block, so a stream cannot hold more selections than the caller has blocks to spend them on. The composed predictors run over that same block walk, so they inherit the ceiling unchanged. Co-Authored-By: Claude Opus 5 <[email protected]>
Four steps made more than one claim, so a red X on them named at most one.
The consumer step in packaging.yml built six unrelated consumer shapes; the
hdf5.yml matrix step configured, built, ctested, installed and consumed in
one; the OpenMP step checked an error bound and a 24-cell thread-count matrix;
the bundled-Zstd step mixed "it used the bundled one" with "none of it
escaped". Each is now one step per claim, or one script that names every
assertion it makes and asserts its own total, the way filterAccessModes.sh
already does.
Four scripts, all runnable by hand with no GitHub context:
consumeInstalledSZ3.sh 11 checks, replaces three byte-identical copies of
the consumer CMakeLists/main.cpp heredoc pair
bundledZstdIsolation.sh 9 checks on what a bundled-Zstd build hands out
openmpThreadCounts.sh 25 checks on OpenMP stream framing
checkMaxError.sh 2 checks, replaces four copies of the parse
Two of the assertions they replace could not fail. `for lib in _bprefix/lib/*.so`
passed on an unmatched glob, and the include-directory walks passed on an empty
list; both now count what they inspected. The OpenMP suite gained the check that
makes the other 24 mean anything: a binary built without OpenMP writes the same
stream at every thread count and passed all 24 of them.
The two `for v in DIMS MODE TOL` runtime guards are gone. check_workflow_env.py
already flags every one of those references statically, at the line, for
undeclared and empty-valued keys alike.
Left alone on purpose: gromacs.yml's H5Z_SZ3_initialize step, whose four
assertions are the controls and the result of one before/after experiment;
cmake.yml's workflow-env job, whose failures do not correlate with the build's,
so folding it into build-linux would report them under a name that says
"Build & Test (Linux)"; and the "Create build dir"/`cmake ..` idiom outside the
one Linux job restructured here, because converging it across the pwsh and
msys2 jobs is unverifiable churn.
Co-Authored-By: Claude Opus 5 <[email protected]>
Registering filterAccessModes.sh as a ctest sent it through the MinGW job for the first time, where 14 of its 31 checks went red. Both causes are the suite reading Windows as if it were POSIX; the filter itself came through clean. Four h5dump checks asked for dataset /ds. An argument shaped like an absolute POSIX path is rewritten by the MSYS2 runtime before a native program sees it, so h5dump was handed D:/a/_temp/msys64/ds and reported no such link. HDF5 resolves an unrooted name against the root group, so the name loses its slash and the assertion stays exactly as strong. The other ten follow from one loader error: app.exe could not find libhdf5sz3.dll. Windows has no RPATH, the install puts the DLL in <prefix>/bin and only the import library in <prefix>/lib, and nothing had put that directory on PATH -- so every application shape failed to start, and the two reader checks that read what those shapes write failed with them. The suite now puts <prefix>/bin on PATH, which is what a Windows consumer has to do, and the README says so where it previously claimed an application that links SZ3::hdf5sz3 need set nothing. consumeInstalledSZ3.sh runs a consumer that links the filter the same way and carries the same assumption, though no job runs it on Windows today. No check was loosened and no skip was added: the total stays 31. Three checks that passed in that run passed for the wrong reason -- h5dump-data-noplugin- fails and h5dump-data-with-plugin-clean on the mangled dataset name, and app-init-file-needs-filter on a file the app never got far enough to write. Co-Authored-By: Claude Opus 5 <[email protected]>
decode() opens the code section by reading an 8-byte length off the stream and then uses that, not the targetLength its caller passed, to size its output and to drive its walk. The cap SZGenericCompressor::decompress puts on quant_inds_size therefore never reaches the number that does the work. Reachable from an untrusted file through ALGO_BIOMD, and so through h5dump, h5repack and h5py on any HDF5 file carrying the filter. On the constant path the field is a symbol count, handed straight to std::vector<T>(len, ...). A 29-byte frame with it patched to 2^28 returned 268435456 values where the caller asked for 64, and committed 1029 MB (/usr/bin/time -v, gcc 13.3). Nothing measures it: a one-symbol tree spends no bits on the bins, so there is no stream length to check it against. encode() writes the num_bin it was handed and the compressor records that same count and hands it back as targetLength, so the two must be equal -- which is what the commented-out assert next to it claimed, and the encode path bears out. Everywhere else the field counts bits, `out` is sized targetLength, and the walk runs on the bits; the buffer check ran only after the walk, too late to keep it inside either. ASan on a 47-byte frame: heap-buffer-overflow, WRITE of size 4, 0 bytes after a 256-byte region. On the fixed-length path equality is again the bound, since encode() records mbft * num_bin for the same num_bin; holding len there makes the walk emit floor(len / mbft) == targetLength values and no more. Below the root every code costs at least one bit, so `len` bits cannot carry more than `len` symbols whichever path is taken. That ceiling -- the one HuffmanEncoder::decode already applies -- now gates these walks as well, which is what keeps their work proportional to the stream rather than to whatever count the caller was handed. The Huffman path has no equality of its own -- a code is anywhere from 1 to tree.limit bits -- and bounding the length would not have been enough anyway. Every code below the root costs at least one bit, so a tree whose codes are shorter than the ones that wrote the stream turns the bits the stream really holds into more symbols than the caller asked for. Both walks now stop at the caller's count as well as at the stream's bits, and a stream that cannot fill that count is rejected rather than half-filled. The cached-codebook walk was already driven by the count, but read ahead of it without limit; past the last code byte it now shifts in zeros instead of touching memory, and afterwards checks that no code actually consumed them. Co-Authored-By: Claude Opus 5 <[email protected]>
Sweeping the rest of the file for the same shape -- a count read off the stream that sizes or drives something -- turns up three more in loadAsDFSOrder, two of them heap corruption. All are reached the same way as the length field fixed in the previous commit: ALGO_BIOMD on an untrusted file. tree.n sizes the node pool, ht.reserve(2 * n), but nothing ties the walk that fills it to n: the walk pushes one node per DFS bit and runs until its stack empties. A full binary tree with n leaves has exactly 2n - 1 nodes, so a well-formed stream stays under the reserve and the raw Node* held in the stack and in the children already linked stay valid -- which is the only reason the walk works at all. A 533-byte tree section declaring n = 2 and then an all-zero bitstream pushes a node per bit and never pops, and the vector reallocates out from under those pointers: ASan reports heap-use-after-free, READ of size 8, and the walk writes through them too. The walk now stops at 2n - 1 nodes, which is both the shape of the tree n describes and what the reserve already assumed. tree.maxval sizes veclen and veccode, which dfs_vec then indexes by the leaf values -- read as tree.mbft raw bits, with nothing tying them to maxval. A 25-byte tree section declaring maxval = 2 and leaf values 200 and 201: ASan reports heap-buffer-overflow, WRITE of size 1, 198 bytes after a 2-byte region. addElementInVector() only ever enters values from [0, maxval), so requiring that of a leaf turns away no tree the encoder wrote. constructHuffmanTree() collapses a one-leaf tree to maxval == 1, and encode() keys off that to spend no bits on the bins, so n == 1 and maxval == 1 are settled together before either is written. A stream that pairs n == 1 with any other maxval sends decode down the general walk into a root with only one child, where the first 1 bit steps onto a null pointer: SEGV on a 30-byte frame. Two smaller ones while here. mbft is a leaf's width in bits and is rebuilt by shifting into a T, so a width the type cannot hold is undefined behaviour; preprocess_encode() never raises it past what a T holds. And readBit took its bit index as an int while the caller counts bits over a whole tree section, which for a large enough section truncates to a negative offset. Left alone, and worth their own pass: n still reserves two Nodes per declared leaf before the walk can disagree, so a stream can still ask for ~384 bytes of node pool per byte of tree section; maxval under the 2^28 ceiling can still size veccode and veclen at about 1.3 GB between them; and dfs_vec and dfs_mp recurse once per level with a uchar depth, so a tree deeper than 255 levels wraps it before any of the above notices. All three are bounded by the stream's own size, which is what the earlier commits on this branch settled for. Co-Authored-By: Claude Opus 5 <[email protected]>
XtcBasedEncoder is libxdrf.cpp from GROMACS turned into a header, and it carries that code's habits. Unlike the rest of this family, the three defects below fire on valid data rather than on crafted input: an ASan+UBSan sz3 run over ALGO_BIOMDXTC on peg-1chain (50 frames x 13406 atoms, ABS 1e-3, gcc 13.3) reports sixteen, and ctest's SZ3_BioMD.InputsTooShortForOneTriplet aborts under ASan. The header is seven 4-byte ints and an 8-byte count, written at whatever byte of the compressor's buffer the decomposition and the encoder's own save() left off at. That byte is odd for every trajectory tried, and both sides reached it by pointing an unsigned int* or a uint64_t* at it: undefined, and exactly the shape that survives until a vectorising -O3, LTO, or a target that faults instead of tolerating. Those sixteen reports are all of this one kind -- three stores at the minInt loop, three at maxInt, one for smallIdx, one for the count, and their eight mirrors in decode. memcpy moves the same bytes with no alignment demand, so the format is untouched; the int*/uint64_t* cursors are gone and the byte cursor that was already there carries the whole header. sizeInt is maxInt - minInt + 1 on both sides. An input with no complete triplet never enters the scan that fills them, so they stay at the INT_MAX and INT_MIN seeds -- and the range guard above passes, because the float difference of the seeds is negative rather than large. INT_MIN - INT_MAX then overflows a signed int, which UBSan reports six times on a one-float input, three in encode and three in decode reading the seeds back off the stream. The destination is unsigned, so doing the arithmetic there is the same value by a defined route: 2 for the seeds, and the same bits as the wrap for every other input. The run loop squares three differences and compares against smaller * smaller. The differences are bounded by smallNum, but smallNum reaches 2^23 at the top of magicInts, so the squares overflow. Reached from a plain float input: 16 atoms 16000.0 apart in every axis quantise ~8e6 bins apart, which puts every neighbour distance past the end of the table, and UBSan reports -8000000 * -8000000 and 6658042 * 6658042. Squaring in unsigned and reading the sum back as int is the wrap the signed version was relying on, so the run is cut in the same places. Last, the genuine memory error. magicInts holds 73 entries and the encoder writes LASTIDX -- 73 -- whenever no entry reaches minDiff, which is every input with fewer than two triplets. The encoder clamps its own lookups and guards the rest behind isSmaller != 0; the decoder's sizeSmall assignment sits outside that guard and subscripted the table at 73. ASan: global-buffer-overflow, and UBSan: index 73 out of bounds for type 'int [73]'. Clamping cannot change a well-formed decode, because a stream that names LASTIDX carries no run and so never reads sizeSmall. The two lookups either side of it take the same clamp, which also holds the index in range when isSmaller walks it there over a long stream. Nothing here may change what the encoder emits, and nothing does. ALGO_BIOMD and ALGO_BIOMDXTC over peg-1chain, ifabp-water and adk-equilibrium at ABS 1e-2, 1e-3 and 1e-4 give the same 18 sha256 sums, compression ratios and maximum errors before and after, as does the 16-atom input that trips the square. Co-Authored-By: Claude Opus 5 <[email protected]>
decode() opens with a 36-byte header -- six coordinate extremes, smallIdx, and an 8-byte count of the packed bytes that follow -- and then copies that many bytes out of the compressed buffer into one of its own. The count was measured against the destination only, and the destination is sized from the caller's targetLength: malloc(targetLength * 1.2 * sizeof(int)), about 4.8 bytes per element. The packed bytes run about one per element, so a frame could claim roughly four times what it carried and the memcpy read the difference off the end of the buffer the lossless layer allocated. Reachable from an untrusted file through ALGO_BIOMD and ALGO_BIOMDXTC, and so through h5dump, h5repack and h5py on any HDF5 file carrying the filter. A 5009-byte ALGO_BIOMDXTC frame over 3000 floats, with that one field patched from 4874 to 14400 -- the largest the destination check accepts -- decompressed without complaint. Under ASan: heap-buffer-overflow, READ of size 14400, 0 bytes after the 4959-byte region Lossless_bypass::decompress allocated. The bound the check was missing is the source: what is left of the compressed buffer once the header is off it. That cannot turn away a stream this encoder wrote, because encode() puts the packed bytes immediately after the header and is the last thing SZGenericCompressor::compress writes into the frame -- so at this point remaining_length is exactly the header plus the count, and the new check is an equality for every well-formed stream. The header is now checked to be present before any of it is read, which is also what makes that subtraction safe. The cursor accounting at the other end of the function belongs to the same statement and was likewise not being made. Every read went through a local pointer, so `bytes` never moved, the consumed count came out zero, and remaining_length was never charged -- leaving the check just above the return with nothing to check. encode() advances its cursor and the other encoders' decode() do too. Nothing depended on the old behaviour: SZGenericCompressor::decompress is the only caller that can reach this decode -- SZAlgoBioMD.hpp is the one wiring, and no test constructs the encoder directly -- and it reads neither the cursor nor the count again, going straight on to postprocess_decode(), releasing the buffer, and decomposition.decompress(). Co-Authored-By: Claude Opus 5 <[email protected]>
…drives A round of decode() ends with a flag bit and, when it is set, five more that carry the run: how many further triplets this round decodes in one go. Those five bits went straight into the loop that writes them. Two things then went wrong at once, and each needs its own bound. The value itself. Five raw bits give up to 31; the decoder takes the remainder mod three off as isSmaller, leaving a multiple of three up to 30, ten triplets. The encoder's run loop tests `run < CHAR_BIT * 3` at the top and adds three per turn, so 24 is the largest run it can hold, and it writes run + isSmaller + 1, at most 26. 27 through 31 are values no stream it wrote can carry. And the loop. Bounding the count is not enough, which is the lesson from HuffmanEncoderV2: the `while (i < numTriplets)` test is only at the top, so a run of any length starting on the last triplet still writes past the end. Both destinations are sized from the frame -- quantData holds targetLength bins and the index buffer targetLength ints -- and the run writes three ints into each per triplet, plus three more on the first. The bound that holds is the frame's own remaining triplets: the encoder opens a run only when another triplet follows, and its run loop re-tests `i < numTriplets` before taking each one, so i never passes numTriplets and a stream it wrote never names more triplets than the frame has room for. run carries over to the next round when the flag bit is 0, so the test sits where the run is used rather than only where it is read. A 30-bin ALGO_BIOMDXTC frame whose last of ten triplets claims a run of 30, through SZ_decompress under ASan: heap-buffer-overflow, WRITE of size 4, 8 bytes after the 120-byte index buffer, then five more -- two writes and three reads past the same buffer, and a WRITE 0 bytes after the 120-byte quantData -- with halt_on_error=0 letting it run on. About thirty ints past the end of both, as the shape predicts. Co-Authored-By: Claude Opus 5 <[email protected]>
smallIdx names an entry of magicInts, and it is also the bit width receiveints reads a run body at. It arrives on the stream, and from then on every round can move it one step in either direction, with nothing holding it: 256 steps up and receiveints, which splits its bit count into bytes on a local int[32], writes off the end of that array. The previous pass clamped the three magicInts lookups but deliberately left the bit count alone, because clamping it would have changed what a well-formed stream decodes to. Rejecting instead does not. The bound is the table's own extent, [FIRSTIDX, LASTIDX], and it is the encoder that fixes it. The seed is FIRSTIDX, raised only by a scan that stops at LASTIDX, so the field on the stream is in range to begin with. For the drift, the encoder computes maxIdx = min(LASTIDX - 1, smallIdx + CHAR_BIT) and minIdx = maxIdx - CHAR_BIT once, before its loop, and never recomputes them; inside the loop it takes a step up only while smallIdx < maxIdx and a step down only while smallIdx > minIdx. So after the first round smallIdx lies in [minIdx, maxIdx], and that window is inside the table: maxIdx is at most LASTIDX - 1 by construction, and minIdx is either the seed, which is at least FIRSTIDX, or LASTIDX - 1 - CHAR_BIT, which is 64. The only value outside the window the encoder ever holds is the LASTIDX it may have been seeded with, which the range already admits. A stream this encoder wrote therefore never leaves [FIRSTIDX, LASTIDX], and at LASTIDX receiveints fills ten of its thirty-two entries. Tightening the initial check from [0, LASTIDX] to [FIRSTIDX, LASTIDX] costs nothing for the same reason: the scan that produces the field starts at FIRSTIDX. A 900-bin ALGO_BIOMDXTC frame whose rounds each ask for one step up walks smallIdx from 9 to 269 and then decodes one run body. Under UBSan: index 32 out of bounds for type 'int [32]', then 33; under ASan: stack-buffer-overflow, WRITE of size 4 at offset 160 of a [32, 160) frame object, and three more past it. Co-Authored-By: Claude Opus 5 <[email protected]>
receivebits() is the whole of the decoder's contact with the packed bytes, and it walked them as buffer->data[buffer->index++] with nothing testing that cursor against the allocation. How far it walks is decided by the stream: the coordinate extremes in the header set the width of every base triplet, and smallIdx sets the width of every run body. Reachable from an untrusted file through ALGO_BIOMDXTC, and so through h5dump, h5repack and h5py on any HDF5 file carrying the filter. Three bins is the tightest frame there is. buffer.data is malloc(targetLength * 1.2 * sizeof(int)), which truncates to twelve bytes, and a header naming minInt 0 and maxInt 2147483646 on each axis makes sizeInt 2^31 - 1, so sizeofint returns 32 and the three base coordinates take all twelve. The flag bit that follows then reads data[12]. Through SZ_decompress under ASan: heap-buffer-overflow, READ of size 1, 0 bytes after the 12-byte region. The bound is the allocation and not the packed byte count beside it in the header, because the allocation is what the cursor runs off and is a fact of this process rather than a number the stream supplied -- and it is the wider of the two, the count having already been measured against it. The buffer now carries that allocation in bits, less every bit read so far, so the test is one comparison against the width being asked for, and it refuses exactly the reads that would leave the buffer and nothing else. A well-formed stream is never one of those: the encoder emitted these bits through sendbits into a buffer the same targetLength * 1.2 * sizeof(int) sized, sendbits and receivebits carry lastbits identically so the decode takes back byte for byte what the encode put in, and decode() has already held the packed count inside the allocation. An instrumented decode puts numbers on it. The cursor stops at exactly the packed byte count on every frame of peg-1chain, ifabp-water and adk-equilibrium, between 22 and 43 per cent of what was allocated; and on the tightest well-formed shape that exists it stops at 12 of 12 -- three bins whose base triplet is the widest sizeofint can describe without overflowing, 3 * 30 bits, plus the flag and the run field, 96 bits against 12 bytes. That one is still accepted, with nothing to spare, which is what says the bound is not a byte too tight. The test is in receivebits and not at its call sites because receiveints calls it in a loop, so a test outside would let the round that runs off the end do so before the next test was reached. It costs a compare, a subtract and a branch that is never taken, per call rather than per bit: adk- equilibrium at ABS 1e-4, 82 MB of packed bytes over 42 M elements, decodes in 0.400 s where it decoded in 0.390, 2.8 per cent, and 1.9 per cent of the wall clock end to end. Counting bits rather than bytes is what keeps it that low -- the same test written as the bytes this call will take, which has to work out whether a partial tail needs another one, cost 6.7 per cent -- and most of what is left is the extra word in DataBuffer rather than the test: carrying the word and not testing it costs 1.8. Two operations on the path to this were already undefined, bounding the cursor leaves both reachable, since both run before the first byte is read, and so both are fixed here. sizeofint doubles num until it passes size, so a size of 2^30 or more takes it past INT_MAX; UBSan on the frame above reports a left shift of negative value -2147483648. Doubling in unsigned returns the same width for every size, and the encoder's range guard holds every size it asks about below 2^30, so no stream it wrote came near. receivebits then formed its mask as 1 << 32 -- shift exponent 32 is too large for 32-bit type 'int'. Building the mask in unsigned gives the identical mask at every width below 32, which is where that same guard holds a well-formed stream; comparing the width as unsigned lets the one test cover a width that arrives negative as well. Nothing here may change what the encoder emits or what the decoder returns, and nothing does. ALGO_BIOMD and ALGO_BIOMDXTC over peg-1chain, ifabp-water and adk-equilibrium at ABS 1e-2, 1e-3 and 1e-4 give the same eighteen pairs of sha256 sums, compressed and decompressed, as before -- and as the two passes before this one. 240 further round trips, over twenty frame sizes from one triplet up, four value distributions and three error bounds, are byte-identical on both sides too. Co-Authored-By: Claude Opus 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An SZ3 that has been installed cannot be used through
find_package(SZ3), and a source buildcannot be configured without network access. Nothing in this repository ever tried either: every
test reaches SZ3 through
add_subdirectoryor FetchContent, and neither readsSZ3Config.cmake.So these have been shipping since at least v3.3.2, and the GROMACS merge request that links an
installed SZ3 (gromacs!5560) has been
stuck in Draft since November waiting on us — GROMACS 2027 replaces XTC with H5MD and reaches lossy
compression through our filter. Spack builds SZ3 the same way, so every Spack consumer met the same
wall.
90 files, +47,069 / −241. 64 of those files and +46,065 of those lines are the vendored Zstd
source; our own change is 26 files, +1,004 / −241. The three CMake files it touches end at 285
lines against master's 246.
find_package(SZ3)could not produce a working targetSZ3Targets.cmakenamedPkgConfig::ZSTD, an imported target that exists only inside the buildthat created it. Every consumer of an SZ3 built against a system Zstd — the default on Linux and
macOS, and what Spack builds — died at generate time with
The link interface of target "SZ3::SZ3" contains: PkgConfig::ZSTD but the target was not found.target_link_libraries(x PRIVATE hdf5sz3)— what GROMACS writes, and what works against an in-tree SZ3 — became
-lhdf5sz3.A config file must not end its caller's configure.
find_package(HDF5 ... REQUIRED),find_package(MPI COMPONENTS CXX REQUIRED)andfind_package(GSL REQUIRED)all calledmessage(FATAL_ERROR)from inside aQUIEToptional probe, taking downGMX_USE_SZ3=AUTO—GROMACS's default, whose contract is to fall back to its own copy. The MPI search should not have
existed:
nm -u libhdf5sz3.sofinds no MPI symbol, and a parallel HDF5 brings its own MPI throughits package config. Resolving MPI a second time only added an independently chosen implementation,
which put
libmpi.so.12andlibmpi.so.40in one binary(gromacs#5505).
The install leaked the build machine into the consumer
${HDF5_INCLUDE_DIRS}wasPUBLICwith no$<BUILD_INTERFACE:>guard, so a consumer who foundHDF5 1.10.6 compiled against the build machine's 2.2.0 headers.
libhdf5sz3.sohad no RUNPATH —lddreportedlibhdf5.so.320 => not found.find_dependency(OpenMP)withoutCOMPONENTS CXXtold a C-only consumerSZ3_FOUND=1, whichthen failed at link. An HDF5 filter is exactly what a C program links.
A crash in released code
set_SZ3_conf_to_H5chose betweenH5Pset_filterandH5Pmodify_filteronH5Zfilter_avail(),which answers whether the filter is registered with the library, not whether it is on this
property list. With
HDF5_PLUGIN_PATHset it always answers yes, so a freshly created propertylist went to
H5Pmodify_filterand faulted inside HDF5:Deterministic, and reproduced on HDF5 1.10.6, 1.14.6 and 2.2.0. It was introduced in v3.3.1:
v3.3.0 branched on
H5Pget_filter_by_id(propertyList, ...), which asks the right question. v3.3.1,v3.3.2 and this branch's parent all crash; v3.3.0 does not.
get_SZ3_conf_from_H5had the same predicate and also ignoredH5Pget_filter_by_id's return, soreading from a list with no SZ3 filter silently overwrote the caller's
Configwith zeros whilereporting success.
Zstd is now a private dependency, vendored rather than downloaded
The bundled Zstd was fetched at configure time from a GitHub release URL, with no
URL_HASH. Thatbuild cannot be configured offline at all — the failure is
Build step for zstdfetched failed—which rules out air-gapped clusters, most distribution build systems, and any reproducible build.
Zstd 1.5.6 is now vendored in
tools/zstd/lib(lib/common,lib/compress,lib/decompress;deprecated/,legacy/anddictBuilder/dropped, 39 compiled files down to 25, −29.8% on thattarget's build time). It stays what it already was — a private static
libsz3_zstdwith hiddensymbols — and it no longer installs a header.
Lossless_zstd.hppdeclares the four Simple-API functions it calls instead of includingzstd.h,so nothing SZ3 installs answers to
zstd.hand a consumer needs no Zstd include directory. Actest compiles those declarations against the real header in both include orders and fails if they
drift.
Resolution stays pkg-config, as on master, falling back to the vendored copy;
SZ3_USE_BUNDLED_ZSTD=ONforces it. A consumer of a system-Zstd install resolves it with onefind_library, so nothing about the build machine's Zstd is written into the installed package.Four cases that failed before now work:
an offline build with no Zstd present, a consumer against a Homebrew prefix, a consumer after the
Zstd prefix used at SZ3 build time was deleted, and an hdf5plugin-style direct compile with no Zstd
include directory on the command line.
1.5.6 decompresses 2–22% faster than the 1.4.5 it replaces on the payloads measured. The 32-bit
ZSTD_CURRENT_MAXdefect (facebook/zstd#4129) cannot be reached from SZ3, which holds noZSTD_CCtxacross calls; the reasoning and the condition that would expire it are recorded intools/zstd/CMakeLists.txt.Smaller fixes
Wavelet.hpp, which has no includer anywhere in the tree;PreprocessorInterface's only virtual is commented out, so it overrode nothing. It is also not around trip — it pads to the next power of two, transforms all of it, and copies back only the
original length. Keeping the header without the dependency would leave an installed public header
that
#includes<gsl/gsl_wavelet.h>and cannot compile, so both go.H5Z_SZ3_initialize/_finalize), a configurable plugin installdirectory, and a filter version string that
h5dump -pHandh5ls -vnow show.Config::loadrejects a negative dimension count.char's signedness is implementationdefined; where it is signed, a
cd_valuesbyte of0xF3arrives as −13 and passedN > 4. Thelater guards did not stop it either: with
bitWidth0 thedim_bytescomputation wraps to 0, soa negative
Nreachedbytes2vector, which asked for 1.8e19 elements and threw a barestd::length_errorwith no diagnosis.Config::loadis no longer the first thing a foreign file meets. On read, the payload's magicand data version are checked before
cd_valuesis parsed under the layout this build assumes. Av3.2.0, v3.2.1 or v3.3.0 file was previously refused by
Config::load's own guards, with"invalid number of dimensions" — which reads as a corrupt file rather than a version mismatch. It
now names both versions. The magic is a precondition rather than a requirement: a chunk the
conf.num < 20path stores raw has no header, andconf.numis not known untilcd_valueshasbeen parsed, so files older than v3.2.0 carry no magic and are still refused by
Config::load.sz.hpp,SZImplOMP.hppandIterator.hppeachemitted the same error two or three times, once of them to stdout. Inside the HDF5 filter that
lands in whatever the user redirected, so
h5dump -d /ds f.h5 > out.txtgot SZ3's diagnosticwritten into the data file. The exception carries the whole message; the filter turns
what()into an entry in HDF5's error stack.
dsz3FromHDF5checks the read before using it.H5Dread's status was stored and ignored: onfailure the tool printed twenty uninitialized floats as "reconstructed data", wrote them into a
new HDF5 file, and reached a trailing check that reported the failure and called
exit(0). Acaller saw a complete output file and a success status. It now returns 1 and writes nothing.
SZ3_DATA_VERSION. v3.3.0 and v3.3.1shipped different layouts under the same declared 3.3.0 and nothing caught it, because the only
digest CI had asks whether Linux and macOS agree with each other and a format change moves both
together.
tools/test/data_format_digest.txtpins the bytes to the declared version.Not changed
The compressed format.
SZ3_DATA_VERSIONis untouched, and all seven algorithms across 1D/2D/3Dcross-decompress bit-identically between v3.3.2 and this branch in both directions.
Verification
Reviewed over six rounds by an agent briefed as a GROMACS maintainer, which found eleven defects in
the first round, two regressions introduced by the fixes in the third, and two more when the patch
was cut down in the fifth.
~/gmxunmodified,EXTERNAL/AUTO/INTERNAL/OFF× serial / MPI, plusEXTERNAL-serial against the vendored-Zstd install: nine configurations, 526/526 h5md testseach.
AUTOagainst a declining SZ3 falls back to internal, as it must.masterand passes here.(4187×3341×3, 53.4 MiB), direct API and HDF5 filter, max error inside the bound, filter output
bit-identical to the API, on both Zstd paths.
tools/H5Z-SZ3/test/filter_access_modes.shasserts 31 properties acrossh5repack,h5dump,h5lsand four link/registration shapes an application can take — includingthat
h5repackwith the plugin unreachable exits 0 and quietly writes an unfiltered copy. CIruns it.
both module and config mode. Shapes: static, shared, relocated prefix,
add_subdirectory.CMake 3.28 and 4.4 in both roles. macOS on a real host.
(g++ 13.3, Linux).
against it, link an executable and run it. Against the pre-fix tree, its assertions fire.
Windows has no development host here, so every change was read against MSVC and MinGW
semantics rather than run locally. CI covers it:
Build & Test (Windows)andBuild & Test (Windows MinGW)both pass, and MSVC defaults to the vendored Zstd, so that is thepath the MSVC job exercises.
🤖 Generated with Claude Code