From d242fd85c4d280a85522bf6ad31e2998d10e1e08 Mon Sep 17 00:00:00 2001 From: Raymond Lu Date: Fri, 1 Nov 2013 14:52:54 -0500 Subject: [svn-r24391] Issue 8528 - H5F_get_objects overfilled the list for object IDs. I merged the fix the trunk. I put the safeguard in both H5F_get_objects and H5F_get_objects_cb to prevent overfill the list. I also added test cases in tfile.c. Tested with h5committest on jam, koala, ostrich, and platypus. --- src/H5F.c | 139 +++++++++++++++++++++++++++++++---------------------------- test/tfile.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 203 insertions(+), 66 deletions(-) diff --git a/src/H5F.c b/src/H5F.c index 8f71a84..bf79153 100644 --- a/src/H5F.c +++ b/src/H5F.c @@ -77,7 +77,7 @@ typedef struct H5F_olist_t { } ptr; } file_info; size_t list_index; /* Current index in open ID array */ - size_t max_index; /* Maximum # of IDs to put into array */ + size_t max_nobjs; /* Maximum # of IDs to put into array */ } H5F_olist_t; @@ -520,7 +520,8 @@ H5Fget_obj_ids(hid_t file_id, unsigned types, size_t max_objs, hid_t *oid_list) HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "not a file id") if(0 == (types & H5F_OBJ_ALL)) HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "not an object type") - HDassert(oid_list); + if(!oid_list) + HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "object ID list is NULL") /* Perform the query */ if(H5F_get_obj_ids(f, types, max_objs, oid_list, TRUE, &obj_id_count) < 0) @@ -579,7 +580,7 @@ done: *--------------------------------------------------------------------------- */ static herr_t -H5F_get_objects(const H5F_t *f, unsigned types, size_t max_index, hid_t *obj_id_list, hbool_t app_ref, size_t *obj_id_count_ptr) +H5F_get_objects(const H5F_t *f, unsigned types, size_t max_nobjs, hid_t *obj_id_list, hbool_t app_ref, size_t *obj_id_count_ptr) { size_t obj_id_count=0; /* Number of open IDs */ H5F_olist_t olist; /* Structure to hold search results */ @@ -591,10 +592,10 @@ H5F_get_objects(const H5F_t *f, unsigned types, size_t max_index, hid_t *obj_id_ HDassert(obj_id_count_ptr); /* Set up search information */ - olist.obj_id_list = (max_index==0 ? NULL : obj_id_list); + olist.obj_id_list = (max_nobjs==0 ? NULL : obj_id_list); olist.obj_id_count = &obj_id_count; olist.list_index = 0; - olist.max_index = max_index; + olist.max_nobjs = max_nobjs; /* Determine if we are searching for local or global objects */ if(types & H5F_OBJ_LOCAL) { @@ -614,38 +615,54 @@ H5F_get_objects(const H5F_t *f, unsigned types, size_t max_index, hid_t *obj_id_ HGOTO_ERROR(H5E_FILE, H5E_BADITER, FAIL, "iteration failed(1)") } /* end if */ - /* Search through dataset IDs to count number of datasets, and put their + /* If the caller just wants to count the number of objects (OLIST.MAX_NOBJS is zero), + * or the caller wants to get the list of IDs and the list isn't full, + * search through dataset IDs to count number of datasets, and put their * IDs on the object list */ - if(types & H5F_OBJ_DATASET) { - olist.obj_type = H5I_DATASET; - if(H5I_iterate(H5I_DATASET, H5F_get_objects_cb, &olist, app_ref) < 0) - HGOTO_ERROR(H5E_FILE, H5E_BADITER, FAIL, "iteration failed(2)") - } /* end if */ + if(!olist.max_nobjs || (olist.max_nobjs && olist.list_indexfile_info.ptr.file || (olist->file_info.ptr.file && (H5F_t*)obj_ptr == olist->file_info.ptr.file) )) || (!olist->file_info.local && ( !olist->file_info.ptr.shared || (olist->file_info.ptr.shared && ((H5F_t*)obj_ptr)->shared == olist->file_info.ptr.shared) ))) { - /* Add the object's ID to the ID list, if appropriate */ - if(olist->obj_id_list) { - olist->obj_id_list[olist->list_index] = obj_id; - olist->list_index++; - } - - /* Increment the number of open objects */ - if(olist->obj_id_count) - (*olist->obj_id_count)++; - - /* Check if we've filled up the array. Return TRUE only if - * we have filled up the array. Otherwise return FALSE(RET_VALUE is - * preset to FALSE) because H5I_iterate needs the return value of - * FALSE to continue the iteration. */ - if(olist->max_index>0 && olist->list_index>=olist->max_index) - HGOTO_DONE(TRUE) /* Indicate that the iterator should stop */ + add_obj = TRUE; } } /* end if */ else { /* either count opened object IDs or put the IDs on the list */ @@ -740,7 +743,7 @@ H5F_get_objects_cb(void *obj_ptr, hid_t obj_id, void *key) case H5I_ERROR_STACK: case H5I_NTYPES: default: - HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "unknown data object") + HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, H5_ITER_ERROR, "unknown data object") } /* end switch */ if((olist->file_info.local && @@ -751,25 +754,29 @@ H5F_get_objects_cb(void *obj_ptr, hid_t obj_id, void *key) ((!olist->file_info.ptr.shared && olist->obj_type == H5I_DATATYPE && H5T_is_immutable((H5T_t *)obj_ptr) == FALSE) || (!olist->file_info.ptr.shared && olist->obj_type != H5I_DATATYPE) || (oloc && oloc->file && oloc->file->shared == olist->file_info.ptr.shared)))) { - /* Add the object's ID to the ID list, if appropriate */ - if(olist->obj_id_list) { - olist->obj_id_list[olist->list_index] = obj_id; - olist->list_index++; - } /* end if */ - - /* Increment the number of open objects */ - if(olist->obj_id_count) - (*olist->obj_id_count)++; - - /* Check if we've filled up the array. Return TRUE only if - * we have filled up the array. Otherwise return FALSE(RET_VALUE is - * preset to FALSE) because H5I_iterate needs the return value of - * FALSE to continue iterating. */ - if(olist->max_index>0 && olist->list_index>=olist->max_index) - HGOTO_DONE(TRUE) /* Indicate that the iterator should stop */ + add_obj = TRUE; } /* end if */ } /* end else */ + if(TRUE==add_obj) { + /* Add the object's ID to the ID list, if appropriate */ + if(olist->obj_id_list) { + olist->obj_id_list[olist->list_index] = obj_id; + olist->list_index++; + } /* end if */ + + /* Increment the number of open objects */ + if(olist->obj_id_count) + (*olist->obj_id_count)++; + + /* Check if we've filled up the array. Return H5_ITER_STOP only if + * we have filled up the array. Otherwise return H5_ITER_CONT(RET_VALUE is + * preset to H5_ITER_CONT) because H5I_iterate needs the return value of + * H5_ITER_CONT to continue the iteration. */ + if(olist->max_nobjs>0 && olist->list_index>=olist->max_nobjs) + HGOTO_DONE(H5_ITER_STOP) /* Indicate that the iterator should stop */ + } + done: FUNC_LEAVE_NOAPI(ret_value) } /* end H5F_get_objects_cb() */ diff --git a/test/tfile.c b/test/tfile.c index 7a52e2c..012de71 100644 --- a/test/tfile.c +++ b/test/tfile.c @@ -102,6 +102,10 @@ /* Declaration for test_libver_macros2() */ #define FILE5 "tfile5.h5" /* Test file */ +/* Declaration for test_get_obj_ids() */ +#define FILE6 "tfile6.h5" /* Test file */ +#define NGROUPS 2 +#define NDSETS 4 static void create_objects(hid_t, hid_t, hid_t *, hid_t *, hid_t *, hid_t *); @@ -924,6 +928,131 @@ create_objects(hid_t fid1, hid_t fid2, hid_t *ret_did, hid_t *ret_gid1, /**************************************************************** ** +** test_get_obj_ids(): Test the bug and the fix for Jira 8528. +** H5Fget_obj_ids overfilled the list of +** object IDs by one. This is an enhancement +** for test_obj_count_and_id(). +** +****************************************************************/ +static void +test_get_obj_ids(void) +{ + hid_t fid, gid[NGROUPS], dset[NDSETS]; + hid_t filespace; + hsize_t file_dims[F2_RANK] = {F2_DIM0, F2_DIM1}; + ssize_t oid_count, ret_count; + hid_t *oid_list = NULL; + herr_t ret; + int i, m, n; + ssize_t oid_list_size = NDSETS; + char gname[64], dname[64]; + + /* Create a new file */ + fid = H5Fcreate(FILE6, H5F_ACC_TRUNC, H5P_DEFAULT, H5P_DEFAULT); + CHECK(fid, FAIL, "H5Fcreate"); + + filespace = H5Screate_simple(F2_RANK, file_dims, NULL); + CHECK(filespace, FAIL, "H5Screate_simple"); + + /* creates NGROUPS groups under the root group */ + for(m = 0; m < NGROUPS; m++) { + sprintf(gname, "group%d", m); + gid[m] = H5Gcreate2(fid, gname, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT); + CHECK(gid[m], FAIL, "H5Gcreate2"); + } + + /* create NDSETS datasets under the root group */ + for(n = 0; n < NDSETS; n++) { + sprintf(dname, "dataset%d", n); + dset[n] = H5Dcreate2(fid, dname, H5T_NATIVE_INT, filespace, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT); + CHECK(dset[m], FAIL, "H5Dcreate2"); + } + + /* The number of opened objects should be NGROUPS + NDSETS + 1. One is opened file. */ + oid_count = H5Fget_obj_count(fid, H5F_OBJ_ALL); + CHECK(oid_count, FAIL, "H5Fget_obj_count"); + VERIFY(oid_count, (NGROUPS + NDSETS + 1), "H5Fget_obj_count"); + + oid_list = (hid_t *)HDcalloc((size_t)oid_list_size, sizeof(hid_t)); + CHECK(oid_list, NULL, "HDcalloc"); + + /* Call the public function H5F_get_obj_ids to use H5F_get_objects. User reported having problem here. + * that the returned size (ret_count) from H5Fget_obj_ids is one greater than the size passed in + * (oid_list_size) */ + ret_count = H5Fget_obj_ids(fid, H5F_OBJ_ALL, (size_t)oid_list_size, oid_list); + CHECK(ret_count, FAIL, "H5Fget_obj_ids"); + VERIFY(ret_count, oid_list_size, "H5Fget_obj_count"); + + /* Close all object IDs on the list except the file ID. The first ID is supposed to be file ID according + * to the library design */ + for(i = 0; i< ret_count; i++) { + if(fid != oid_list[i]) { + ret = H5Oclose(oid_list[i]); + CHECK(ret, FAIL, "H5Oclose"); + } + } + + /* The number of opened objects should be NGROUPS + 1 + 1. The first one is opened file. The second one + * is the dataset ID left open from the previous around of H5Fget_obj_ids */ + oid_count = H5Fget_obj_count(fid, H5F_OBJ_ALL); + CHECK(oid_count, FAIL, "H5Fget_obj_count"); + VERIFY(oid_count, NGROUPS + 2, "H5Fget_obj_count"); + + /* Get the IDs of the left opend objects */ + ret_count = H5Fget_obj_ids(fid, H5F_OBJ_ALL, (size_t)oid_list_size, oid_list); + CHECK(ret_count, FAIL, "H5Fget_obj_ids"); + VERIFY(ret_count, oid_list_size, "H5Fget_obj_count"); + + /* Close all object IDs on the list except the file ID. The first ID is still the file ID */ + for(i = 0; i< ret_count; i++) { + if(fid != oid_list[i]) { + ret = H5Oclose(oid_list[i]); + CHECK(ret, FAIL, "H5Oclose"); + } + } + + H5Sclose(filespace); + H5Fclose(fid); + + HDfree(oid_list); + + /* Reopen the file to check whether H5Fget_obj_count and H5Fget_obj_ids still works + * when the file is closed first */ + fid = H5Fopen(FILE6, H5F_ACC_RDONLY, H5P_DEFAULT); + CHECK(fid, FAIL, "H5Fopen"); + + /* Open NDSETS datasets under the root group */ + for(n = 0; n < NDSETS; n++) { + sprintf(dname, "dataset%d", n); + dset[n] = H5Dopen2(fid, dname, H5P_DEFAULT); + CHECK(dset[n], FAIL, "H5Dcreate2"); + } + + /* Close the file first */ + H5Fclose(fid); + + /* Get the number of all opened objects */ + oid_count = H5Fget_obj_count(H5F_OBJ_ALL, H5F_OBJ_ALL); + CHECK(oid_count, FAIL, "H5Fget_obj_count"); + VERIFY(oid_count, NDSETS, "H5Fget_obj_count"); + + oid_list = (hid_t *)HDcalloc((size_t)oid_count, sizeof(hid_t)); + CHECK(oid_list, NULL, "HDcalloc"); + + /* Get the list of all opened objects */ + ret_count = H5Fget_obj_ids(H5F_OBJ_ALL, H5F_OBJ_ALL, (size_t)oid_count, oid_list); + CHECK(ret_count, FAIL, "H5Fget_obj_ids"); + VERIFY(ret_count, NDSETS, "H5Fget_obj_count"); + + /* Close all open objects with H5Oclose */ + for(n = 0; n < oid_count; n++) + H5Oclose(oid_list[n]); + + HDfree(oid_list); +} + +/**************************************************************** +** ** test_get_file_id(): Test H5Iget_file_id() ** *****************************************************************/ @@ -2832,6 +2961,7 @@ test_file(void) test_file_close(); /* Test file close behavior */ #endif /* H5_NO_SHARED_WRITING */ test_get_file_id(); /* Test H5Iget_file_id */ + test_get_obj_ids(); /* Test H5Fget_obj_ids for Jira Issue 8528 */ test_file_perm(); /* Test file access permissions */ test_file_perm2(); /* Test file access permission again */ test_file_freespace(); /* Test file free space information */ -- cgit v0.12