Fix for a segfault when H5Pset_fapl_log is passed an invalid fapl ID (#607)

* Committing clang-format changes

* Fixes an issue where H5Pset_fapl_log sefaults when passed an invalid
fapl ID

This was due to a pointer-containing struct being memset after the first
internal API call. If the first call failed, the error condition would
check if the pointer was not NULL and then attempt to free it if not.
This would lead to the freeing of a wild pointer if an invalid fapl ID
were passed in.

This was fixed by reordering the memset and adding a test to ensure the
problem stays fixed.

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

3 files changed, 30 insertions, 3 deletions

diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt
index 359f222..f014d72 100644
--- a/release_docs/RELEASE.txt
+++ b/release_docs/RELEASE.txt
@@ -414,6 +414,19 @@ New Features
 
     Library:
     --------
+    - H5Pset_fapl_log() no longer crashes when passed an invalid fapl ID
+
+      When passed an invalid fapl ID, H5Pset_fapl_log() would usually
+      segfault when attempting to free an uninitialized pointer in the error
+      handling code. This behavior is more common in release builds or
+      when the memory sanitization checks were not selected as a build
+      option.
+
+      The pointer is now correctly initialized and the API call now
+      produces a normal HDF5 error when fed an invalid fapl ID.
+
+        (DER - 2021/04/28, HDFFV-11240)
+
     - HSYS_GOTO_ERROR now emits the results of GetLastError() on Windows
 
       HSYS_GOTO_ERROR is an internal macro that is used to produce error
diff --git a/src/H5FDlog.c b/src/H5FDlog.c
index 57902b6..707c97b 100644
--- a/src/H5FDlog.c
+++ b/src/H5FDlog.c
@@ -328,12 +328,15 @@ H5Pset_fapl_log(hid_t fapl_id, const char *logfile, unsigned long long flags, si
     FUNC_ENTER_API(FAIL)
     H5TRACE4("e", "i*sULz", fapl_id, logfile, flags, buf_size);
 
+    /* Do this first, so that we don't try to free a wild pointer if
+     * H5P_object_verify() fails.
+     */
+    HDmemset(&fa, 0, sizeof(H5FD_log_fapl_t));
+
     /* Check arguments */
     if (NULL == (plist = H5P_object_verify(fapl_id, H5P_FILE_ACCESS)))
         HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not a file access property list")
 
-    HDmemset(&fa, 0, sizeof(H5FD_log_fapl_t));
-
     /* Duplicate the log file string
      * A little wasteful, since this string will just be copied later, but
      * passing it in as a pointer sets off a chain of impossible-to-resolve
diff --git a/test/vfd.c b/test/vfd.c
index 6b851a2..8484bfd 100644
--- a/test/vfd.c
+++ b/test/vfd.c
@@ -1867,12 +1867,23 @@ test_log(void)
     hsize_t       file_size = 0;
     unsigned int  flags     = H5FD_LOG_ALL;
     size_t        buf_size  = 4 * KB;
+    herr_t        ret       = SUCCEED;
 
     TESTING("LOG file driver");
 
-    /* Set property list and file name for log driver. */
     if ((fapl = H5Pcreate(H5P_FILE_ACCESS)) < 0)
         TEST_ERROR;
+
+    /* Make sure calling with an invalid fapl doesn't crash */
+    H5E_BEGIN_TRY
+    {
+        ret = H5Pset_fapl_log(H5I_INVALID_HID, LOG_FILENAME, 0, 0);
+    }
+    H5E_END_TRY;
+    if (SUCCEED == ret)
+        TEST_ERROR;
+
+    /* Set property list and file name for log driver. */
     if (H5Pset_fapl_log(fapl, LOG_FILENAME, flags, buf_size) < 0)
         TEST_ERROR;
     h5_fixname(FILENAME[6], fapl, filename, sizeof filename);