Repository: Unidata/netcdf-c (c)

Issue:
udata.grps in libhdf5/hdf5open.c rec_read_metadata() may be uninitialized during BAIL() exit processing
Version: 4.7.1
Operating System: Windows, Linux, MacOS

The _rec_read_metadata()_ function in libhdf5/hdf5open.c may terminate early on error conditions by invoking the _BAIL()_ macro. _BAIL()_ jumps to the 'exit:' label, where there's an attempt to clean up the child groups as specified by udata.grps. Unfortunately, _BAIL()_ may be invoked prior to udata.grps being initialized (statement 2567). Invoking _nclistlength()_ and _nclistget()_ on an uninitialized udata.grps results in unpredictable behavior (and usually a crash).

There are likely a number of scenarios to reproduce this. The following code that (1) creates an HDF5 file (without H5P_CRT_ORDER_TRACKED), then (2) attempts to reopen it using _nc_open()_ (w/ NC_WRITE) will do the trick ...

```
  hid_t file_hid, fcpl_hid;
  int ncid, retval;
  char *filename = "foo.h5";

/* fails */
  file_hid = H5Fcreate(filename, H5F_ACC_TRUNC, H5P_DEFAULT, H5P_DEFAULT);

/* works */
/*
  fcpl_hid = H5Pcreate(H5P_FILE_CREATE);
  H5Pset_link_creation_order(fcpl_hid, H5P_CRT_ORDER_TRACKED);
  H5Pset_attr_creation_order(fcpl_hid, H5P_CRT_ORDER_TRACKED);
  file_hid = H5Fcreate(filename, H5F_ACC_TRUNC, fcpl_hid, H5P_DEFAULT);
*/

  H5Fclose(file_hid);

  if ((retval = nc_open(filename, NC_WRITE, &ncid)))
    ERR(retval);
  if ((retval = nc_close(ncid)))
    ERR(retval);
```



Pull request:
fix memory issue that may occur for some HDF5 file opens
Fixes #1558 

Fixes a memory problem that can occur on failed HDF5 file opens. The test fails address sanitizer with current master, but passes with fix.

Fix is just to move initialization of udata.grp to top of function, before any BAIL macros are called.

Thanks @brentd42 for finding this one.

Patch:
diff --git a/libhdf5/hdf5open.c b/libhdf5/hdf5open.c
index 38362d0c7..5d45ef1eb 100644
--- a/libhdf5/hdf5open.c
+++ b/libhdf5/hdf5open.c
@@ -2532,6 +2532,10 @@ rec_read_metadata(NC_GRP_INFO_T *grp)
     /* Get HDF5-specific group info. */
     hdf5_grp = (NC_HDF5_GRP_INFO_T *)grp->format_grp_info;
 
+    /* Set user data for iteration over any child groups. */
+    udata.grp = grp;
+    udata.grps = nclistnew();
+
     /* Open this HDF5 group and retain its grpid. It will remain open
      * with HDF5 until this file is nc_closed. */
     if (!hdf5_grp->hdf_grpid)
@@ -2579,10 +2583,6 @@ rec_read_metadata(NC_GRP_INFO_T *grp)
         iter_index = H5_INDEX_NAME;
     }
 
-    /* Set user data for iteration over any child groups. */
-    udata.grp = grp;
-    udata.grps = nclistnew();
-
     /* Iterate over links in this group, building lists for the types,
      * datasets and groups encountered. A pointer to udata will be
      * passed as a parameter to the callback function


Test patch:
diff --git a/nc_test4/tst_interops5.c b/nc_test4/tst_interops5.c
index 3faf379ad..cb4dd0e62 100644
--- a/nc_test4/tst_interops5.c
+++ b/nc_test4/tst_interops5.c
@@ -1,8 +1,10 @@
-/* This is part of the netCDF package. Copyright 2005-2018, University
-   Corporation for Atmospheric Research/Unidata.  See COPYRIGHT file
+/* This is part of the netCDF package. Copyright 2005-2019, University
+   Corporation for Atmospheric Research/Unidata. See COPYRIGHT file
    for conditions of use.
 
    Test that HDF5 and NetCDF-4 can read and write the same file.
+
+   Ed Hartnett
 */
 #include <config.h>
 #include <nc_tests.h>
@@ -253,5 +255,24 @@ main(int argc, char **argv)
    }
    SUMMARIZE_ERR;
 #endif /* USE_SZIP */
+   /* This test suggested by user brentd42 to find a memory problem in
+    * function rec_read_metadata(). This test demonstrates the bug on
+    * address sanitizer runs. See
+    * https://github.com/Unidata/netcdf-c/issues/1558. */
+   printf("*** testing error when opening HDF5 file without creating ordering...");
+   {
+       hid_t file_hid;
+       int ncid;
+       char *filename = "tst_interops5.h5";
+
+       /* Create a HDF5 file, but don't set creation ordering on. */
+       file_hid = H5Fcreate(filename, H5F_ACC_TRUNC, H5P_DEFAULT, H5P_DEFAULT);
+       H5Fclose(file_hid);
+
+       /* Open the file with netCDF. */
+       nc_set_log_level(3);
+       if (nc_open(filename, NC_WRITE, &ncid) != NC_ECANTWRITE) ERR;
+   }
+   SUMMARIZE_ERR;
    FINAL_RESULTS;
 }