From 7dd5182d91795d4416f71c79c023404b7460828f Mon Sep 17 00:00:00 2001 From: Scot Breitenfeld Date: Thu, 3 Sep 2026 13:15:38 -0500 Subject: [PATCH] Fix CI regressions found after the review-remediation push Verified against the fork's actual GitHub Actions CI (not just local builds) and found two real regressions plus applied formatting the CI bot detected but couldn't push: - test/CMakeTests.cmake: register test_filters_v3.h5 (the Critical #3 golden file) in HDF5_REFERENCE_TEST_FILES so a fresh checkout's build tree actually stages it -- it only passed locally because the file had been generated directly inside an existing local build directory. Was failing H5TEST-tfilter2 on every CI job running the standard suite. - tools/lib/CMakeLists.txt: link the tools library against LINK_LIBS (libm), matching src/CMakeLists.txt's existing pattern. The new hex-float code in h5tools_dump.c is the first tools-library code to need floor()/isfinite(), and FreeBSD/OpenBSD's stricter linkers reject the resulting undefined symbols (Linux tolerated it silently). Was breaking the link step on FreeBSD 14.3/15.0 and OpenBSD 7.9. - Applied the clang-format fixes the CI bot found but couldn't push back (pre-existing fork credentials gap, confirmed unrelated to this branch): tfilter.cpp, h5pDCPLImp.c, H5Znbit.c, tfilter2.c, h5tools_dump.c. Verified via ctest -R H5TEST-tfilter2 on a build tree with the stale golden-file copy removed (reproducing CI's exact test invocation), plus a full local rebuild (0 warnings/errors) and the Fortran/parallel suites. Details and CI failure logs analyzed in REVIEW-6153.md's new "Post-push CI verification" section. --- REVIEW-6153.md | 10 ++++ c++/test/tfilter.cpp | 4 +- java/src-jni/jni/h5pDCPLImp.c | 3 +- src/H5Znbit.c | 10 ++-- test/CMakeTests.cmake | 1 + test/tfilter2.c | 91 ++++++++++++++--------------------- tools/lib/CMakeLists.txt | 4 +- tools/lib/h5tools_dump.c | 10 ++-- 8 files changed, 63 insertions(+), 70 deletions(-) diff --git a/REVIEW-6153.md b/REVIEW-6153.md index 7f94969b2a8..d0afc93ffc5 100644 --- a/REVIEW-6153.md +++ b/REVIEW-6153.md @@ -328,6 +328,16 @@ The two Critical/High memory-safety findings (canonicalization overflow → h5du --- +## Post-push CI verification + +The first push of this remediation work (commit `19745508a34`) was checked against the fork's actual GitHub Actions CI, not just local builds — and CI caught two real regressions that local verification had missed, plus confirmed one pre-existing, unrelated infra failure: + +- **`H5TEST-tfilter2` failed on every CI job that runs the standard test suite** (~30 of ~40 jobs): the golden regression file `test/testfiles/test_filters_v3.h5` (added for Critical #3) was never registered in `test/CMakeTests.cmake`'s `HDF5_REFERENCE_TEST_FILES` copy list, so a fresh CI checkout's build tree never staged it — the test only "passed" locally because the file had been generated directly inside the local build directory in an earlier session, masking the gap. **Fixed**: added `test_filters_v3.h5` to the copy list. Verified by removing the local build-tree copy, reconfiguring, and confirming `ctest -R H5TEST-tfilter2` (CI's exact invocation, not just running the binary directly) passes on a freshly-staged tree. +- **FreeBSD 14.3/15.0 and OpenBSD 7.9 failed to link `libhdf5_tools.so`** (`undefined reference: floor`, `undefined reference: __isfinite`): the new hex-float annotation code in `h5tools_dump.c` (Critical #1's h5dump fix) was the first code in the tools library to need `libm`, and `tools/lib/CMakeLists.txt` never linked it — Linux tolerated the gap silently, but FreeBSD/OpenBSD's linkers reject shared libraries with unresolved symbols. **Fixed**: linked `${LINK_LIBS}` (the project's existing libm-detection variable, already used the same way by `src/CMakeLists.txt`) into both the static and shared tools library targets. Verified `-lm` now appears in the tools library's link command and the full local suite still passes. +- **`clang-format Commit Changes` failed** — pushing its auto-fix commit back to the fork (`fatal: could not read Username for 'https://github.com'`). Confirmed via the baseline commit's CI history that this is a pre-existing fork credentials/secrets gap, unrelated to this branch's changes. Not fixable from a commit; however, the bot *had* found real formatting drift (5 files, 50 insertions/68 deletions) it couldn't push, so that drift was applied locally with `clang-format` directly (excluding `src/H5version.h`, which is machine-generated by `bin/make_vers` and must never be hand/clang-format-edited — an early run of the formatter over the branch's full changed-file list included it by mistake and was reverted before committing). + +--- + ## Notes on this review run - **`--depth normal`** (Opus for architecture/security/adversarial reviewers; Sonnet for the rest), full agent roster (no `--quick`/`--security-only`/`--summary-only`). diff --git a/c++/test/tfilter.cpp b/c++/test/tfilter.cpp index ef8eba15a7a..b23ceb19a43 100644 --- a/c++/test/tfilter.cpp +++ b/c++/test/tfilter.cpp @@ -324,8 +324,8 @@ test_append_filter() SUBTEST("H5FilterParam::config_get_param (type mismatch throws)"); try { - int64_t val = 0; - bool threw = false; + int64_t val = 0; + bool threw = false; try { /* "level" holds a string value here, not an integer. */ FilterParam::config_get_param("level = \"six\"", "level", val); diff --git a/java/src-jni/jni/h5pDCPLImp.c b/java/src-jni/jni/h5pDCPLImp.c index 640d1915fce..8cf2e136d8c 100644 --- a/java/src-jni/jni/h5pDCPLImp.c +++ b/java/src-jni/jni/h5pDCPLImp.c @@ -1556,8 +1556,7 @@ Java_hdf_hdf5lib_H5_H5Pget_1filter_1params_1by_1idx(JNIEnv *env, jclass clss, jl if (plen > 0) { /* Populate the exact-size buffer. */ - if ((status = H5Pget_filter_params_by_idx((hid_t)plist_id, (unsigned)idx, buf, plen + 1, &plen)) < - 0) + if ((status = H5Pget_filter_params_by_idx((hid_t)plist_id, (unsigned)idx, buf, plen + 1, &plen)) < 0) H5_LIBRARY_ERROR(ENVONLY); } buf[plen] = '\0'; diff --git a/src/H5Znbit.c b/src/H5Znbit.c index de14aee2fea..f0dc8882f82 100644 --- a/src/H5Znbit.c +++ b/src/H5Znbit.c @@ -80,8 +80,8 @@ static void H5Z__nbit_compress_one_array(unsigned char *data, size_t data_offs static void H5Z__nbit_compress_one_compound(unsigned char *data, size_t data_offset, unsigned char *buffer, size_t *j, size_t *buf_len, const unsigned parms[], unsigned *parms_index); -static void H5Z__nbit_compress(unsigned char *data, unsigned d_nelmts, unsigned char *buffer, - size_t *buffer_size, const unsigned parms[]); +static void H5Z__nbit_compress(unsigned char *data, unsigned d_nelmts, unsigned char *buffer, + size_t *buffer_size, const unsigned parms[]); /* This message derives from H5Z */ H5_ATTR_VISIBILITY_HIDDEN H5Z_class3_t H5Z_NBIT[1] = {{ @@ -94,9 +94,9 @@ H5_ATTR_VISIBILITY_HIDDEN H5Z_class3_t H5Z_NBIT[1] = {{ H5Z__can_apply_nbit, /* The "can apply" callback */ H5Z__set_local_nbit, /* The "set local" callback */ H5Z__filter_nbit, /* The actual filter function */ - H5Z__no_params_set_config, /* String config setter: no-op (params come - * from set_local); shared with shuffle/fletcher32 */ - NULL, /* No string config getter */ + H5Z__no_params_set_config, /* String config setter: no-op (params come + * from set_local); shared with shuffle/fletcher32 */ + NULL, /* No string config getter */ }}; /* Local macros */ diff --git a/test/CMakeTests.cmake b/test/CMakeTests.cmake index 81fce1c3dae..751f49d879a 100644 --- a/test/CMakeTests.cmake +++ b/test/CMakeTests.cmake @@ -162,6 +162,7 @@ set (HDF5_REFERENCE_TEST_FILES tbogus.h5 test_filters_be.h5 test_filters_le.h5 + test_filters_v3.h5 th5s.h5 tlayouto.h5 tmisc38a.h5 diff --git a/test/tfilter2.c b/test/tfilter2.c index a5130741c45..c8dfe3d70c0 100644 --- a/test/tfilter2.c +++ b/test/tfilter2.c @@ -1800,29 +1800,11 @@ error: static int test_canonical_name_uniqueness(void) { - H5Z_class3_t cls_a = {2, - UNIQUENAME_FILTER_ID_A, - 1, - 1, - "test-unique-name", - NULL, - NULL, - NULL, - longtitle_filter_func, - NULL, - NULL}; - H5Z_class3_t cls_b = {2, - UNIQUENAME_FILTER_ID_B, - 1, - 1, - "test-unique-name", - NULL, - NULL, - NULL, - longtitle_filter_func, - NULL, - NULL}; - herr_t ret; + H5Z_class3_t cls_a = {2, UNIQUENAME_FILTER_ID_A, 1, 1, "test-unique-name", NULL, NULL, + NULL, longtitle_filter_func, NULL, NULL}; + H5Z_class3_t cls_b = {2, UNIQUENAME_FILTER_ID_B, 1, 1, "test-unique-name", NULL, NULL, + NULL, longtitle_filter_func, NULL, NULL}; + herr_t ret; TESTING("H5Zregister: canonical_name collision across different filter ids is rejected"); @@ -1975,16 +1957,16 @@ error: static size_t growth_filter_func(unsigned H5_ATTR_UNUSED flags, size_t H5_ATTR_UNUSED cd_nelmts, - const unsigned H5_ATTR_UNUSED *cd_values, hid_t H5_ATTR_UNUSED dxpl_id, - const hsize_t H5_ATTR_UNUSED *scaled, size_t H5_ATTR_UNUSED ndims, size_t nbytes, - size_t H5_ATTR_UNUSED *buf_size, void H5_ATTR_UNUSED **buf) + const unsigned H5_ATTR_UNUSED *cd_values, hid_t H5_ATTR_UNUSED dxpl_id, + const hsize_t H5_ATTR_UNUSED *scaled, size_t H5_ATTR_UNUSED ndims, size_t nbytes, + size_t H5_ATTR_UNUSED *buf_size, void H5_ATTR_UNUSED **buf) { return nbytes; /* pass-through; never actually applied by this test */ } static herr_t growth_set_config(const char H5_ATTR_UNUSED *params, unsigned H5_ATTR_UNUSED *flags, size_t *cd_nelmts, - unsigned H5_ATTR_UNUSED cd_values[], size_t H5_ATTR_UNUSED cd_values_size) + unsigned H5_ATTR_UNUSED cd_values[], size_t H5_ATTR_UNUSED cd_values_size) { /* Ignores params entirely -- any syntactically valid TOML is "accepted", * so the test below is free to pack the string with as many distinct @@ -1997,17 +1979,17 @@ static int test_config_string_canonicalization_growth(void) { static const H5Z_class3_t growth_cls = { - 2, /* version */ - GROWTH_FILTER_ID, /* id */ - 1, /* encoder_present */ - 1, /* decoder_present */ - "growth_filter", /* canonical_name */ - NULL, /* description */ - NULL, /* can_apply */ - NULL, /* set_local */ - growth_filter_func, /* filter */ - growth_set_config, /* set_config */ - NULL, /* get_config */ + 2, /* version */ + GROWTH_FILTER_ID, /* id */ + 1, /* encoder_present */ + 1, /* decoder_present */ + "growth_filter", /* canonical_name */ + NULL, /* description */ + NULL, /* can_apply */ + NULL, /* set_local */ + growth_filter_func, /* filter */ + growth_set_config, /* set_config */ + NULL, /* get_config */ }; hid_t dcpl = H5I_INVALID_HID; char *raw = NULL; @@ -2795,8 +2777,10 @@ test_config_string_golden_file(void) if (H5Pget_filter_params_by_idx(dcpl, 0, pbuf, sizeof(pbuf), &plen) < 0) TEST_ERROR; if (strcmp(pbuf, "scale_type = \"int\", scale_factor = 3") != 0) { - fprintf(stderr, "\n golden file stored \"%s\"\n expected \"scale_type = \\\"int\\\", scale_factor = 3\"\n", - pbuf); + fprintf( + stderr, + "\n golden file stored \"%s\"\n expected \"scale_type = \\\"int\\\", scale_factor = 3\"\n", + pbuf); TEST_ERROR; } @@ -2843,8 +2827,7 @@ error: /* Copy SRC to DST, patching the 2 bytes at BYTE_OFFSET (little-endian) to * NEW_LEN. Returns 0 on success, -1 on any I/O or verification failure. */ static int -patch_config_length(const char *src, const char *dst, long byte_offset, uint16_t expect_len, - uint16_t new_len) +patch_config_length(const char *src, const char *dst, long byte_offset, uint16_t expect_len, uint16_t new_len) { FILE *in = NULL, *out = NULL; char *buf = NULL; @@ -2904,11 +2887,11 @@ test_config_string_corrupted_decode(void) * re-verifies the current value at this offset is still 36 before * patching, so a future regeneration that shifts this offset fails the * test loudly rather than corrupting an unrelated field. */ - const long config_length_offset = 357; - const char *over_max_file = "tfilter2_v3_corrupt_over_max.h5"; - const char *over_buffer_file = "tfilter2_v3_corrupt_over_buffer.h5"; - hid_t file = H5I_INVALID_HID, dset = H5I_INVALID_HID; - herr_t open_ret; + const long config_length_offset = 357; + const char *over_max_file = "tfilter2_v3_corrupt_over_max.h5"; + const char *over_buffer_file = "tfilter2_v3_corrupt_over_buffer.h5"; + hid_t file = H5I_INVALID_HID, dset = H5I_INVALID_HID; + herr_t open_ret; TESTING("config string: corrupted on-disk config_length is rejected, not crashed on"); @@ -2995,11 +2978,11 @@ error: static int test_config_string_pdecode_corrupted(void) { - hid_t dcpl = H5I_INVALID_HID, dcpl_dec = H5I_INVALID_HID; - void *enc_buf = NULL; - size_t enc_size = 0; - char *padded = NULL; - size_t padded_len; + hid_t dcpl = H5I_INVALID_HID, dcpl_dec = H5I_INVALID_HID; + void *enc_buf = NULL; + size_t enc_size = 0; + char *padded = NULL; + size_t padded_len; uint8_t *needle = NULL; size_t i; @@ -3019,8 +3002,8 @@ test_config_string_pdecode_corrupted(void) if (NULL == (padded = (char *)malloc(padded_len + 1))) TEST_ERROR; { - int n = snprintf(padded, padded_len + 1, "level=9"); - size_t pos = (size_t)n; + int n = snprintf(padded, padded_len + 1, "level=9"); + size_t pos = (size_t)n; unsigned k; for (k = 0; pos + 16 < padded_len; k++) { int flen = snprintf(padded + pos, padded_len + 1 - pos, ",p%u=1", k); diff --git a/tools/lib/CMakeLists.txt b/tools/lib/CMakeLists.txt index 0fde818f93a..b6d9762cf93 100644 --- a/tools/lib/CMakeLists.txt +++ b/tools/lib/CMakeLists.txt @@ -54,7 +54,7 @@ if (BUILD_STATIC_LIBS) TARGET_C_PROPERTIES (${HDF5_TOOLS_LIB_TARGET} STATIC) target_link_libraries (${HDF5_TOOLS_LIB_TARGET} PUBLIC ${HDF5_LIB_TARGET} - PRIVATE "$<$:MPI::MPI_C>" + PRIVATE ${LINK_LIBS} "$<$:MPI::MPI_C>" ) set_global_variable (HDF5_LIBRARIES_TO_EXPORT "${HDF5_LIBRARIES_TO_EXPORT};${HDF5_TOOLS_LIB_TARGET}") H5_SET_LIB_OPTIONS (${HDF5_TOOLS_LIB_TARGET} ${HDF5_TOOLS_LIB_NAME} STATIC 0) @@ -80,7 +80,7 @@ if (BUILD_SHARED_LIBS) TARGET_C_PROPERTIES (${HDF5_TOOLS_LIBSH_TARGET} SHARED) target_link_libraries (${HDF5_TOOLS_LIBSH_TARGET} PUBLIC ${HDF5_LIBSH_TARGET} - PRIVATE "$<$:MPI::MPI_C>" + PRIVATE ${LINK_LIBS} "$<$:MPI::MPI_C>" ) set_global_variable (HDF5_LIBRARIES_TO_EXPORT "${HDF5_LIBRARIES_TO_EXPORT};${HDF5_TOOLS_LIBSH_TARGET}") H5_SET_LIB_OPTIONS (${HDF5_TOOLS_LIBSH_TARGET} ${HDF5_TOOLS_LIB_NAME} SHARED "TOOLS") diff --git a/tools/lib/h5tools_dump.c b/tools/lib/h5tools_dump.c index 2e868bfb3e3..c2716e6809f 100644 --- a/tools/lib/h5tools_dump.c +++ b/tools/lib/h5tools_dump.c @@ -3391,8 +3391,8 @@ h5tools_params_hex_annotation(const char *params, char *out, size_t out_size) p++; if (*p == 'x' || *p == 'X') isfloat = ishex = true; /* C99 hex float or bare hex integer -- strtod() - * consumes either in full; ishex suppresses - * annotation for both. */ + * consumes either in full; ishex suppresses + * annotation for both. */ else if (*p == 'b' || *p == 'B' || *p == 'o' || *p == 'O') { /* TOML binary/octal prefix (0b1010, 0o17): never a float, and * strtod() stops after the leading "0" without understanding this @@ -3867,8 +3867,8 @@ h5tools_dump_dcpl(FILE *stream, const h5tool_format_t *info, h5tools_context_t * const size_t params_buf_size = H5Z_CONFIG_STRING_MAX + 1; char *params_str_buf = (char *)malloc(params_buf_size); char *params_annot = (char *)malloc(params_buf_size); /* hex comment, "" if none */ - char *params_str = NULL; /* escaped, NULL if none to print */ - const char *filter_descr = NULL; /* library-owned, no free needed */ + char *params_str = NULL; /* escaped, NULL if none to print */ + const char *filter_descr = NULL; /* library-owned, no free needed */ bool have_extra; /* true if this filter has a PARAMS_STRING and/or DESCRIPTION */ if (!params_str_buf || !params_annot) { @@ -3902,7 +3902,7 @@ h5tools_dump_dcpl(FILE *stream, const h5tool_format_t *info, h5tools_context_t * * reporting a length past params_buf_size would * otherwise read out of bounds below. Clamp to what was * really copied before using it as a bound. */ - size_t copied = (plen < params_buf_size) ? plen : params_buf_size - 1; + size_t copied = (plen < params_buf_size) ? plen : params_buf_size - 1; params_str_buf[copied] = '\0'; /* Scan the raw string -- not the escaped form below -- for