From a75542b3e77664f5f4be39dac24e9e3c791456da Mon Sep 17 00:00:00 2001 From: bmribler <39579120+bmribler@users.noreply.github.com> Date: Fri, 6 Sep 2024 11:50:10 -0400 Subject: [PATCH] Detect invalid ID to H5Gmove2 (#4765) User's application segfaulted because the returned value H5I_BADID wasn't detected when H5I_get_type() was called. This PR adds checks for invalid file/group identifiers passed into H5Gmove2. This defect occurs in many other places, hence, issue GH-4764. Fixes #4737 --- src/H5Gdeprec.c | 43 ++++++++++++++++++++------- src/H5L.c | 74 +++++++++++++++++++++++++++++++--------------- src/H5VLcallback.c | 4 +++ test/links.c | 53 +++++++++++++++++++++++++++++---- 4 files changed, 135 insertions(+), 39 deletions(-) diff --git a/src/H5Gdeprec.c b/src/H5Gdeprec.c index 6871283ed03..5f9ad63756e 100644 --- a/src/H5Gdeprec.c +++ b/src/H5Gdeprec.c @@ -519,10 +519,35 @@ H5Gmove2(hid_t src_loc_id, const char *src_name, hid_t dst_loc_id, const char *d H5VL_loc_params_t loc_params1; H5VL_object_t *vol_obj2 = NULL; /* Object of dst_id */ H5VL_loc_params_t loc_params2; + H5I_type_t src_id_type = H5I_BADID, dst_id_type = H5I_BADID; herr_t ret_value = SUCCEED; /* Return value */ FUNC_ENTER_API(FAIL) + /* Check arguments */ + if (!src_name || !*src_name) + HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "no current name specified"); + if (!dst_name || !*dst_name) + HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "no destination name specified"); + + /* src and dst location IDs cannot both have the value of H5L_SAME_LOC */ + if (src_loc_id == H5L_SAME_LOC && dst_loc_id == H5L_SAME_LOC) + HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "current and destination should not both be H5L_SAME_LOC"); + + /* reset an ID in the case of H5L_SAME_LOC */ + if (src_loc_id == H5L_SAME_LOC) + src_loc_id = dst_loc_id; + else if (dst_loc_id == H5L_SAME_LOC) + dst_loc_id = src_loc_id; + + src_id_type = H5I_get_type(src_loc_id); + if (!(H5I_GROUP == src_id_type || H5I_FILE == src_id_type)) + HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "invalid group (or file) ID, src_loc_id"); + + dst_id_type = H5I_get_type(dst_loc_id); + if (!(H5I_GROUP == dst_id_type || H5I_FILE == dst_id_type)) + HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "invalid group (or file) ID, dst_loc_id"); + /* Set up collective metadata if appropriate */ if (H5CX_set_loc(dst_loc_id) < 0) HGOTO_ERROR(H5E_SYM, H5E_CANTSET, FAIL, "can't set collective metadata read info"); @@ -531,22 +556,20 @@ H5Gmove2(hid_t src_loc_id, const char *src_name, hid_t dst_loc_id, const char *d loc_params1.type = H5VL_OBJECT_BY_NAME; loc_params1.loc_data.loc_by_name.name = src_name; loc_params1.loc_data.loc_by_name.lapl_id = H5P_LINK_ACCESS_DEFAULT; - loc_params1.obj_type = H5I_get_type(src_loc_id); + loc_params1.obj_type = src_id_type; /* Set location parameter for destination object */ loc_params2.type = H5VL_OBJECT_BY_NAME; loc_params2.loc_data.loc_by_name.name = dst_name; loc_params2.loc_data.loc_by_name.lapl_id = H5P_LINK_ACCESS_DEFAULT; - loc_params2.obj_type = H5I_get_type(dst_loc_id); + loc_params2.obj_type = dst_id_type; - if (H5L_SAME_LOC != src_loc_id) - /* get the location object */ - if (NULL == (vol_obj1 = H5VL_vol_object(src_loc_id))) - HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); - if (H5L_SAME_LOC != dst_loc_id) - /* get the location object */ - if (NULL == (vol_obj2 = H5VL_vol_object(dst_loc_id))) - HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); + /* get the location object */ + if (NULL == (vol_obj1 = H5VL_vol_object(src_loc_id))) + HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); + /* get the location object */ + if (NULL == (vol_obj2 = H5VL_vol_object(dst_loc_id))) + HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); /* Move the link */ if (H5VL_link_move(vol_obj1, &loc_params1, vol_obj2, &loc_params2, H5P_LINK_CREATE_DEFAULT, diff --git a/src/H5L.c b/src/H5L.c index cbc584f6b44..3616cb75a59 100644 --- a/src/H5L.c +++ b/src/H5L.c @@ -94,7 +94,8 @@ H5Lmove(hid_t src_loc_id, const char *src_name, hid_t dst_loc_id, const char *ds H5VL_object_t *vol_obj2 = NULL; /* Object of dst_id */ H5VL_loc_params_t loc_params1; H5VL_loc_params_t loc_params2; - H5VL_object_t tmp_vol_obj; /* Temporary object */ + H5VL_object_t tmp_vol_obj; /* Temporary object */ + H5I_type_t src_id_type = H5I_BADID, dst_id_type = H5I_BADID; herr_t ret_value = SUCCEED; /* Return value */ FUNC_ENTER_API(FAIL) @@ -106,6 +107,21 @@ H5Lmove(hid_t src_loc_id, const char *src_name, hid_t dst_loc_id, const char *ds HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "no current name specified"); if (!dst_name || !*dst_name) HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "no destination name specified"); + + /* reset an ID in the case of H5L_SAME_LOC */ + if (src_loc_id == H5L_SAME_LOC) + src_loc_id = dst_loc_id; + else if (dst_loc_id == H5L_SAME_LOC) + dst_loc_id = src_loc_id; + + /* verify that src and dst IDs are either a file or a group ID */ + src_id_type = H5I_get_type(src_loc_id); + if (!(H5I_GROUP == src_id_type || H5I_FILE == src_id_type)) + HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "invalid group (or file) ID, src_loc_id"); + dst_id_type = H5I_get_type(dst_loc_id); + if (!(H5I_GROUP == dst_id_type || H5I_FILE == dst_id_type)) + HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "invalid group (or file) ID, dst_loc_id"); + if (lcpl_id != H5P_DEFAULT && (true != H5P_isa_class(lcpl_id, H5P_LINK_CREATE))) HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not a link creation property list"); @@ -117,30 +133,27 @@ H5Lmove(hid_t src_loc_id, const char *src_name, hid_t dst_loc_id, const char *ds H5CX_set_lcpl(lcpl_id); /* Verify access property list and set up collective metadata if appropriate */ - if (H5CX_set_apl(&lapl_id, H5P_CLS_LACC, ((src_loc_id != H5L_SAME_LOC) ? src_loc_id : dst_loc_id), true) < - 0) + if (H5CX_set_apl(&lapl_id, H5P_CLS_LACC, dst_loc_id, true) < 0) HGOTO_ERROR(H5E_LINK, H5E_CANTSET, FAIL, "can't set access property list info"); /* Set location parameter for source object */ loc_params1.type = H5VL_OBJECT_BY_NAME; loc_params1.loc_data.loc_by_name.name = src_name; loc_params1.loc_data.loc_by_name.lapl_id = lapl_id; - loc_params1.obj_type = H5I_get_type(src_loc_id); + loc_params1.obj_type = src_id_type; /* Set location parameter for destination object */ loc_params2.type = H5VL_OBJECT_BY_NAME; loc_params2.loc_data.loc_by_name.name = dst_name; loc_params2.loc_data.loc_by_name.lapl_id = lapl_id; - loc_params2.obj_type = H5I_get_type(dst_loc_id); + loc_params2.obj_type = dst_id_type; - if (H5L_SAME_LOC != src_loc_id) - /* Get the location object */ - if (NULL == (vol_obj1 = H5VL_vol_object(src_loc_id))) - HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); - if (H5L_SAME_LOC != dst_loc_id) - /* Get the location object */ - if (NULL == (vol_obj2 = H5VL_vol_object(dst_loc_id))) - HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); + /* Get the location object */ + if (NULL == (vol_obj1 = H5VL_vol_object(src_loc_id))) + HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); + /* Get the location object */ + if (NULL == (vol_obj2 = H5VL_vol_object(dst_loc_id))) + HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); /* Make sure that the VOL connectors are the same */ if (vol_obj1 && vol_obj2) { @@ -195,7 +208,8 @@ H5Lcopy(hid_t src_loc_id, const char *src_name, hid_t dst_loc_id, const char *ds H5VL_loc_params_t loc_params1; H5VL_object_t *vol_obj2 = NULL; /* Object of dst_id */ H5VL_loc_params_t loc_params2; - H5VL_object_t tmp_vol_obj; /* Temporary object */ + H5VL_object_t tmp_vol_obj; /* Temporary object */ + H5I_type_t src_id_type = H5I_BADID, dst_id_type = H5I_BADID; herr_t ret_value = SUCCEED; /* Return value */ FUNC_ENTER_API(FAIL) @@ -210,6 +224,20 @@ H5Lcopy(hid_t src_loc_id, const char *src_name, hid_t dst_loc_id, const char *ds if (lcpl_id != H5P_DEFAULT && (true != H5P_isa_class(lcpl_id, H5P_LINK_CREATE))) HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not a link creation property list"); + /* reset an ID in the case of H5L_SAME_LOC */ + if (src_loc_id == H5L_SAME_LOC) + src_loc_id = dst_loc_id; + else if (dst_loc_id == H5L_SAME_LOC) + dst_loc_id = src_loc_id; + + /* verify that src and dst IDs are either a file or a group ID */ + src_id_type = H5I_get_type(src_loc_id); + if (!(H5I_GROUP == src_id_type || H5I_FILE == src_id_type) && src_loc_id != H5L_SAME_LOC) + HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "invalid group (or file) ID, src_loc_id"); + dst_id_type = H5I_get_type(dst_loc_id); + if (!(H5I_GROUP == dst_id_type || H5I_FILE == dst_id_type) && dst_loc_id != H5L_SAME_LOC) + HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "invalid group (or file) ID, dst_loc_id"); + /* Check the link create property list */ if (H5P_DEFAULT == lcpl_id) lcpl_id = H5P_LINK_CREATE_DEFAULT; @@ -226,22 +254,20 @@ H5Lcopy(hid_t src_loc_id, const char *src_name, hid_t dst_loc_id, const char *ds loc_params1.type = H5VL_OBJECT_BY_NAME; loc_params1.loc_data.loc_by_name.name = src_name; loc_params1.loc_data.loc_by_name.lapl_id = lapl_id; - loc_params1.obj_type = H5I_get_type(src_loc_id); + loc_params1.obj_type = src_id_type; /* Set location parameter for destination object */ loc_params2.type = H5VL_OBJECT_BY_NAME; loc_params2.loc_data.loc_by_name.name = dst_name; loc_params2.loc_data.loc_by_name.lapl_id = lapl_id; - loc_params2.obj_type = H5I_get_type(dst_loc_id); + loc_params2.obj_type = dst_id_type; - if (H5L_SAME_LOC != src_loc_id) - /* Get the location object */ - if (NULL == (vol_obj1 = H5VL_vol_object(src_loc_id))) - HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); - if (H5L_SAME_LOC != dst_loc_id) - /* Get the location object */ - if (NULL == (vol_obj2 = H5VL_vol_object(dst_loc_id))) - HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); + /* Get the location object */ + if (NULL == (vol_obj1 = H5VL_vol_object(src_loc_id))) + HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); + /* Get the location object */ + if (NULL == (vol_obj2 = H5VL_vol_object(dst_loc_id))) + HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "invalid location identifier"); /* Make sure that the VOL connectors are the same */ if (vol_obj1 && vol_obj2) { diff --git a/src/H5VLcallback.c b/src/H5VLcallback.c index 58e839c9985..0e696088ebf 100644 --- a/src/H5VLcallback.c +++ b/src/H5VLcallback.c @@ -5100,6 +5100,10 @@ H5VL_link_move(const H5VL_object_t *src_vol_obj, const H5VL_loc_params_t *loc_pa FUNC_ENTER_NOAPI(FAIL) + /* Sanity check */ + assert(src_vol_obj); + assert(src_vol_obj->data); + /* Set wrapper info in API context */ vol_obj = (src_vol_obj->data ? src_vol_obj : dst_vol_obj); if (H5VL_set_vol_wrapper(vol_obj) < 0) diff --git a/test/links.c b/test/links.c index 6612f56e363..ad9948af04d 100644 --- a/test/links.c +++ b/test/links.c @@ -1938,12 +1938,14 @@ test_move_preserves(hid_t fapl_id, bool new_format) *------------------------------------------------------------------------- */ #ifndef H5_NO_DEPRECATED_SYMBOLS +#define NUM_OBJS 3 /* number of groups in FILENAME[0] file */ static int test_deprec(hid_t fapl, bool new_format) { hid_t file_id = H5I_INVALID_HID; hid_t group1_id = H5I_INVALID_HID; hid_t group2_id = H5I_INVALID_HID; + hid_t group3_id = H5I_INVALID_HID; H5G_stat_t sb_hard1, sb_hard2, sb_soft1, sb_soft2; H5G_obj_t obj_type; /* Object type */ hsize_t num_objs; /* Number of objects in a group */ @@ -1967,6 +1969,8 @@ test_deprec(hid_t fapl, bool new_format) FAIL_STACK_ERROR; if ((group2_id = H5Gcreate2(file_id, "group2", H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT)) < 0) FAIL_STACK_ERROR; + if ((group3_id = H5Gcreate2(file_id, "group3", H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT)) < 0) + FAIL_STACK_ERROR; /* Test H5Gset and get comment */ @@ -2022,7 +2026,7 @@ test_deprec(hid_t fapl, bool new_format) /* Test getting the number of objects in a group */ if (H5Gget_num_objs(file_id, &num_objs) < 0) FAIL_STACK_ERROR; - if (num_objs != 2) + if (num_objs != NUM_OBJS) TEST_ERROR; if (H5Gget_num_objs(group1_id, &num_objs) < 0) FAIL_STACK_ERROR; @@ -2113,9 +2117,43 @@ test_deprec(hid_t fapl, bool new_format) /* Test H5Gmove and H5Gmove2 */ if (H5Gmove(file_id, "group1", "moved_group1") < 0) - FAIL_STACK_ERROR; + TEST_ERROR; if (H5Gmove2(file_id, "group2", group1_id, "moved_group2") < 0) - FAIL_STACK_ERROR; + TEST_ERROR; + if (H5Gmove2(file_id, "group3", H5L_SAME_LOC, "moved_group3") < 0) + TEST_ERROR; + if (H5Gmove2(file_id, "moved_group3", group2_id, "moved_group3_to_group2") < 0) + TEST_ERROR; + + /* Test H5Gmove2 with H5L_SAME_LOC */ + if (H5Gmove2(group2_id, "moved_group3_to_group2", H5L_SAME_LOC, "group3_same_loc") < 0) + TEST_ERROR; + + /* Test H5Gmove2 with H5L_SAME_LOC */ + if (H5Gmove2(H5L_SAME_LOC, "moved_group1/moved_group2", file_id, "moved_group2_again") < 0) + TEST_ERROR; + + /* Put back moved_group2 for subsequent tests */ + if (H5Gmove2(file_id, "moved_group2_again", file_id, "moved_group1/moved_group2") < 0) + TEST_ERROR; + + /* Test passing in invalid ID */ + H5E_BEGIN_TRY + { + hid_t bad_id = H5I_BADID; + if (H5Gmove2(bad_id, "group2", group1_id, "moved_group2") >= 0) + TEST_ERROR; + } + H5E_END_TRY + + /* Test passing in invalid ID */ + H5E_BEGIN_TRY + { + hid_t bad_id = H5I_BADID; + if (H5Gmove2(file_id, "group2", bad_id, "moved_group2") >= 0) + TEST_ERROR; + } + H5E_END_TRY /* Ensure that both groups can be opened */ if (H5Gclose(group2_id) < 0) @@ -2129,6 +2167,8 @@ test_deprec(hid_t fapl, bool new_format) FAIL_STACK_ERROR; /* Close open IDs */ + if (H5Gclose(group3_id) < 0) + FAIL_STACK_ERROR; if (H5Gclose(group2_id) < 0) FAIL_STACK_ERROR; if (H5Gclose(group1_id) < 0) @@ -2154,6 +2194,7 @@ test_deprec(hid_t fapl, bool new_format) error: H5E_BEGIN_TRY { + H5Gclose(group3_id); H5Gclose(group2_id); H5Gclose(group1_id); H5Fclose(file_id); @@ -3293,7 +3334,8 @@ external_link_closing_deprec(hid_t fapl, bool new_format) /* Test copy (as of this test, it uses the same code as move) */ if (H5Lcopy(fid1, "elink/elink/elink", fid1, "elink/elink/elink_copied", H5P_DEFAULT, H5P_DEFAULT) < 0) FAIL_STACK_ERROR; - if (H5Lcopy(fid1, "elink/elink/elink", fid1, "elink/elink/elink/elink_copied2", H5P_DEFAULT, + /* Also exercise H5L_SAME_LOC */ + if (H5Lcopy(H5L_SAME_LOC, "elink/elink/elink", fid1, "elink/elink/elink/elink_copied2", H5P_DEFAULT, H5P_DEFAULT) < 0) FAIL_STACK_ERROR; @@ -4325,7 +4367,8 @@ lapl_nlinks_deprec(hid_t fapl, bool new_format) */ if (H5Lcopy(fid, "soft17", fid, "soft17/newer_soft", H5P_DEFAULT, plist) < 0) TEST_ERROR; - if (H5Lmove(fid, "soft17/newer_soft", fid, "soft17/newest_soft", H5P_DEFAULT, plist) < 0) + /* Also exercise H5L_SAME_LOC */ + if (H5Lmove(fid, "soft17/newer_soft", H5L_SAME_LOC, "soft17/newest_soft", H5P_DEFAULT, plist) < 0) TEST_ERROR; /* H5Olink */