Skip to content

Commit

Permalink
Replace incorrect use of an internal function (#4668)
Browse files Browse the repository at this point in the history 
* Replace incorrect use of an internal function

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 #GH-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 GH-4662
  • Loading branch information
@bmribler
bmribler authored Jul 24, 2024
1 parent 6cf3389 commit 9fd4fd0
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 9fd4fd0

Please sign in to comment.