mirror of
https://github.com/HDFGroup/hdf5.git
synced 2026-09-25 04:09:44 +03:00
Fix H5Z_modify accepting a filter that is not in the pipeline
The not-found check used a strict > where the search loop can only
leave idx == nused. When the requested filter was absent the check
passed, and the function went on to write flags, cd_nelmts, cd_values
and config into pline->filter[nused] -- past the used entries.
Once nused has reached nalloc (H5Z_MAX_NFILTERS, 32, which is also the
initial nalloc) that slot is one past the end of the array, so the
write runs off the allocation. Even below that bound, a cd_nelmts
greater than H5Z_COMMON_CD_VALUES makes the entry own a heap allocation
that nothing can reach or free.
Confirmed both effects against the unfixed build:
- H5Pmodify_filter(dcpl, <filter not in pipeline>, ...) returned
success where it must fail;
- with a full 32-filter pipeline and cd_nelmts = 8, ASAN reported
"Direct leak of 32 byte(s) ... in H5Z_modify" for the cd_values
buffer stranded in the out-of-range slot.
There is no membership pre-check anywhere in the H5Pmodify_filter ->
H5P_modify_filter -> H5Z_modify path, so the API was reachable directly
from public code. Predates the RFC work.
Adds test_modify_filter_absent, covering both the ordinary and the
full-pipeline cases; it fails against the strict > version.
This commit is contained in:
@@ -1318,8 +1318,12 @@ H5Z_modify(const H5O_pline_t *pline, H5Z_filter_t filter, unsigned flags, size_t
|
||||
if (pline->filter[idx].id == filter)
|
||||
break;
|
||||
|
||||
/* Check if the filter was not already in the pipeline */
|
||||
if (idx > pline->nused)
|
||||
/* Check if the filter was not already in the pipeline. The loop above
|
||||
* leaves idx == nused when the filter is absent, so this must be >= :
|
||||
* with a strict > the absent case fell through and wrote to
|
||||
* filter[nused] -- past the used entries, and past the end of the array
|
||||
* entirely once nused has reached nalloc (H5Z_MAX_NFILTERS). */
|
||||
if (idx >= pline->nused)
|
||||
HGOTO_ERROR(H5E_PLINE, H5E_NOTFOUND, FAIL, "filter not in pipeline");
|
||||
|
||||
/* Change parameters for filter */
|
||||
|
||||
@@ -558,6 +558,79 @@ error:
|
||||
* This test verifies that a filter appended via the string API produces
|
||||
* cd_values that round-trip correctly through this pattern.
|
||||
* ---------------------------------------------------------------------- */
|
||||
|
||||
/* H5Pmodify_filter on a filter that is not in the pipeline must fail.
|
||||
*
|
||||
* H5Z_modify locates the filter with a loop that leaves idx == nused when
|
||||
* the filter is absent, so its not-found check has to be >= rather than >.
|
||||
* With a strict >, the absent case fell through and wrote to
|
||||
* filter[nused]: past the used entries, and past the end of the array
|
||||
* altogether once nused reached nalloc (H5Z_MAX_NFILTERS), leaking the
|
||||
* cd_values allocation it stored there. */
|
||||
static int
|
||||
test_modify_filter_absent(void)
|
||||
{
|
||||
hid_t dcpl = H5I_INVALID_HID;
|
||||
unsigned cd[8];
|
||||
herr_t status;
|
||||
|
||||
TESTING("H5Pmodify_filter: absent filter is rejected");
|
||||
|
||||
for (size_t i = 0; i < 8; i++)
|
||||
cd[i] = 0xAAAAAAAAu;
|
||||
|
||||
if ((dcpl = H5Pcreate(H5P_DATASET_CREATE)) < 0)
|
||||
TEST_ERROR;
|
||||
if (H5Pset_deflate(dcpl, 6) < 0)
|
||||
TEST_ERROR;
|
||||
|
||||
/* Shuffle is not in the pipeline */
|
||||
H5E_BEGIN_TRY
|
||||
{
|
||||
status = H5Pmodify_filter(dcpl, H5Z_FILTER_SHUFFLE, 0, 1, cd);
|
||||
}
|
||||
H5E_END_TRY
|
||||
if (status >= 0)
|
||||
TEST_ERROR;
|
||||
if (H5Pget_nfilters(dcpl) != 1)
|
||||
TEST_ERROR;
|
||||
if (H5Pclose(dcpl) < 0)
|
||||
TEST_ERROR;
|
||||
dcpl = H5I_INVALID_HID;
|
||||
|
||||
/* Same check with the pipeline filled to H5Z_MAX_NFILTERS, where
|
||||
* nused == nalloc and the stray write ran off the end of the array.
|
||||
* cd_nelmts > H5Z_COMMON_CD_VALUES so the entry would own a heap
|
||||
* allocation that nothing could ever free. */
|
||||
if ((dcpl = H5Pcreate(H5P_DATASET_CREATE)) < 0)
|
||||
TEST_ERROR;
|
||||
for (int i = 0; i < H5Z_MAX_NFILTERS; i++)
|
||||
if (H5Pset_filter(dcpl, H5Z_FILTER_SHUFFLE, H5Z_FLAG_OPTIONAL, 0, NULL) < 0)
|
||||
TEST_ERROR;
|
||||
if (H5Pget_nfilters(dcpl) != H5Z_MAX_NFILTERS)
|
||||
TEST_ERROR;
|
||||
H5E_BEGIN_TRY
|
||||
{
|
||||
status = H5Pmodify_filter(dcpl, H5Z_FILTER_SZIP, 0, 8, cd);
|
||||
}
|
||||
H5E_END_TRY
|
||||
if (status >= 0)
|
||||
TEST_ERROR;
|
||||
if (H5Pclose(dcpl) < 0)
|
||||
TEST_ERROR;
|
||||
|
||||
PASSED();
|
||||
return 0;
|
||||
|
||||
error:
|
||||
H5E_BEGIN_TRY
|
||||
{
|
||||
H5Pclose(dcpl);
|
||||
}
|
||||
H5E_END_TRY
|
||||
return -1;
|
||||
}
|
||||
|
||||
static int
|
||||
test_modify_filter_pattern(void)
|
||||
{
|
||||
@@ -3024,6 +3097,7 @@ main(void)
|
||||
nerrors += test_callback_contracts() < 0 ? 1 : 0;
|
||||
|
||||
/* Modify-filter pattern (H5Pget_filter_by_id2 + H5Pmodify_filter) */
|
||||
nerrors += test_modify_filter_absent() < 0 ? 1 : 0;
|
||||
nerrors += test_modify_filter_pattern() < 0 ? 1 : 0;
|
||||
|
||||
/* Round-trip tests */
|
||||
|
||||
Reference in New Issue
Block a user