Skip to content

Commit

Permalink
Replace incorrect use of an internal function
Browse files Browse the repository at this point in the history 
In some API functions, the internal function H5I_object() was used instead
of H5I_object_verify(), which verifies the type of an ID argument.  So
when an inappropriate ID was passed in to the affected API, it was accepted.
This behavior can cause issues at a later time, including a segfault, as
reported in issue #HDFGroupGH-4656.

The fix was applied to the following functions:
H5Fget_intent()
H5Fget_fileno()
H5Fget_freespace()
H5Fget_create_plist()
H5Fget_access_plist()
H5Fget_vfd_handle()
H5Dvlen_get_buf_size()
H5Fget_mdc_config()
H5Fset_mdc_config()
H5Freset_mdc_hit_rate_stats()

Fixes HDFGroupGH-4662
  • Loading branch information
@bmribler
bmribler committed Jul 24, 2024
1 parent 1fafdc9 commit 6e7c0a7
Show file tree
Hide file tree
Showing 5 changed files with 226 additions and 10 deletions.
2 changes: 1 addition & 1 deletion src/H5D.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1874,7 +1874,7 @@ H5Dvlen_get_buf_size(hid_t dataset_id, hid_t type_id, hid_t space_id, hsize_t *s
FUNC_ENTER_API(FAIL)

/* Check args */
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object(dataset_id)))
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object_verify(dataset_id, H5I_DATASET)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid dataset identifier");
if (H5I_DATATYPE != H5I_get_type(type_id))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid datatype identifier");
Expand Down
18 changes: 9 additions & 9 deletions src/H5F.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ H5Fget_create_plist(hid_t file_id)
FUNC_ENTER_API(H5I_INVALID_HID)

/* check args */
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object(file_id)))
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object_verify(file_id, H5I_FILE)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, H5I_INVALID_HID, "invalid file identifier");

/* Set up VOL callback arguments */
Expand Down Expand Up @@ -164,7 +164,7 @@ H5Fget_access_plist(hid_t file_id)
FUNC_ENTER_API(H5I_INVALID_HID)

/* Check args */
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object(file_id)))
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object_verify(file_id, H5I_FILE)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, H5I_INVALID_HID, "invalid file identifier");

/* Set up VOL callback arguments */
Expand Down Expand Up @@ -439,7 +439,7 @@ H5Fget_vfd_handle(hid_t file_id, hid_t fapl_id, void **file_handle /*out*/)
HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "invalid file handle pointer");

/* Get the file object */
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object(file_id)))
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object_verify(file_id, H5I_FILE)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid file identifier");

/* Set up VOL callback arguments */
Expand Down Expand Up @@ -1555,7 +1555,7 @@ H5Fget_intent(hid_t file_id, unsigned *intent_flags /*out*/)
H5VL_file_get_args_t vol_cb_args; /* Arguments to VOL callback */

/* Get the internal file structure */
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object(file_id)))
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object_verify(file_id, H5I_FILE)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid file identifier");

/* Set up VOL callback arguments */
Expand Down Expand Up @@ -1594,7 +1594,7 @@ H5Fget_fileno(hid_t file_id, unsigned long *fnumber /*out*/)
H5VL_file_get_args_t vol_cb_args; /* Arguments to VOL callback */

/* Get the internal file structure */
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object(file_id)))
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object_verify(file_id, H5I_FILE)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid file identifier");

/* Set up VOL callback arguments */
Expand Down Expand Up @@ -1631,7 +1631,7 @@ H5Fget_freespace(hid_t file_id)
FUNC_ENTER_API((-1))

/* Get the file object */
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object(file_id)))
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object_verify(file_id, H5I_FILE)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, (-1), "invalid file identifier");

/* Set up VOL callback arguments */
Expand Down Expand Up @@ -1789,7 +1789,7 @@ H5Fget_mdc_config(hid_t file_id, H5AC_cache_config_t *config /*out*/)
HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "Bad config ptr");

/* Get the file object */
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object(file_id)))
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object_verify(file_id, H5I_FILE)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid file identifier");

/* Set up VOL callback arguments */
Expand Down Expand Up @@ -1827,7 +1827,7 @@ H5Fset_mdc_config(hid_t file_id, const H5AC_cache_config_t *config_ptr)
FUNC_ENTER_API(FAIL)

/* Get the file object */
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object(file_id)))
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object_verify(file_id, H5I_FILE)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid file identifier");

/* Set up VOL callback arguments */
Expand Down Expand Up @@ -1959,7 +1959,7 @@ H5Freset_mdc_hit_rate_stats(hid_t file_id)
FUNC_ENTER_API(FAIL)

/* Get the file object */
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object(file_id)))
if (NULL == (vol_obj = (H5VL_object_t *)H5I_object_verify(file_id, H5I_FILE)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid file identifier");

/* Set up VOL callback arguments */
Expand Down
73 changes: 73 additions & 0 deletions test/cache_api.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1858,6 +1858,15 @@ check_file_mdc_api_errs(unsigned paged, hid_t fcpl_id)

/* test H5Fget_mdc_config(). */

/* Create an ID to use in the H5Fset_mdc_config/H5Fget_mdc_config tests */
hid_t dtype_id = H5Tcopy(H5T_NATIVE_INT);

if (dtype_id < 0) {

pass = false;
failure_mssg = "H5Tcopy() failed.\n";
}

scratch.version = H5C__CURR_AUTO_SIZE_CTL_VER;
if (pass) {

Expand All @@ -1877,6 +1886,18 @@ check_file_mdc_api_errs(unsigned paged, hid_t fcpl_id)
pass = false;
failure_mssg = "H5Fget_mdc_config() accepted invalid file_id.";
}

H5E_BEGIN_TRY
{
result = H5Fget_mdc_config(dtype_id, &scratch); /* not a file ID */
}
H5E_END_TRY

if (result >= 0) {

pass = false;
failure_mssg = "H5Fget_mdc_config() accepted an ID that is not a file ID.";
}
}

if (pass) {
Expand Down Expand Up @@ -1941,6 +1962,27 @@ check_file_mdc_api_errs(unsigned paged, hid_t fcpl_id)
pass = false;
failure_mssg = "H5Fset_mdc_config() accepted bad invalid file_id.";
}

H5E_BEGIN_TRY
{
result = H5Fset_mdc_config(dtype_id, &default_config);
}
H5E_END_TRY

if (result >= 0) {

pass = false;
failure_mssg = "H5Fset_mdc_config() accepted an ID that is not a file ID.";
}
}

/* Close the temporary datatype */
result = H5Tclose(dtype_id);

if (result < 0) {

pass = false;
failure_mssg = "H5Tclose() failed.\n";
}

if (pass) {
Expand Down Expand Up @@ -2050,6 +2092,37 @@ check_file_mdc_api_errs(unsigned paged, hid_t fcpl_id)
pass = false;
failure_mssg = "H5Freset_mdc_hit_rate_stats() accepted bad file_id.";
}

/* Create an ID to use in the next test */
hid_t scalarsp_id = H5Screate(H5S_SCALAR);

if (scalarsp_id < 0) {

pass = false;
failure_mssg = "H5Screate() failed.\n";
}

/* Try to call H5Freset_mdc_hit_rate_stats with an inappropriate ID */
H5E_BEGIN_TRY
{
result = H5Freset_mdc_hit_rate_stats(scalarsp_id);
}
H5E_END_TRY

if (result >= 0) {

pass = false;
failure_mssg = "H5Freset_mdc_hit_rate_stats() accepted an ID that is not a file_id.";
}

/* Close the temporary dataspace */
result = H5Sclose(scalarsp_id);

if (result < 0) {

pass = false;
failure_mssg = "H5Sclose() failed.\n";
}
}

/* test H5Fget_mdc_size() */
Expand Down
127 changes: 127 additions & 0 deletions test/tid.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
#define H5I_FRIEND /*suppress error about including H5Ipkg */
#include "H5Ipkg.h"

/* Defines used in test_appropriate_ids */
#define FILE_NAME "tid.h5"
#define DSET_NAME "Dataset 1"

static herr_t
free_wrapper(void *p, void H5_ATTR_UNUSED **_ctx)
{
Expand Down Expand Up @@ -1369,6 +1373,127 @@ test_future_ids(void)
return -1;
} /* end test_future_ids() */

/*-------------------------------------------------------------------------
* Function: test_appropriate_ids
*
* Purpose: Tests several API functions on detecting inappropriate ID.
*
* Return: Success: 0
* Failure: number of errors
*
*-------------------------------------------------------------------------
*/
static int
test_appropriate_ids(void)
{
hid_t file_id = H5I_INVALID_HID;
hid_t fapl_id = H5I_INVALID_HID;
hid_t fcpl_id = H5I_INVALID_HID;
hid_t plist = H5I_INVALID_HID;
hid_t dset_id = H5I_INVALID_HID;
hid_t space_id = H5I_INVALID_HID;
hsize_t dims = 2;
hssize_t free_space;
herr_t ret = SUCCEED; /* Generic return value */

/* Create file create property list */
fcpl_id = H5Pcreate(H5P_FILE_CREATE);
CHECK(fcpl_id, H5I_INVALID_HID, "H5Pcreate");

file_id = H5Fcreate(FILE_NAME, H5F_ACC_TRUNC, fcpl_id, H5P_DEFAULT);
CHECK(file_id, H5I_INVALID_HID, "H5Fcreate");

/* Create a dataset in the file */
space_id = H5Screate_simple(1, &dims, NULL);
CHECK(space_id, H5I_INVALID_HID, "H5Screate_simple");
dset_id = H5Dcreate2(file_id, DSET_NAME, H5T_NATIVE_INT, space_id, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT);
CHECK(dset_id, H5I_INVALID_HID, "H5Dcreate2");

/* Close IDs */
ret = H5Pclose(fcpl_id);
CHECK(ret, FAIL, "H5Pclose");
ret = H5Sclose(space_id);
CHECK(ret, FAIL, "H5Sclose");
ret = H5Dclose(dset_id);
CHECK(ret, FAIL, "H5Dclose");
ret = H5Fclose(file_id);
CHECK(ret, FAIL, "H5Fclose");

file_id = H5Fopen(FILE_NAME, H5F_ACC_RDONLY, H5P_DEFAULT);
CHECK(file_id, H5I_INVALID_HID, "H5Fopen");

/* Get the file create property */
fcpl_id = H5Fget_create_plist(file_id);
CHECK(fcpl_id, H5I_INVALID_HID, "H5Fget_create_plist");

/* Get the file access property */
fapl_id = H5Fget_access_plist(file_id);
CHECK(fapl_id, H5I_INVALID_HID, "H5Fget_access_plist");

dset_id = H5Dopen2(file_id, DSET_NAME, H5P_DEFAULT);
CHECK(dset_id, H5I_INVALID_HID, "H5Dopen2");

/*-------------------------------------------------------------
* Try to call functions passing in a wrong ID
*-----------------------------------------------------------*/
H5E_BEGIN_TRY
{
plist = H5Fget_create_plist(dset_id); /* dset_id is not file ID */
}
H5E_END_TRY
VERIFY(plist, H5I_INVALID_HID, "H5Fget_create_plist");

H5E_BEGIN_TRY
{
plist = H5Fget_access_plist(fapl_id); /* fapl_id is not file ID */
}
H5E_END_TRY
VERIFY(plist, H5I_INVALID_HID, "H5Fget_access_plist");

H5E_BEGIN_TRY
{
unsigned intent; /* File access flags */
ret = H5Fget_intent(dset_id, &intent); /* dset_id is not file ID */
}
H5E_END_TRY
VERIFY(ret, FAIL, "H5Fget_intent");

H5E_BEGIN_TRY
{
unsigned long fileno = 0;
ret = H5Fget_fileno(dset_id, &fileno); /* dset_id is not file ID */
}
H5E_END_TRY
VERIFY(ret, FAIL, "H5Fget_fileno");

H5E_BEGIN_TRY
{
free_space = H5Fget_freespace(dset_id); /* dset_id is not file ID */
}
H5E_END_TRY
VERIFY(free_space, FAIL, "H5Fget_freespace");

H5E_BEGIN_TRY
{
void *os_file_handle = NULL; /* OS file handle */
ret = H5Fget_vfd_handle(fapl_id, H5P_DEFAULT, &os_file_handle); /* fapl_id is not file ID */
}
H5E_END_TRY
VERIFY(ret, FAIL, "H5Fget_vfd_handle");

/* Close IDs */
ret = H5Pclose(fapl_id);
CHECK(ret, FAIL, "H5Pclose");
ret = H5Pclose(fcpl_id);
CHECK(ret, FAIL, "H5Pclose");
ret = H5Dclose(dset_id);
CHECK(ret, FAIL, "H5Dclose");
ret = H5Fclose(file_id);
CHECK(ret, FAIL, "H5Fclose");

return 0;
}

void
test_ids(void)
{
Expand All @@ -1389,4 +1514,6 @@ test_ids(void)
TestErrPrintf("ID remove during H5Iclear_type test failed\n");
if (test_future_ids() < 0)
TestErrPrintf("Future ID test failed\n");
if (test_appropriate_ids() < 0)
TestErrPrintf("Detection of inappropriate ID test failed\n");
}
16 changes: 16 additions & 0 deletions test/tvltypes.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,22 @@ test_vltypes_vlen_atomic(void)
H5E_END_TRY
VERIFY(ret, FAIL, "H5Dvlen_get_buf_size");

/* Try to call H5Dvlen_get_buf_size with a wrong ID */
H5E_BEGIN_TRY
{
ret = H5Dvlen_get_buf_size(tid1, dataset, sid2, &size); /* IDs in wrong order */
}
H5E_END_TRY
VERIFY(ret, FAIL, "H5Dvlen_get_buf_size");

/* Try to call H5Dvlen_get_buf_size with a wrong ID */
H5E_BEGIN_TRY
{
ret = H5Dvlen_get_buf_size(fid1, tid1, sid2, &size); /* not a dataset ID */
}
H5E_END_TRY
VERIFY(ret, FAIL, "H5Dvlen_get_buf_size");

/* Read dataset from disk */
ret = H5Dread(dataset, tid1, H5S_ALL, H5S_ALL, xfer_pid, rdata);
CHECK(ret, FAIL, "H5Dread");
Expand Down

0 comments on commit 6e7c0a7

Please sign in to comment.