Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Fix issue with invalid compound conversions performing no-op I/O #4166

Closed
wants to merge 2 commits into from
+218 −4
Closed

Fix issue with invalid compound conversions performing no-op I/O #4166

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5548a16
Fix issue with invalid compound conversions performing no-op I/O
jhendersonHDF Mar 17, 2024
b8fab0d
Merge branch 'develop' into compound_conv_empty_subset
jhendersonHDF Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions release_docs/RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,17 @@ Bug Fixes since HDF5-1.14.0 release

Library
-------
- Fixed an issue where certain invalid compound datatype conversions would
cause I/O calls to incorrectly succeed

When performing I/O on a dataset or attribute with an in-memory compound
datatype where data conversion is required, but none of the members of
the datatype appear in the object's compound datatype in the file, the
library would perform no-op I/O rather than returning an error about not
finding a valid path for the data conversion. This would result in the
I/O call succeeding while not actually writing or reading any data. The
library now correctly returns an error in this case.

- Fixed error when overwriting certain nested variable length types

Previously, when using a datatype that included a variable length type
Expand Down
13 changes: 12 additions & 1 deletion src/H5Tconv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2003,7 +2003,8 @@ H5T__conv_struct_init(H5T_t *src, H5T_t *dst, H5T_cdata_t *cdata, const H5T_conv
int *src2dst = NULL;
unsigned src_nmembs, dst_nmembs;
unsigned i, j;
herr_t ret_value = SUCCEED; /* Return value */
bool empty_source_subset = false;
herr_t ret_value = SUCCEED; /* Return value */

FUNC_ENTER_PACKAGE

Expand Down Expand Up @@ -2114,11 +2115,14 @@ H5T__conv_struct_init(H5T_t *src, H5T_t *dst, H5T_cdata_t *cdata, const H5T_conv
(priv->memb_path = (H5T_path_t **)H5MM_malloc(src->shared->u.compnd.nmembs * sizeof(H5T_path_t *))))
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "memory allocation failed");

empty_source_subset = true;
for (i = 0; i < src_nmembs; i++) {
if (src2dst[i] >= 0) {
H5T_path_t *tpath = H5T_path_find(src->shared->u.compnd.memb[i].type,
dst->shared->u.compnd.memb[src2dst[i]].type);

empty_source_subset = false;

if (NULL == (priv->memb_path[i] = tpath)) {
H5T__conv_struct_free(priv);
cdata->priv = NULL;
Expand All @@ -2127,6 +2131,13 @@ H5T__conv_struct_init(H5T_t *src, H5T_t *dst, H5T_cdata_t *cdata, const H5T_conv
} /* end if */
} /* end for */

if (empty_source_subset) {
H5T__conv_struct_free(priv);
cdata->priv = NULL;
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCONVERT, FAIL,
"no members in source compound type appear in destination compound type");
}

/* The compound conversion functions need a background buffer */
cdata->need_bkg = H5T_BKG_YES;

Expand Down
1 change: 1 addition & 0 deletions test/CMakeTests.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ set (test_CLEANFILES
dtransform.h5
dtypes3.h5
dtypes4.h5
dtypes11.h5
min_dset_ohdr_testfile.h5
ohdr_min_a.h5
sec2_file.h5
Expand Down
2 changes: 1 addition & 1 deletion test/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ CHECK_CLEANFILES+=accum.h5 cmpd_dset.h5 mdset.h5 compact_dataset.h5 dataset.h5 d
dt_arith[1-2] links.h5 links[0-6]*.h5 extlinks[0-15].h5 \
tmp tmp_links tmp2_links tmp_links_env tmp_vds tmp_vds_env \
big.data big[0-9][0-9][0-9][0-9][0-9].h5 \
stdio.h5 sec2.h5 dtypes[0-9].h5 dtypes1[0].h5 dt_arith[1-2].h5 tattr.h5 \
stdio.h5 sec2.h5 dtypes[0-9].h5 dtypes1[0-1].h5 dt_arith[1-2].h5 tattr.h5 \
tselect.h5 mtime.h5 unlink.h5 unicode.h5 coord.h5 \
fillval_[0-9].h5 fillval.raw mount_[0-9].h5 testmeta.h5 ttime.h5 \
trefer[1-3].h5 trefer_*.h5 tvltypes.h5 tvlstr.h5 tvlstr2.h5 twriteorder.dat \
Expand Down
195 changes: 193 additions & 2 deletions test/dtypes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,9 @@
} \
} while (0)

static const char *FILENAME[] = {"dtypes0", "dtypes1", "dtypes2", "dtypes3", "dtypes4", "dtypes5",
"dtypes6", "dtypes7", "dtypes8", "dtypes9", "dtypes10", NULL};
static const char *FILENAME[] = {"dtypes0", "dtypes1", "dtypes2", "dtypes3", "dtypes4",
"dtypes5", "dtypes6", "dtypes7", "dtypes8", "dtypes9",
"dtypes10", "dtypes11", NULL};

#define TESTFILE "bad_compound.h5"

Expand Down Expand Up @@ -129,6 +130,8 @@ typedef enum { E1_RED, E1_GREEN, E1_BLUE, E1_ORANGE, E1_YELLOW } color_t;
#define ASCII_DATASET "ascii"
#define ASCII_DATASET2 "2nd_ascii"

#define COMPOUND19_NUMELEMS 10

/* Count opaque conversions */
static int num_opaque_conversions_g = 0;

Expand Down Expand Up @@ -3819,6 +3822,193 @@ test_compound_18(void)
return 1;
} /* end test_compound_18() */

/*-------------------------------------------------------------------------
* Function: test_compound_19
*
* Purpose: Tests that the library fails correctly when trying to
* convert between compounds where none of the members in the
* source compound appear in the destination compound.
*
* Return: Success: 0
* Failure: number of errors
*
*-------------------------------------------------------------------------
*/
static int
test_compound_19(void)
{
#if H5_SIZEOF_INT32_T != 0
H5T_order_t byte_order;
hsize_t dset_dims[] = {COMPOUND19_NUMELEMS};
hid_t fid = H5I_INVALID_HID;
hid_t did = H5I_INVALID_HID;
hid_t space_id = H5I_INVALID_HID;
hid_t dst_compound = H5I_INVALID_HID;
herr_t status;
char filename[512];
#if H5_SIZEOF_INT16_T != 0
hid_t compound_i16 = H5I_INVALID_HID;
#endif
#if H5_SIZEOF_INT32_T != 0
hid_t compound_i32 = H5I_INVALID_HID;
#endif
#if H5_SIZEOF_INT64_T != 0
hid_t compound_i64 = H5I_INVALID_HID;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed size types are used here just to make sure that the optimizations in struct conversions around smaller or larger destination types don't potentially break this in the future

#endif
#endif

TESTING("compound conversions where destination compound has no members from source compound");

#if H5_SIZEOF_INT32_T != 0
Copy link
Member

Choose a reason for hiding this comment

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

Why the ifdefs on type sizes? We demand C99 types and use them elsewhere in the library without protection.

Copy link
Collaborator Author

@jhendersonHDF jhendersonHDF Mar 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

int8_t, 16, 32 and 64 are optional in C99, though I suspect they're fairly widely supported. It could be a problem on some systems though; this is just being cautious about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The least and fast variants are guaranteed in C99 for 8, 16, 32, and 64, but the exact width types aren't.

Copy link
Member

Choose a reason for hiding this comment

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

We use int32_t, etc. throughout the library. A hid_t ID is defined to be an int64_t. It's basically a requirement to build, or even use, HDF5. There's no need for the ifdefs here.

Copy link
Collaborator Author

@jhendersonHDF jhendersonHDF Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just because int64_t is defined doesn't guarantee that int16_t is, for example. I can remove the ifdefs if we think it's just visual noise, but something to think about. If those are requirements for the library, it may not be a bad idea to throw an error at configure time if one of the types we use directly is missing (if we don't already have that).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think int16_t is as well-established as int32_t and int64_t at this point.

if ((byte_order = H5Tget_order(H5T_NATIVE_INT)) < 0)
TEST_ERROR;

if ((dst_compound = H5Tcreate(H5T_COMPOUND, H5Tget_size(H5T_STD_I32LE))) < 0)
TEST_ERROR;
if (H5Tinsert(dst_compound, "different_mem", 0, H5T_STD_I32LE) < 0)
TEST_ERROR;
if (H5Tset_order(dst_compound, byte_order) < 0)
TEST_ERROR;

h5_fixname(FILENAME[11], H5P_DEFAULT, filename, sizeof filename);
if ((fid = H5Fcreate(filename, H5F_ACC_TRUNC, H5P_DEFAULT, H5P_DEFAULT)) < 0)
TEST_ERROR;

if ((space_id = H5Screate_simple(1, dset_dims, NULL)) < 0)
TEST_ERROR;

if ((did = H5Dcreate2(fid, "/dset", dst_compound, space_id, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT)) < 0)
TEST_ERROR;

#if H5_SIZEOF_INT16_T != 0
{
int16_t int16_data[COMPOUND19_NUMELEMS];

for (size_t i = 0; i < COMPOUND19_NUMELEMS; i++)
int16_data[i] = (int16_t)(rand() % INT16_MAX);

/* Test with a smaller source compound type */
if ((compound_i16 = H5Tcreate(H5T_COMPOUND, H5Tget_size(H5T_STD_I16LE))) < 0)
TEST_ERROR;
if (H5Tinsert(compound_i16, "i16_mem", 0, H5T_STD_I16LE) < 0)
TEST_ERROR;
if (H5Tset_order(dst_compound, byte_order) < 0)
TEST_ERROR;

H5E_BEGIN_TRY
{
/* Should fail with "no appropriate function for conversion path" */
status = H5Dwrite(did, compound_i16, H5S_ALL, H5S_ALL, H5P_DEFAULT, int16_data);
}
H5E_END_TRY

if (status >= 0)
TEST_ERROR;

if (H5Tclose(compound_i16) < 0)
TEST_ERROR;
}
#endif

{
int32_t int32_data[COMPOUND19_NUMELEMS];

for (size_t i = 0; i < COMPOUND19_NUMELEMS; i++)
int32_data[i] = (int32_t)(rand() % INT32_MAX);

/* Test with an equal-sized source compound type */
if ((compound_i32 = H5Tcreate(H5T_COMPOUND, H5Tget_size(H5T_STD_I32LE))) < 0)
TEST_ERROR;
if (H5Tinsert(compound_i32, "i32_mem", 0, H5T_STD_I32LE) < 0)
TEST_ERROR;
if (H5Tset_order(dst_compound, byte_order) < 0)
TEST_ERROR;

H5E_BEGIN_TRY
{
/* Should fail with "no appropriate function for conversion path" */
status = H5Dwrite(did, compound_i32, H5S_ALL, H5S_ALL, H5P_DEFAULT, int32_data);
}
H5E_END_TRY

if (status >= 0)
TEST_ERROR;

if (H5Tclose(compound_i32) < 0)
TEST_ERROR;
}

#if H5_SIZEOF_INT64_T != 0
{
int64_t int64_data[COMPOUND19_NUMELEMS];

for (size_t i = 0; i < COMPOUND19_NUMELEMS; i++)
int64_data[i] = (int64_t)(rand() % INT64_MAX);

/* Test with a larger source compound type */
if ((compound_i64 = H5Tcreate(H5T_COMPOUND, H5Tget_size(H5T_STD_I64LE))) < 0)
TEST_ERROR;
if (H5Tinsert(compound_i64, "i64_mem", 0, H5T_STD_I64LE) < 0)
TEST_ERROR;
if (H5Tset_order(dst_compound, byte_order) < 0)
TEST_ERROR;

H5E_BEGIN_TRY
{
/* Should fail with "no appropriate function for conversion path" */
status = H5Dwrite(did, compound_i64, H5S_ALL, H5S_ALL, H5P_DEFAULT, int64_data);
}
H5E_END_TRY

if (status >= 0)
TEST_ERROR;

if (H5Tclose(compound_i64) < 0)
TEST_ERROR;
}
#endif

if (H5Tclose(dst_compound) < 0)
TEST_ERROR;
if (H5Sclose(space_id) < 0)
TEST_ERROR;
if (H5Dclose(did) < 0)
TEST_ERROR;
if (H5Fclose(fid) < 0)
TEST_ERROR;

H5Fdelete(filename, H5P_DEFAULT);

PASSED();

return 0;

error:
H5E_BEGIN_TRY
{
#if H5_SIZEOF_INT16_T != 0
H5Tclose(compound_i16);
#endif
#if H5_SIZEOF_INT32_T != 0
H5Tclose(compound_i32);
#endif
#if H5_SIZEOF_INT64_T != 0
H5Tclose(compound_i64);
#endif
H5Tclose(dst_compound);
H5Sclose(space_id);
H5Dclose(did);
H5Fclose(fid);
}
H5E_END_TRY

return 1;
#else
SKIPPED();
return 0;
#endif
} /* end test_compound_19() */

/*-------------------------------------------------------------------------
* Function: test_user_compound_conversion
*
Expand Down Expand Up @@ -9163,6 +9353,7 @@ main(void)
nerrors += test_compound_16();
nerrors += test_compound_17();
nerrors += test_compound_18();
nerrors += test_compound_19();
nerrors += test_user_compound_conversion();
nerrors += test_conv_enum_1();
nerrors += test_conv_enum_2();
Expand Down
Loading