[svn-r27415] Fix potential error with H5I_clear_type which could occur when a callback closed

a different ID in the same type.  Added a new skiplist routine,
H5SL_try_free_safe, which iterates over items, freeing some of them, and which
intercepts and defers attempts to remove from the list outside of the main
iteration.  Changed H5I_clear_type to use this function.

Tested: jam, koala, ostrich (h5committest); ummon

2 files changed, 402 insertions, 0 deletions

diff --git a/test/tid.c b/test/tid.c
index 85d26c9..68cebf3 100644
--- a/test/tid.c
+++ b/test/tid.c
@@ -533,12 +533,211 @@ out:
 	return -1;
 }
 
+    /* Test removing ids in callback for H5Iclear_type */
+
+/* There was a rare bug where, if an id free callback being called by
+ * H5I_clear_type() removed another id in that type, a segfault could occur.
+ * This test tests for that error (and freeing ids "out of order" within
+ * H5Iclear_type() in general). */
+/* Macro definitions */
+#define TEST_RCT_MAX_NOBJS 25
+#define TEST_RCT_MIN_NOBJS 5
+#define TEST_RCT_NITER 50
+
+/* Structure to hold the list of objects */
+typedef struct {
+    struct test_rct_obj_t *list; /* List of objects */
+    long nobjs;                 /* Number of objects in list */
+    long nobjs_rem;             /* Number of objects in list that have not been freed */
+} test_rct_list_t;
+
+/* Structure for an object */
+typedef struct test_rct_obj_t {
+    hid_t id;                   /* ID for this object */
+    int nfrees;                 /* Number of times this object has been freed */
+    hbool_t freeing;            /* Whether we are currently freeing this object directly (through H5Idec_ref()) */
+    test_rct_list_t *obj_list;  /* List of all objects */
+} test_rct_obj_t;
+
+/* Free callback */
+static herr_t test_rct_free(void *_obj) {
+    test_rct_obj_t *obj = (test_rct_obj_t *)_obj;
+    long rem_idx, i;
+    herr_t  ret;        /* return value */
+
+    /* Mark this object as freed */
+    obj->nfrees++;
+    obj->obj_list->nobjs_rem--;
+
+    /* Check freeing and nobjs_rem */
+    if(!obj->freeing && (obj->obj_list->nobjs_rem > 0)) {
+        /* Remove a random object from the list */
+        rem_idx = HDrandom() % obj->obj_list->nobjs_rem;
+
+        /* Scan the list, finding the rem_idx'th object that has not been
+         * freed */
+        for(i = 0; i < obj->obj_list->nobjs; i++)
+            if(obj->obj_list->list[i].nfrees == 0) {
+                if(rem_idx == 0)
+                    break;
+                else
+                    rem_idx--;
+            } /* end if */
+        if(i == obj->obj_list->nobjs) {
+            ERROR("invalid obj_list");
+            goto out;
+        } /* end if */
+        else {
+            /* Remove the object.  Mark as "freeing" so its own callback does
+             * not free another object. */
+            obj->obj_list->list[i].freeing = TRUE;
+            ret = H5Idec_ref(obj->obj_list->list[i].id);
+            CHECK(ret, FAIL, "H5Idec_ref");
+            if(ret == FAIL)
+                goto out;
+            obj->obj_list->list[i].freeing = FALSE;
+        } /* end else */
+    } /* end if */
+
+    /* Verify nobjs_rem is non-negative */
+    if(obj->obj_list->nobjs_rem < 0) {
+        ERROR("invalid nobjs_rem");
+        goto out;
+    } /* end if */
+
+    return 0;
+
+out:
+    return -1;
+} /* end test_rct_free() */
+
+/* Test function */
+static int test_remove_clear_type(void)
+{
+    H5I_type_t obj_type;
+    test_rct_list_t obj_list;
+    test_rct_obj_t list[TEST_RCT_MAX_NOBJS];
+    long i, j;
+    long nobjs_found;
+    hsize_t nmembers;
+    herr_t  ret;        /* return value */
+
+    /* Register type */
+    obj_type = H5Iregister_type((size_t)8, 0, test_rct_free);
+    CHECK(obj_type, H5I_BADID, "H5Iregister_type");
+    if(obj_type == H5I_BADID)
+        goto out;
+
+    /* Init obj_list.list */
+    obj_list.list = list;
+
+    for(i = 0; i < TEST_RCT_NITER; i++) {
+        /* Build object list */
+        obj_list.nobjs = obj_list.nobjs_rem = TEST_RCT_MIN_NOBJS + (HDrandom() % (long)(TEST_RCT_MAX_NOBJS - TEST_RCT_MIN_NOBJS + 1));
+        for(j = 0; j < obj_list.nobjs; j++) {
+            list[j].nfrees = 0;
+            list[j].freeing = FALSE;
+            list[j].obj_list = &obj_list;
+            list[j].id = H5Iregister(obj_type, &list[j]);
+            CHECK(list[j].id, FAIL, "H5Iregister");
+            if(list[j].id == FAIL)
+                goto out;
+            if(HDrandom() % 2) {
+                ret = H5Iinc_ref(list[j].id);
+                CHECK(ret, FAIL, "H5Iinc_ref");
+                if(ret == FAIL)
+                    goto out;
+            } /* end if */
+        } /* end for */
+
+        /* Clear the type */
+        ret = H5Iclear_type(obj_type, FALSE);
+        CHECK(ret, FAIL, "H5Iclear_type");
+        if(ret == FAIL)
+            goto out;
+
+        /* Verify list */
+        nobjs_found = 0;
+        for(j = 0; j < obj_list.nobjs; j++) {
+            if(list[j].nfrees == 0)
+                nobjs_found++;
+            else {
+                VERIFY(list[j].nfrees, (long)1, "list[j].nfrees");
+                if(list[j].nfrees != (long)1)
+                    goto out;
+            } /* end else */
+            VERIFY(list[j].freeing, FALSE, "list[j].freeing");
+            if(list[j].freeing != FALSE)
+                goto out;
+        } /* end for */
+
+        /* Verify number of objects */
+        VERIFY(obj_list.nobjs_rem, nobjs_found, "obj_list.nobjs_rem");
+        if(obj_list.nobjs_rem != nobjs_found)
+            goto out;
+        ret = H5Inmembers(obj_type, &nmembers);
+        CHECK(ret, FAIL, "H5Inmembers");
+        if(ret == FAIL)
+            goto out;
+        VERIFY(nmembers, (size_t)nobjs_found, "H5Inmembers");
+        if(nmembers != (size_t)nobjs_found)
+            goto out;
+
+        /* Clear the type with force set to TRUE */
+        ret = H5Iclear_type(obj_type, TRUE);
+        CHECK(ret, FAIL, "H5Iclear_type");
+        if(ret == FAIL)
+            goto out;
+
+        /* Verify list */
+        for(j = 0; j < obj_list.nobjs; j++) {
+            VERIFY(list[j].nfrees, (long)1, "list[j].nfrees");
+            if(list[j].nfrees != (long)1)
+                goto out;
+            VERIFY(list[j].freeing, FALSE, "list[j].freeing");
+            if(list[j].freeing != FALSE)
+                goto out;
+        } /* end for */
+
+        /* Verify number of objects is 0 */
+        VERIFY(obj_list.nobjs_rem, (long)0, "obj_list.nobjs_rem");
+        if(obj_list.nobjs_rem != (long)0)
+            goto out;
+        ret = H5Inmembers(obj_type, &nmembers);
+        CHECK(ret, FAIL, "H5Inmembers");
+        if(ret == FAIL)
+            goto out;
+        VERIFY(nmembers, (size_t)0, "H5Inmembers");
+        if(nmembers != (size_t)0)
+            goto out;
+    } /* end for */
+
+    /* Destroy type */
+    ret = H5Idestroy_type(obj_type);
+    CHECK(ret, FAIL, "H5Idestroy_type");
+    if(ret == FAIL)
+        goto out;
+
+    return 0;
+
+out:
+    /* Cleanup.  For simplicity, just destroy the types and ignore errors. */
+    H5E_BEGIN_TRY
+        H5Idestroy_type(obj_type);
+    H5E_END_TRY
+    return -1;
+} /* end test_remove_clear_type() */
+
 void test_ids(void)
 {
+    /* Set the random # seed */
+    HDsrandom((unsigned)HDtime(NULL));
+
 	if (basic_id_test() < 0) TestErrPrintf("Basic ID test failed\n");
 	if (id_predefined_test() < 0) TestErrPrintf("Predefined ID type test failed\n");
 	if (test_is_valid() < 0) TestErrPrintf("H5Iis_valid test failed\n");
 	if (test_get_type() < 0) TestErrPrintf("H5Iget_type test failed\n");
 	if (test_id_type_list() < 0) TestErrPrintf("ID type list test failed\n");
+	if (test_remove_clear_type() < 0) TestErrPrintf("ID remove during H5Iclear_type test failed\n");
 
 }
diff --git a/test/tskiplist.c b/test/tskiplist.c
index 07e63fd..62c5f8e 100644
--- a/test/tskiplist.c
+++ b/test/tskiplist.c
@@ -1171,6 +1171,208 @@ test_skiplist_free(void)
 
 /****************************************************************
 **
+**  test_skiplist_try_free_safe(): Test H5SL (skip list) code.
+**      Tests 'try_free_safe' operation in skip lists.
+**
+****************************************************************/
+/* Macro definitions */
+#define TEST_TFS_MAX_NOBJS 100
+#define TEST_TFS_MIN_NOBJS 5
+#define TEST_TFS_NITER 50
+
+/* Structure to hold the list of objects */
+typedef struct {
+    H5SL_t *slist;              /* Skiplist holding the objects */
+    struct test_tfs_obj_t *list; /* Linear list of objects */
+    int nobjs;                  /* Number of objects in list */
+    int nobjs_rem;              /* Number of objects in list that have not been freed */
+} test_tfs_list_t;
+
+/* Structure for an object */
+typedef struct test_tfs_obj_t {
+    int idx;                    /* Index (key) for this object */
+    int nfrees;                 /* Number of times this object has been freed */
+} test_tfs_obj_t;
+
+/* op_data struct for H5SL_iterate() */
+typedef struct test_tfs_it_ud_t {
+    test_tfs_list_t *obj_list;  /* List of objects */
+    int last_idx;               /* Index of last object visited in iteration */
+    int ncalls;                 /* Number of times this callback was called */
+} test_tfs_it_ud_t;
+
+/* iterate callback */
+static herr_t test_tfs_iter(void *_obj, void *key, void *_udata) {
+    test_tfs_obj_t *obj = (test_tfs_obj_t *)_obj;
+    test_tfs_it_ud_t *udata = (test_tfs_it_ud_t *)_udata;
+
+    /* Check consistency */
+    VERIFY((void *)&obj->idx, key, "obj->idx");
+    VERIFY(obj, &udata->obj_list->list[obj->idx], "obj_list->list[obj->idx]");
+
+    /* Increment number of calls */
+    udata->ncalls++;
+
+    /* Verify we were given the correct object */
+    do {
+        udata->last_idx++;
+    } while(udata->obj_list->list[udata->last_idx].nfrees != 0);
+    VERIFY(udata->last_idx, obj->idx, "H5SL_iterate");
+
+    return 0;
+} /* end test_tfs_iter() */
+
+/* try_free_safe callback */
+static htri_t test_tfs_free(void *_obj, void *key, void *_obj_list) {
+    test_tfs_obj_t *obj = (test_tfs_obj_t *)_obj;
+    test_tfs_list_t *obj_list = (test_tfs_list_t *)_obj_list;
+    test_tfs_it_ud_t iter_ud;
+    int nrems, rem_idx, i, j;
+    test_tfs_obj_t *obj_ret;
+    herr_t  ret;        /* return value */
+    htri_t ret_value;
+
+    /* Check consistency */
+    VERIFY((void *)&obj->idx, key, "obj->idx");
+    VERIFY(obj, &obj_list->list[obj->idx], "obj_list->list[obj->idx]");
+
+    /* Mark this object as freed (to make sure it isn't recursively freed, that
+     * is not something we support, we will undo this if we decide later not to
+     * free the object) */
+    obj->nfrees++;
+    obj_list->nobjs_rem--;
+
+    /* Decide how many objects to remove */
+    nrems = (int)(HDrandom() % (long)3);
+
+    /* Remove objects */
+    for(i = 0; i < nrems; i++)
+        /* Check nobjs_rem */
+        if(obj_list->nobjs_rem > 0) {
+            /* Remove a random object from the list */
+            rem_idx = (int)(HDrandom() % (long)obj_list->nobjs_rem);
+
+            /* Scan the list, finding the rem_idx'th object that has not been
+             * freed */
+            for(j = 0; j < obj_list->nobjs; j++)
+                if(obj_list->list[j].nfrees == 0) {
+                    if(rem_idx == 0)
+                        break;
+                    else
+                        rem_idx--;
+                } /* end if */
+            if(j == obj_list->nobjs)
+                ERROR("invalid obj_list");
+            else {
+                /* Remove the object */
+                obj_ret = (test_tfs_obj_t *)H5SL_remove(obj_list->slist, &j);
+                CHECK(obj_ret, NULL, "H5SL_remove");
+                obj_ret->nfrees++;
+                obj_list->nobjs_rem--;
+            } /* end else */
+        } /* end if */
+
+    /* Mark this object as not freed so we know to expect it in the iterate call
+     */
+    obj->nfrees--;
+    obj_list->nobjs_rem++;
+
+    /* Iterate over skip list (maybe) */
+    if(HDrandom() % (long)5) {
+        iter_ud.obj_list = obj_list;
+        iter_ud.last_idx = -1;
+        iter_ud.ncalls = 0;
+        ret = H5SL_iterate(obj_list->slist, test_tfs_iter, &iter_ud);
+        CHECK(ret, FAIL, "H5SL_iterate");
+        VERIFY(iter_ud.ncalls, obj_list->nobjs_rem, "H5SL_iterate");
+    } /* end if */
+
+    /* Verify nobjs_rem is non-negative */
+    if(obj_list->nobjs_rem < 0)
+        ERROR("invalid nobjs_rem");
+
+    /* Decide whether this object should be freed */
+    if(HDrandom() % (long)2) {
+        /* Free the object */
+        ret_value = TRUE;
+        obj->nfrees++;
+        obj_list->nobjs_rem--;
+    } /* end if */
+    else
+        /* Do not free the object */
+        ret_value = FALSE;
+
+    return ret_value;
+} /* end test_tfs_free() */
+
+/* Test function */
+static void
+test_skiplist_try_free_safe(void)
+{
+    test_tfs_list_t obj_list;
+    test_tfs_obj_t list[TEST_TFS_MAX_NOBJS];
+    int i, j;
+    int nobjs_found;
+    hsize_t count;
+    herr_t ret;         /* Generic return value */
+
+    /* Output message about test being performed */
+    MESSAGE(7, ("Testing Skip List 'Try Free Safe' Operation\n"));
+
+    /* Create a skip list */
+    obj_list.slist = H5SL_create(H5SL_TYPE_INT, NULL);
+    CHECK(obj_list.slist, NULL, "H5SL_create");
+
+    /* Init obj_list.list */
+    obj_list.list = list;
+    for(j = 0; j < TEST_TFS_MAX_NOBJS; j++)
+        list[j].idx = j;
+
+    for(i = 0; i < TEST_TFS_NITER; i++) {
+        /* Build object list */
+        obj_list.nobjs = obj_list.nobjs_rem = (int)(TEST_TFS_MIN_NOBJS + (HDrandom() % (long)(TEST_TFS_MAX_NOBJS - TEST_TFS_MIN_NOBJS + 1)));
+        for(j = 0; j < obj_list.nobjs; j++) {
+            list[j].nfrees = 0;
+            ret = H5SL_insert(obj_list.slist, &list[j], &list[j].idx);
+            CHECK(ret, FAIL, "H5SL_insert");
+        } /* end for */
+
+        /* Call H5S_try_free_safe() - free most of the items in the skip list in
+         * a safe manner */
+        ret = H5SL_try_free_safe(obj_list.slist, test_tfs_free, &obj_list);
+        CHECK(ret, FAIL, "H5SL_try_free_safe");
+
+        /* Verify list */
+        nobjs_found = 0;
+        for(j = 0; j < obj_list.nobjs; j++)
+            if(list[j].nfrees == 0)
+                nobjs_found++;
+            else
+                VERIFY(list[j].nfrees, (long)1, "list[j].nfrees");
+
+        /* Verify number of objects */
+        VERIFY(obj_list.nobjs_rem, nobjs_found, "obj_list.nobjs_rem");
+        count = H5SL_count(obj_list.slist);
+        VERIFY(count, (size_t)nobjs_found, "H5SL_count");
+
+        /* Release the skip list, forcibly freeing all nodes (will not make
+         * callbacks) */
+        ret = H5SL_release(obj_list.slist);
+        CHECK(ret, FAIL, "H5SL_release");
+
+        /* Verify number of objects is 0 */
+        count = H5SL_count(obj_list.slist);
+        VERIFY(count, (size_t)0, "H5SL_count");
+    } /* end for */
+
+    /* Close the skip list */
+    ret = H5SL_close(obj_list.slist);
+    CHECK(ret, FAIL, "H5SL_close");
+
+} /* end test_skiplist_try_free_safe() */
+
+/****************************************************************
+**
 **  test_skiplist_less(): Test H5SL (skip list) code.
 **      Tests 'less' operation in skip lists.
 **
@@ -1577,6 +1779,7 @@ test_skiplist(void)
     test_skiplist_add();        /* Test 'add' operation */
     test_skiplist_destroy();    /* Test 'destroy' operation */
     test_skiplist_free();       /* Test 'free' operation */
+    test_skiplist_try_free_safe(); /* Test 'try_free_safe' operation */
     test_skiplist_less();       /* Test 'less' operation */
     test_skiplist_greater();    /* Test 'greater' operation */
     test_skiplist_below();      /* Test 'below' operation */