Skip to content

Commit

Permalink
Detect invalid ID to H5Gmove2 (#4765)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@bmribler
bmribler authored Sep 6, 2024
1 parent 2eaa016 commit a75542b
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 39 deletions.
43 changes: 33 additions & 10 deletions src/H5Gdeprec.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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,
Expand Down
74 changes: 50 additions & 24 deletions src/H5L.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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");

Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand All @@ -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;
Expand All @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions src/H5VLcallback.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
53 changes: 48 additions & 5 deletions test/links.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -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 */

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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);
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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 */
Expand Down

0 comments on commit a75542b

Please sign in to comment.