mirror of
https://github.com/HDFGroup/hdf5.git
synced 2026-09-25 04:09:44 +03:00
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.
This commit is contained in:
Scot Breitenfeld
@@ -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`).
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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';
|
||||
|
||||
+5
-5
@@ -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 */
|
||||
|
||||
@@ -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
|
||||
|
||||
+37
-54
@@ -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);
|
||||
|
||||
@@ -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 "$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:MPI::MPI_C>"
|
||||
PRIVATE ${LINK_LIBS} "$<$<BOOL:${HDF5_ENABLE_PARALLEL}>: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 "$<$<BOOL:${HDF5_ENABLE_PARALLEL}>:MPI::MPI_C>"
|
||||
PRIVATE ${LINK_LIBS} "$<$<BOOL:${HDF5_ENABLE_PARALLEL}>: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")
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user