New 1 10 hdffv 11052 (#2932)

Fix for HDFFV-11052: h5debug fails on a corrupted file (h5_nrefs_POC) producing a core dump.
When h5debug closes the corrupted file, the library calls H5F__dest() which performs all the
closing operations for the file "f" (H5F_t *) but just keeping note of errors in "ret_value"
all the way till the end of the routine.  The user-provided corrupted file has an illegal
file size causing failure when reading the image during the closing process.
At the end of this routine it sets f->shared to NULL and then frees "f".
This is done whether there is error or not in "ret_value".
Due to the failure in reading the file earlier, the routine then returns error.
The error return from H5F__dest() causes the file object "f" not being removed from the
ID node table.  When the library finally exits, it will try to close the
file objects in the table.  This causes assertion failure for f->file_id > 0.
Fix:
a) H5F_dest(): free the f only when there is no error in "ret_value" at the end of the routine.
b) H5F__close_cb(): if f->shared is NULL, free "f"; otherwise, perform closing on "f" as before.
c) h5debug.c main(): track error return from H5Fclose().

6 files changed, 101 insertions, 28 deletions

diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt
index e2b87fc..7a21f8b 100644
--- a/release_docs/RELEASE.txt
+++ b/release_docs/RELEASE.txt
@@ -117,6 +117,25 @@ Bug Fixes since HDF5-1.10.10 release
 ===================================
     Library
     -------
+    - Seg fault on file close
+
+      h5debug fails at file close with core dump on a file that has an
+      illegal file size in its cache image.  In H5F__dest(), the library
+      performs all the closing operations for the file and keeps track of
+      the error encountered when reading the file cache image.  
+      At the end of the routine, it frees the file's file structure and 
+      returns error.  Due to the error return, the file object is not removed 
+      from the ID node table.  This eventually causes assertion failure in 
+      H5F__close_cb() when the library finally exits and tries to 
+      access that file object in the table for closing.
+
+      The closing routine, H5F__dest(), will not free the file structure if
+      there is error, keeping a valid file structure in the ID node table.
+      It will be freed later in H5F__close_cb() when the library exits and 
+      terminates the file package.
+
+      Fix for HDFFV-11052, CVE-2020-10812
+
     - Fixed memory leaks that could occur when reading a dataset from a
       malformed file
 
diff --git a/src/H5Fint.c b/src/H5Fint.c
index e05f64e..405de4a 100644
--- a/src/H5Fint.c
+++ b/src/H5Fint.c
@@ -1466,7 +1466,9 @@ H5F__dest(H5F_t *f, hbool_t flush)
     if (H5FO_top_dest(f) < 0)
         HDONE_ERROR(H5E_FILE, H5E_CANTINIT, FAIL, "problems closing file")
     f->shared = NULL;
-    f         = H5FL_FREE(H5F_t, f);
+
+    if (ret_value >= 0)
+        f = H5FL_FREE(H5F_t, f);
 
     FUNC_LEAVE_NOAPI(ret_value)
 } /* end H5F__dest() */
@@ -2193,35 +2195,43 @@ H5F__close_cb(H5F_t *f)
 
     /* Sanity check */
     HDassert(f);
-    HDassert(f->file_id >
-             0); /* This routine should only be called when a file ID's ref count drops to zero */
+    HDassert(f->shared == NULL ||
+             f->file_id >
+                 0); /* This routine should only be called when a file ID's ref count drops to zero */
 
-    /* Perform checks for "semi" file close degree here, since closing the
-     * file is not allowed if there are objects still open.
-     */
-    if (f->shared->fc_degree == H5F_CLOSE_SEMI) {
-        unsigned nopen_files = 0; /* Number of open files in file/mount hierarchy */
-        unsigned nopen_objs  = 0; /* Number of open objects in file/mount hierarchy */
-
-        /* Get the number of open objects and open files on this file/mount hierarchy */
-        if (H5F__mount_count_ids(f, &nopen_files, &nopen_objs) < 0)
-            HGOTO_ERROR(H5E_SYM, H5E_MOUNT, FAIL, "problem checking mount hierarchy")
-
-        /* If there are no other file IDs open on this file/mount hier., but
-         * there are still open objects, issue an error and bail out now,
-         * without decrementing the file ID's reference count and triggering
-         * a "real" attempt at closing the file.
+    if (f->shared == NULL)
+        f = H5FL_FREE(H5F_t, f);
+
+    else {
+
+        /* Perform checks for "semi" file close degree here, since closing the
+         * file is not allowed if there are objects still open.
          */
-        if (nopen_files == 1 && nopen_objs > 0)
-            HGOTO_ERROR(H5E_FILE, H5E_CANTCLOSEFILE, FAIL, "can't close file, there are objects still open")
-    }
+        if (f->shared->fc_degree == H5F_CLOSE_SEMI) {
+            unsigned nopen_files = 0; /* Number of open files in file/mount hierarchy */
+            unsigned nopen_objs  = 0; /* Number of open objects in file/mount hierarchy */
+
+            /* Get the number of open objects and open files on this file/mount hierarchy */
+            if (H5F__mount_count_ids(f, &nopen_files, &nopen_objs) < 0)
+                HGOTO_ERROR(H5E_SYM, H5E_MOUNT, FAIL, "problem checking mount hierarchy")
+
+            /* If there are no other file IDs open on this file/mount hier., but
+             * there are still open objects, issue an error and bail out now,
+             * without decrementing the file ID's reference count and triggering
+             * a "real" attempt at closing the file.
+             */
+            if (nopen_files == 1 && nopen_objs > 0)
+                HGOTO_ERROR(H5E_FILE, H5E_CANTCLOSEFILE, FAIL,
+                            "can't close file, there are objects still open")
+        }
 
-    /* Reset the file ID for this file */
-    f->file_id = H5I_INVALID_HID;
+        /* Reset the file ID for this file */
+        f->file_id = H5I_INVALID_HID;
 
-    /* Attempt to close the file/mount hierarchy */
-    if (H5F_try_close(f, NULL) < 0)
-        HGOTO_ERROR(H5E_FILE, H5E_CANTCLOSEFILE, FAIL, "can't close file")
+        /* Attempt to close the file/mount hierarchy */
+        if (H5F_try_close(f, NULL) < 0)
+            HGOTO_ERROR(H5E_FILE, H5E_CANTCLOSEFILE, FAIL, "can't close file")
+    }
 
 done:
     FUNC_LEAVE_NOAPI(ret_value)
diff --git a/test/CMakeTests.cmake b/test/CMakeTests.cmake
index bbb9372..af49457 100644
--- a/test/CMakeTests.cmake
+++ b/test/CMakeTests.cmake
@@ -135,6 +135,7 @@ set (HDF5_REFERENCE_TEST_FILES
     btree_idx_1_6.h5
     btree_idx_1_8.h5
     corrupt_stab_msg.h5
+    cve_2020_10812.h5
     deflate.h5
     family_v16_00000.h5
     family_v16_00001.h5
diff --git a/test/cve_2020_10812.h5 b/test/cve_2020_10812.h5
new file mode 100755
index 0000000..a20369d
--- /dev/null
+++ b/test/cve_2020_10812.h5
diff --git a/test/tmisc.c b/test/tmisc.c
index d0a1c63..65f17ad 100644
--- a/test/tmisc.c
+++ b/test/tmisc.c
@@ -329,6 +329,11 @@ typedef struct {
 #define MISC35_SPACE_DIM3 13
 #define MISC35_NPOINTS    10
 
+/* Definitions for misc. test #36 */
+/* The test file is formerly named h5_nrefs_POC.
+   See https://nvd.nist.gov/vuln/detail/CVE-2020-10812 */
+#define CVE_2020_10812_FILENAME "cve_2020_10812.h5"
+
 /****************************************************************
 **
 **  test_misc1(): test unlinking a dataset from a group and immediately
@@ -5816,6 +5821,39 @@ test_misc35(void)
 
 /****************************************************************
 **
+**  test_misc36():
+**      Test for seg fault issue when closing the provided test file
+**      which has an illegal file size in its cache image.
+**      See HDFFV-11052/CVE-2020-10812 for details.
+**
+****************************************************************/
+static void
+test_misc36(void)
+{
+    const char *fname;
+    hid_t       fid;
+    herr_t      ret;
+
+    /* Output message about test being performed */
+    MESSAGE(5, ("Fix for HDFFV-11052/CVE-2020-10812"));
+
+    fname = H5_get_srcdir_filename(CVE_2020_10812_FILENAME);
+    fid   = H5Fopen(fname, H5F_ACC_RDONLY, H5P_DEFAULT);
+    CHECK(fid, FAIL, "H5Fopen");
+
+    /* This should fail due to the illegal file size.
+       It should fail gracefully and not seg fault */
+    H5E_BEGIN_TRY
+    {
+        ret = H5Fclose(fid);
+    }
+    H5E_END_TRY;
+    VERIFY(ret, FAIL, "H5Fclose");
+
+} /* end test_misc36() */
+
+/****************************************************************
+**
 **  test_misc(): Main misc. test routine.
 **
 ****************************************************************/
@@ -5865,6 +5903,7 @@ test_misc(void)
     test_misc33();  /* Test to verify that H5HL_offset_into() returns error if offset exceeds heap block */
     test_misc34();  /* Test behavior of 0 and NULL in H5MM API calls */
     test_misc35();  /* Test behavior of free-list & allocation statistics API calls */
+    test_misc36();  /* Test for seg fault failure at file close */
 
 } /* test_misc() */
 
diff --git a/tools/src/misc/h5debug.c b/tools/src/misc/h5debug.c
index 7706fd4..ad7e1a6 100644
--- a/tools/src/misc/h5debug.c
+++ b/tools/src/misc/h5debug.c
@@ -808,8 +808,12 @@ main(int argc, char *argv[])
 done:
     if (fapl > 0)
         H5Pclose(fapl);
-    if (fid > 0)
-        H5Fclose(fid);
+    if (fid > 0) {
+        if (H5Fclose(fid) < 0) {
+            HDfprintf(stderr, "Error in closing file!\n");
+            exit_value = 1;
+        }
+    }
 
     /* Pop API context */
     if (api_ctx_pushed)