Fix v1 object header gap bug in H5Ocopy (#2785) (#2833)

3 files changed, 83 insertions, 2 deletions

diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt
index a5eb585..1e0af89 100644
--- a/release_docs/RELEASE.txt
+++ b/release_docs/RELEASE.txt
@@ -205,6 +205,18 @@ Bug Fixes since HDF5-1.12.2 release
 ===================================
     Library
     -------
+    - Fixed a bug in H5Ocopy that could generate invalid HDF5 files
+
+      H5Ocopy was missing a check to determine whether the new object's
+      object header version is greater than version 1. Without this check,
+      copying of objects with object headers that are smaller than a
+      certain size would cause H5Ocopy to create an object header for the
+      new object that has a gap in the header data. According to the
+      HDF5 File Format Specification, this is not allowed for version
+      1 of the object header format.
+
+      Fixes GitHub issue #2653
+
     - Fixed potential heap buffer overflow in decoding of link info message
 
       Detections of buffer overflow were added for decoding version, index
diff --git a/src/H5Ocopy.c b/src/H5Ocopy.c
index abd1f0b..90c7ed8 100644
--- a/src/H5Ocopy.c
+++ b/src/H5Ocopy.c
@@ -664,10 +664,15 @@ H5O__copy_header_real(const H5O_loc_t *oloc_src, H5O_loc_t *oloc_dst /*out*/, H5
         HDassert((oh_dst->flags & H5O_HDR_CHUNK0_SIZE) == H5O_HDR_CHUNK0_1);
 
         /* Determine whether to create gap or NULL message */
-        if (delta < H5O_SIZEOF_MSGHDR_OH(oh_dst))
+        if ((oh_dst->version > H5O_VERSION_1) && (delta < H5O_SIZEOF_MSGHDR_OH(oh_dst)))
             dst_oh_gap = delta;
-        else
+        else {
+            /* NULL message must be at least size of message header */
+            if (delta < H5O_SIZEOF_MSGHDR_OH(oh_dst))
+                delta = H5O_SIZEOF_MSGHDR_OH(oh_dst);
+
             dst_oh_null = delta;
+        }
 
         /* Increase destination object header size */
         dst_oh_size += delta;
diff --git a/test/objcopy.c b/test/objcopy.c
index 03e5b68..5a5b8f9 100644
--- a/test/objcopy.c
+++ b/test/objcopy.c
@@ -16391,6 +16391,68 @@ error:
     return (H5_ITER_ERROR);
 } /* end test_copy_iterate_cb */
 
+/*
+ * Test for a bug with copying of v1 object headers where the
+ * new object header would end up with a gap in the header data,
+ * which v1 object header shouldn't have.
+ */
+static int
+test_copy_cdt_v1_header_bug(hid_t fcpl_src, hid_t src_fapl)
+{
+    hid_t file_id   = H5I_INVALID_HID;
+    hid_t type_id   = H5I_INVALID_HID;
+    hid_t ocpypl_id = H5I_INVALID_HID;
+    char  src_filename[NAME_BUF_SIZE];
+
+    TESTING("H5Ocopy(): bug with copying v1 object headers");
+
+    /* Initialize the filenames */
+    h5_fixname(FILENAME[0], src_fapl, src_filename, sizeof src_filename);
+
+    if ((file_id = H5Fcreate(src_filename, H5F_ACC_TRUNC, fcpl_src, src_fapl)) < 0)
+        TEST_ERROR;
+
+    if ((type_id = H5Tcreate(H5T_STRING, 385)) < 0)
+        TEST_ERROR;
+    if (H5Tset_strpad(type_id, H5T_STR_NULLPAD) < 0)
+        TEST_ERROR;
+    if (H5Tset_cset(type_id, H5T_CSET_ASCII) < 0)
+        TEST_ERROR;
+
+    if (H5Tcommit2(file_id, "committed_str_type", type_id, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT) < 0)
+        TEST_ERROR;
+
+    if ((ocpypl_id = H5Pcreate(H5P_OBJECT_COPY)) < 0)
+        TEST_ERROR;
+    if (H5Pset_copy_object(ocpypl_id, H5O_COPY_WITHOUT_ATTR_FLAG) < 0)
+        TEST_ERROR;
+
+    if (H5Ocopy(file_id, "committed_str_type", file_id, "committed_str_type2", ocpypl_id, H5P_DEFAULT) < 0)
+        TEST_ERROR;
+
+    if (H5Tclose(type_id) < 0)
+        TEST_ERROR;
+    if (H5Pclose(ocpypl_id) < 0)
+        TEST_ERROR;
+    if (H5Fclose(file_id) < 0)
+        TEST_ERROR;
+
+    PASSED();
+
+    return 0;
+
+error:
+    H5E_BEGIN_TRY
+    {
+        H5Tclose(type_id);
+        H5Pclose(ocpypl_id);
+        H5Fclose(file_id);
+    }
+    H5E_END_TRY;
+
+    return 1;
+}
+
 static int
 test_copy_iterate(hid_t fcpl_src, hid_t fcpl_dst, hid_t src_fapl, hid_t dst_fapl)
 {
@@ -17555,6 +17617,8 @@ main(void)
             nerrors += test_copy_null_ref(fcpl_src, fcpl_dst, src_fapl, dst_fapl);
             nerrors += test_copy_null_ref_open(fcpl_src, fcpl_dst, src_fapl, dst_fapl);
 
+            nerrors += test_copy_cdt_v1_header_bug(fcpl_src, src_fapl);
+
             nerrors += test_copy_iterate(fcpl_src, fcpl_dst, src_fapl, dst_fapl);
         } /* end if */