Fix for HDFFV-10659: The library abort with "infinite loop closing library"

after deleting attributes in densed storage.
The fix: When deleting attribute nodes from the name index v2 B-tree,
if an attribute is found in the intermediate B-tree nodes, which may be
merged/redistributed in the process, we need to free the dynamically
allocated spaces for the intermediate decoded attribute.

10 files changed, 315 insertions, 20 deletions

diff --git a/MANIFEST b/MANIFEST
index 8783d84..cc2ddc0 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -996,6 +996,7 @@
 ./test/filespace_1_6.h5
 ./test/freespace.c
 ./test/filenotclosed.c
+./test/del_many_dense_attrs.c
 ./test/file_image.c
 ./test/file_image_core_test.h5
 ./test/fill_old.h5
@@ -1103,7 +1104,7 @@
 ./test/testcheck_version.sh.in
 ./test/testerror.sh.in
 ./test/testlinks_env.sh.in
-./test/test_filenotclosed.sh.in
+./test/testabort_fail.sh.in
 ./test/test_filter_plugin.sh.in
 ./test/testflushrefresh.sh.in
 ./test/testframe.c
diff --git a/configure.ac b/configure.ac
index b761042..a583353 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3406,7 +3406,7 @@ AC_CONFIG_FILES([src/libhdf5.settings
                  test/testlinks_env.sh
                  test/testswmr.sh
                  test/testvdsswmr.sh
-                 test/test_filenotclosed.sh
+                 test/testabort_fail.sh
                  test/test_filter_plugin.sh
                  test/test_usecases.sh
                  testpar/Makefile
diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt
index 024db9a..37e863b 100644
--- a/release_docs/RELEASE.txt
+++ b/release_docs/RELEASE.txt
@@ -243,6 +243,19 @@ Bug Fixes since HDF5-1.10.3 release
 
     Library
     -------
+    - Deleting attributes in densed storage
+
+      The library abort with "infinite loop closing library" after
+      attributes in densed storage are created and then deleted.
+
+      When deleting the attribute nodes from the name index v2 B-tree,
+      if an attribute is found in the intermediate B-tree nodes,
+      which may be merged/redistributed in the process, we need to 
+      free the dynamically allocated spaces for the intermediate 
+      decoded attribute.
+
+      (VC - 2018/12/26, HDFFV-10659)
+
     - Allow H5detect and H5make_libsettings to take a file as an argument.
 
       Rather than only writing to stdout, add a command argument to name
diff --git a/src/H5Adense.c b/src/H5Adense.c
index 5bed82d..18cdfe9 100644
--- a/src/H5Adense.c
+++ b/src/H5Adense.c
@@ -300,18 +300,48 @@ static herr_t
 H5A__dense_fnd_cb(const H5A_t *attr, hbool_t *took_ownership, void *_user_attr)
 {
     H5A_t const **user_attr = (H5A_t const **)_user_attr; /* User data from v2 B-tree attribute lookup */
+    herr_t ret_value = SUCCEED;         /* Return value */
 
-    FUNC_ENTER_STATIC_NOERR
+    FUNC_ENTER_STATIC
 
     /* Check arguments */
     HDassert(attr);
     HDassert(user_attr);
+    /*
+     *  If there is an attribute already stored in "user_attr", 
+     *  we need to free the dynamially allocated spaces for the 
+     *  attribute, otherwise we got infinite loop closing library due to 
+     *  outstanding allocation. (HDFFV-10659)
+     *
+     *  This callback is used by H5A__dense_remove() to close/free the
+     *  attribute stored in "user_attr" (via H5O__msg_free_real()) after
+     *  the attribute node is deleted from the name index v2 B-tree.
+     *  The issue is: 
+     *      When deleting the attribute node from the B-tree, 
+     *      if the attribute is found in the intermediate B-tree nodes, 
+     *      which may be merged/redistributed, we need to free the dynamically
+     *      allocated spaces for the intermediate decoded attribute.
+     */
+    if(*user_attr != NULL) {
+        H5A_t *old_attr = *user_attr;
+        if(old_attr->shared) {
+            /* Free any dynamically allocated items */
+            if(H5A__free(old_attr) < 0)
+                HGOTO_ERROR(H5E_ATTR, H5E_CANTRELEASE, FAIL, "can't release attribute info")
+
+            /* Destroy shared attribute struct */
+            old_attr->shared = H5FL_FREE(H5A_shared_t, old_attr->shared);
+        } /* end if */
+
+        old_attr = H5FL_FREE(H5A_t, old_attr);
+     } /* end if */
 
     /* Take over attribute ownership */
     *user_attr = attr;
     *took_ownership = TRUE;
 
-    FUNC_LEAVE_NOAPI(SUCCEED)
+done:
+    FUNC_LEAVE_NOAPI(ret_value)
 } /* end H5A__dense_fnd_cb() */
 
 
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index ed6bacc..a82fe82 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -374,6 +374,7 @@ set (H5_CHECK_TESTS
     atomic_reader
     links_env
     filenotclosed
+    del_many_dense_attrs
     flushrefresh
 )
 
diff --git a/test/CMakeTests.cmake b/test/CMakeTests.cmake
index 26faba6..575eb6d 100644
--- a/test/CMakeTests.cmake
+++ b/test/CMakeTests.cmake
@@ -517,6 +517,7 @@ set (test_CLEANFILES
     flushrefresh_VERIFICATION_CHECKPOINT2
     flushrefresh_VERIFICATION_DONE
     filenotclosed.h5
+    del_many_dense_attrs.h5
     atomic_data
     accum_swmr_big.h5
     ohdr_swmr.h5
@@ -793,6 +794,7 @@ set_tests_properties (H5TEST-tcheck_version-release PROPERTIES
 #    atomic_reader
 #    links_env
 #    filenotclosed
+#    del_many_dense_attrs
 #    flushrefresh
 ##############################################################################
 # autotools script tests
@@ -800,7 +802,7 @@ set_tests_properties (H5TEST-tcheck_version-release PROPERTIES
 # NOT CONVERTED accum_swmr_reader is used by accum.c.
 # NOT CONVERTED atomic_writer and atomic_reader are standalone programs.
 # links_env is used by testlinks_env.sh
-# filenotclosed is used by test_filenotclosed.sh
+# filenotclosed and del_many_dense_attrs are used by testabort_fail.sh
 # NOT CONVERTED flushrefresh is used by testflushrefresh.sh.
 # NOT CONVERTED use_append_chunk, use_append_mchunks and use_disable_mdc_flushes are used by test_usecases.sh
 # NOT CONVERTED swmr_* files (besides swmr.c) are used by testswmr.sh.
@@ -827,6 +829,23 @@ set_tests_properties (H5TEST-filenotclosed PROPERTIES
     WORKING_DIRECTORY ${HDF5_TEST_BINARY_DIR}/H5TEST
 )
 
+#-- Adding test for del_many_dense_attrs
+add_test (
+    NAME H5TEST-clear-del_many_dense_attrs-objects
+    COMMAND    ${CMAKE_COMMAND}
+        -E remove
+        del_many_dense_attrs.h5
+    WORKING_DIRECTORY
+        ${HDF5_TEST_BINARY_DIR}/H5TEST
+)
+set_tests_properties (H5TEST-clear-del_many_dense_attrs-objects PROPERTIES FIXTURES_SETUP  del_many_dense_attrs_clear_objects)
+add_test (NAME H5TEST-del_many_dense_attrs COMMAND $<TARGET_FILE:del_many_dense_attrs>)
+set_tests_properties (H5TEST-del_many_dense_attrs PROPERTIES
+    FIXTURES_REQUIRED del_many_dense_attrs_clear_objects
+    ENVIRONMENT "srcdir=${HDF5_TEST_BINARY_DIR}/H5TEST"
+    WORKING_DIRECTORY ${HDF5_TEST_BINARY_DIR}/H5TEST
+)
+
 #-- Adding test for err_compat
 if (HDF5_ENABLE_DEPRECATED_SYMBOLS)
   add_test (NAME H5TEST-clear-err_compat-objects
diff --git a/test/Makefile.am b/test/Makefile.am
index 1526eca..0bb7410 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -29,12 +29,13 @@ AM_CPPFLAGS+=-I$(top_srcdir)/src -I$(top_builddir)/src
 #       testflushrefresh.sh: flushrefresh
 #       testswmr.sh: swmr*
 #       testvdsswmr.sh: vds_swmr*
-#       test_filenotclosed.sh: filenotclosed.c
+#       testabort_fail.sh: filenotclosed.c and del_many_dense_attrs.c
 #       test_filter_plugin.sh: filter_plugin.c
 #       test_usecases.sh: use_append_chunk, use_append_mchunks, use_disable_mdc_flushes
-TEST_SCRIPT = testerror.sh testlibinfo.sh testcheck_version.sh testlinks_env.sh test_filenotclosed.sh\
-    testswmr.sh testvdsswmr.sh testflushrefresh.sh test_usecases.sh
-SCRIPT_DEPEND = error_test$(EXEEXT) err_compat$(EXEEXT) links_env$(EXEEXT) filenotclosed$(EXEEXT) \
+TEST_SCRIPT = testerror.sh testlibinfo.sh testcheck_version.sh testlinks_env.sh \
+    testswmr.sh testvdsswmr.sh testflushrefresh.sh test_usecases.sh testabort_fail.sh
+SCRIPT_DEPEND = error_test$(EXEEXT) err_compat$(EXEEXT) links_env$(EXEEXT) \
+    filenotclosed$(EXEEXT) del_many_dense_attrs$(EXEEXT) \
     flushrefresh$(EXEEXT) use_append_chunk$(EXEEXT) use_append_mchunks$(EXEEXT) use_disable_mdc_flushes$(EXEEXT) \
     swmr_generator$(EXEEXT) swmr_reader$(EXEEXT) swmr_writer$(EXEEXT) \
     swmr_remove_reader$(EXEEXT) swmr_remove_writer$(EXEEXT) swmr_addrem_writer$(EXEEXT) \
@@ -68,7 +69,7 @@ TEST_PROG= testhdf5 \
 # accum_swmr_reader is used by accum.c.
 # atomic_writer and atomic_reader are standalone programs.
 # links_env is used by testlinks_env.sh
-# filenotclosed is used by test_filenotclosed.sh
+# filenotclosed and del_many_dense_attrs are used by testabort_fail.sh
 # flushrefresh is used by testflushrefresh.sh.
 # use_append_chunk, use_append_mchunks and use_disable_mdc_flushes are used by test_usecases.sh
 # swmr_* files (besides swmr.c) are used by testswmr.sh.
@@ -78,7 +79,8 @@ TEST_PROG= testhdf5 \
 # and this lets automake keep all its test programs in one place.
 check_PROGRAMS=$(TEST_PROG) error_test err_compat tcheck_version \
     testmeta accum_swmr_reader atomic_writer atomic_reader \
-    links_env filenotclosed flushrefresh use_append_chunk use_append_mchunks use_disable_mdc_flushes \
+    links_env filenotclosed del_many_dense_attrs flushrefresh \
+    use_append_chunk use_append_mchunks use_disable_mdc_flushes \
     swmr_generator swmr_start_write swmr_reader swmr_writer swmr_remove_reader \
     swmr_remove_writer swmr_addrem_writer swmr_sparse_reader swmr_sparse_writer \
     swmr_check_compat_vfd vds_swmr_gen vds_swmr_reader vds_swmr_writer
@@ -220,6 +222,6 @@ use_disable_mdc_flushes_SOURCES=use_disable_mdc_flushes.c
 
 # Temporary files.
 DISTCLEANFILES=testerror.sh testlibinfo.sh testcheck_version.sh testlinks_env.sh test_filter_plugin.sh \
-    testswmr.sh testvdsswmr.sh test_usecases.sh testflushrefresh.sh test_filenotclosed.sh
+    testswmr.sh testvdsswmr.sh test_usecases.sh testflushrefresh.sh testabort_fail.sh
 
 include $(top_srcdir)/config/conclude.am
diff --git a/test/ShellTests.cmake b/test/ShellTests.cmake
index 58dc85d..98f3daf 100644
--- a/test/ShellTests.cmake
+++ b/test/ShellTests.cmake
@@ -171,6 +171,7 @@ if (UNIX)
     #    atomic_writer
     #    atomic_reader
     #    filenotclosed
+    #    del_many_dense_attrs
     #    flushrefresh
     ##############################################################################
     # autotools script tests
@@ -178,7 +179,7 @@ if (UNIX)
     # NOT CONVERTED accum_swmr_reader is used by accum.c.
     # NOT CONVERTED atomic_writer and atomic_reader are standalone programs.
     # links_env is used by testlinks_env.sh
-    # filenotclosed is used by test_filenotclosed.sh
+    # filenotclosed and del_many_dense_attrs are used by testabort_fail.sh
     # NOT CONVERTED flushrefresh is used by testflushrefresh.sh.
     # NOT CONVERTED use_append_chunk, use_append_mchunks and use_disable_mdc_flushes are used by test_usecases.sh
     # NOT CONVERTED swmr_* files (besides swmr.c) are used by testswmr.sh.
diff --git a/test/del_many_dense_attrs.c b/test/del_many_dense_attrs.c
new file mode 100644
index 0000000..5ca51d9
--- /dev/null
+++ b/test/del_many_dense_attrs.c
@@ -0,0 +1,203 @@
+/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
+ * Copyright by The HDF Group.                                               *
+ * Copyright by the Board of Trustees of the University of Illinois.         *
+ * All rights reserved.                                                      *
+ *                                                                           *
+ * This file is part of HDF5.  The full HDF5 copyright notice, including     *
+ * terms governing use, modification, and redistribution, is contained in    *
+ * the COPYING file, which can be found at the root of the source code       *
+ * distribution tree, or in https://support.hdfgroup.org/ftp/HDF5/releases.  *
+ * If you do not have access to either file, you may request a copy from     *
+ * help@hdfgroup.org.                                                        *
+ * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
+
+/*
+ * Purpose:	Test to verify that the infinite loop closing library/abort failure
+ *          is fixed when the application creates and removes densed attributes 
+ *          (See HDFFV-10659).
+ */
+
+
+#include "h5test.h"
+
+/* The test file name */
+const char *FILENAME[] = {
+    "del_many_dense_attrs", 
+    NULL
+};
+
+#define ATTR_COUNT  64          /* The number of attributes */
+
+/*-------------------------------------------------------------------------
+ * Function:    catch_signal
+ *
+ * Purpose:     The signal handler to catch the SIGABRT signal.
+ *
+ * Return:      No return
+ *
+ * Programmer:  Vailin Choi
+ *
+ *-------------------------------------------------------------------------
+ */
+static void catch_signal(int H5_ATTR_UNUSED signo)
+{
+    HDexit(1);
+} /* catch_signal() */
+
+
+/*-------------------------------------------------------------------------
+ * Function:	main
+ *
+ * Purpose:	Test to verify that the infinite loop closing library/abort failure
+ *          is fixed when the application creates and removes densed attributes 
+ *          (See HDFFV-10659).
+ *
+ * Return:	Success:	exit(EXIT_SUCCESS)
+ *		    Failure:	exit(EXIT_FAILURE)
+ *
+ * Programmer:  Vailin Choi; Dec 2018
+ *
+ *-------------------------------------------------------------------------
+ */
+int
+main(void)
+{
+    hid_t fid = -1;             /* HDF5 File ID                 */
+    hid_t gid = -1;             /* Group ID                     */
+    hid_t sid = -1;             /* Dataspace ID                 */
+    hid_t aid = -1;             /* Attribute ID                 */
+    hid_t tid = -1;             /* Datatype ID                  */
+    hid_t fapl = -1;            /* File access property lists   */
+    hid_t gcpl = -1;            /* Group creation property list */
+    char aname[50];             /* Name of attribute            */
+    char *basename="attr";      /* Name prefix for attribute    */
+    char filename[100];         /* File name                    */
+    int i;                      /* Local index variable         */
+
+    /* Testing setup */
+    h5_reset();
+
+    /* To exit from the file for SIGABRT signal */
+    if(HDsignal(SIGABRT, catch_signal) == SIG_ERR)
+       TEST_ERROR
+
+    fapl = h5_fileaccess();
+    h5_fixname(FILENAME[0], fapl, filename, sizeof(filename));
+
+    /* Set to latest format */
+    if(H5Pset_libver_bounds(fapl, H5F_LIBVER_LATEST, H5F_LIBVER_LATEST) < 0)
+        TEST_ERROR
+
+    /* Create the file  */
+    if((fid = H5Fcreate(filename, H5F_ACC_TRUNC, H5P_DEFAULT, fapl)) < 0)
+        TEST_ERROR
+
+    /* Close the file */
+    if(H5Fclose(fid) < 0)
+        TEST_ERROR
+
+     /* Re-open the file */
+    if((fid = H5Fopen(filename, H5F_ACC_RDWR, fapl)) < 0)
+        TEST_ERROR
+
+    /* Create the group creation property list */
+    if((gcpl = H5Pcreate(H5P_GROUP_CREATE)) < 0)
+        TEST_ERROR
+
+    /* Set to use dense storage for all attributes on the group */
+    if(H5Pset_attr_phase_change(gcpl, 0, 0) < 0)
+        TEST_ERROR
+
+    /* Create the group in the file */
+    if((gid = H5Gcreate2(fid, "group", H5P_DEFAULT, gcpl, H5P_DEFAULT)) < 0)
+        TEST_ERROR
+
+    /* Create dataspace */
+    if((sid = H5Screate(H5S_SCALAR)) < 0)
+        TEST_ERROR
+
+    /* Get a copy of the datatype */
+    if((tid = H5Tcopy(H5T_NATIVE_FLOAT)) < 0)
+        TEST_ERROR
+
+    /* Create attributes in the group */
+    for(i = ATTR_COUNT; i >= 0; i--) {
+        /* Set up the attribute name */
+        HDsprintf(aname, "%s%d", basename, i);
+
+        /* Create the attribute */
+        if((aid = H5Acreate2(gid, aname, tid, sid, H5P_DEFAULT, H5P_DEFAULT)) < 0)
+            TEST_ERROR
+
+        /* Write to the attribute */
+        if(H5Awrite(aid, tid, &i) < 0)
+            TEST_ERROR
+
+        /* Close the attribute */
+        if(H5Aclose(aid) < 0)
+            TEST_ERROR
+    }
+    
+    /* Close the datatype */
+    if(H5Tclose(tid) < 0)
+       TEST_ERROR
+
+    /* Close the dataspace */
+    if(H5Sclose(sid) < 0)
+        TEST_ERROR
+
+    /* Close the group */
+    if(H5Gclose(gid) < 0)
+        TEST_ERROR
+
+    /* Close the group creation property list */
+    if(H5Pclose(gcpl) < 0)
+        TEST_ERROR
+
+    /* Close the file */
+    if(H5Fclose(fid) < 0)
+        TEST_ERROR
+
+    /* Re-open the file */
+    if((fid = H5Fopen(filename, H5F_ACC_RDWR, fapl)) < 0)
+        TEST_ERROR
+
+    /* Open the group */
+    if((gid = H5Gopen(fid, "group", H5P_DEFAULT)) < 0)
+        TEST_ERROR
+
+    /* Delete the attributes */
+    for (i = 0; i <= ATTR_COUNT; i++) {
+        /* Set up the attribute name */
+        HDsprintf(aname, "%s%d", basename, i);
+
+        /* Delete the attribute */
+        if(H5Adelete(gid, aname) < 0)
+            TEST_ERROR
+    } /* end for */
+
+    /* Close the group */
+    if(H5Gclose(gid) < 0)
+        TEST_ERROR
+
+    /* Close the file */
+    if(H5Fclose(fid) < 0)
+        TEST_ERROR
+
+    h5_cleanup(FILENAME, fapl);
+
+    return(EXIT_SUCCESS);
+
+error:
+    H5E_BEGIN_TRY {
+        H5Gclose(gid);
+        H5Sclose(sid);
+        H5Tclose(tid);
+        H5Aclose(aid);
+        H5Fclose(fid);
+        H5Pclose(gcpl);
+        H5Pclose(fapl);
+    } H5E_END_TRY
+
+    return EXIT_FAILURE;
+}
diff --git a/test/test_filenotclosed.sh.in b/test/testabort_fail.sh.in
index 0b43c5b..90f8077 100644
--- a/test/test_filenotclosed.sh.in
+++ b/test/testabort_fail.sh.in
@@ -13,6 +13,9 @@
 #
 # Test to verify that the assertion/abort failure is fixed when the application
 # does not close the file. (See HDFFV-10160)
+#
+# Test to verify that the infinite loop closing library/abort failure is fixed 
+# when the application creates and removes densed attributes (See HDFFV-10659)
 
 srcdir=@srcdir@
 
@@ -20,22 +23,44 @@ nerrors=0
 
 ##############################################################################
 ##############################################################################
-###			  T H E   T E S T                                              ###
+###			  T H E   T E S T S                                            ###
 ##############################################################################
 ##############################################################################
-
+#
+#
 echo "Testing file not closed assertion/abort failure"
 TEST_NAME=filenotclosed     # The test name
-TEST_BIN=`pwd`/$TEST_NAME 	# The path of the test binary
+TEST_BIN=`pwd`/$TEST_NAME   # The path of the test binary
 #
 # Run the test
-#$RUNSERIAL $TEST_BIN >/dev/null 2>&1 
-$RUNSERIAL $TEST_BIN 2>&1 
+$RUNSERIAL $TEST_BIN >/dev/null 2>&1 
 exitcode=$?
 if [ $exitcode -eq 0 ]; then
     echo "Test PASSED"
 else
+    echo "Test FAILED"
  	nerrors="`expr $nerrors + 1`"
-	echo "***Error encountered***"
 fi
-exit $nerrors
+#
+#
+echo "Testing infinite loop closing library/abort failure"
+TEST_NAME=del_many_dense_attrs  # The test name
+TEST_BIN=`pwd`/$TEST_NAME 	    # The path of the test binary
+# Run the test
+$RUNSERIAL $TEST_BIN >/dev/null 2>&1 
+exitcode=$?
+if [ $exitcode -eq 0 ]; then
+    echo "Test PASSED"
+else
+    echo "Test FAILED"
+ 	nerrors="`expr $nerrors + 1`"
+fi
+#
+#
+if test $nerrors -eq 0 ; then
+    echo "All tests for abort failure passed."
+    exit 0
+else
+    echo "Tests for abort failure failed with $nerrors errors."
+    exit 1
+fi