Replace incorrect use of an internal function (#4668)

* 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

src/H5D.c

@@ -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");

src/H5F.c

@@ -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 */
@@ -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 */
@@ -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 */
@@ -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 */
@@ -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 */
@@ -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 */
@@ -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 */
@@ -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 */
@@ -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 */

test/cache_api.c

@@ -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) {
 
@@ -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) {
@@ -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) {
@@ -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() */

test/tid.c

@@ -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)
 {
@@ -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)
 {
@@ -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");
 }

test/tvltypes.c

@@ -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");