Repository: Unidata/netcdf-c (c)

Issue:
segfault while reading virtual datasets
I am trying to create a netCDF4 compatible dataset which is composed of several different sources. My issue is that the source files may not be changed and are too large to afford making copies of the data. As a consequence, I decided to create new datasets using the HDF5 external and virtual datasets API such that most of my original data can stay in place while creating a new, more user friendly and higher level view to the original data. The first step was to convert a bunch of non-netcdf binary files to a single "virtual" netcdf file using external storage of HDF5, which works great. In the second step, which in my case is changing some coordinate variables and replacing broken data with new data, I tried to use the virtual dataset feature.

This leads to a segmentation fault when opening the datasets using netcdf4 (i.e. `ncdump -h`). The segfault is in [`nc4_adjust_var_cache`](https://github.com/Unidata/netcdf-c/blob/cee0a9332d72df6fa29d44b6aca6a9b348b3737f/libhdf5/nc4hdf.c#L1067) and I assume that this is related to [`get_chunking_info`](https://github.com/Unidata/netcdf-c/blob/cee0a9332d72df6fa29d44b6aca6a9b348b3737f/libhdf5/hdf5open.c#L1114), which misses a case for `H5D_VIRTUAL` which may be returned by the [`H5Pget_layout`](https://support.hdfgroup.org/HDF5/doc/RM/RM_H5P.html#Property-GetLayout) function. I've not yet tested it further but my guess is that `var->storage` in this case defaults to 0 which is `NC_CHUNKED` but `var->chunksizes` is not allocated, but later read by `nc4_adjust_var_cache`.

Notably, opening the dataset using [`h5netcdf`](https://github.com/shoyer/h5netcdf) works as expected.

I tested this on Ubuntu 18.04.4 with netcdf version 4.6.0. But as the missing case is still present in master, I assume the error will show up there as well.


Pull request description:
Support for reading HDF5 virtual datasets
* Fixes #1799 

This PR is an attempt in solving  #1799. HDF5 recently introduced the possibility to create virtual datasets. These kinds of datasets are especially useful if new datasets should be derived from other datasets (e.g. processing measurement data to higher levels). With virtual datasets some variables can be reused from existing files. This can significantly reduce the need for storage space while improving the user experience for dataset consumers.

It is not yet possible to create virtual datasets using netCDF4. However, in principle it should be possible to create netCDF4-compliant HDF5 files which use this feature (see added test case). In fact, the python library h5netcdf is already able to read such specially crafted HDF5-files, so netcdf-c should be able to do the same.

Up to my current knowledge, the compatibility issues with netCDF and HDF in this regard are twofold. First, [get_chunking_info](https://github.com/Unidata/netcdf-c/blob/cee0a9332d72df6fa29d44b6aca6a9b348b3737f/libhdf5/hdf5open.c#L1114) has no catch-all `else` case which could cover any new storage mechanism which has been or will be invented by HDF5 in a default way. Second, the [`SEMI`](https://github.com/Unidata/netcdf-c/blob/cee0a9332d72df6fa29d44b6aca6a9b348b3737f/libhdf5/hdf5open.c#L764) fclose-degree does not play well with the current way of opening virtual datasets in HDF5. In particular, the HDF5 library [opens a name from another file](https://bitbucket.hdfgroup.org/projects/HDFFV/repos/hdf5/browse/src/H5Dvirtual.c?at=refs%2Ftags%2Fhdf5-1_12_0#941) without closing it before the [file is closed again](https://bitbucket.hdfgroup.org/projects/HDFFV/repos/hdf5/browse/src/H5Dvirtual.c?at=refs%2Ftags%2Fhdf5-1_12_0#966). In this case, the reference count to the file is not 0 and if the fclose-degree is set to `SEMI`, the library will complain.

I am not sure if this is an elegant or complete way of solving the issue, but maybe it is a good start for further discussion.

Patch:
diff --git a/include/netcdf.h b/include/netcdf.h
index 4395b30e8..99bc0c219 100644
--- a/include/netcdf.h
+++ b/include/netcdf.h
@@ -293,11 +293,14 @@ NOTE: The NC_MAX_DIMS, NC_MAX_ATTRS, and NC_MAX_VARS limits
 
 /** In HDF5 files you can set storage for each variable to be either
  * contiguous or chunked, with nc_def_var_chunking().  This define is
- * used there. */
+ * used there. Unknown storage is used for further extensions of HDF5
+ * storage models, which should be handled transparently by netcdf */
 /**@{*/
-#define NC_CHUNKED    0
-#define NC_CONTIGUOUS 1
-#define NC_COMPACT    2
+#define NC_CHUNKED         0
+#define NC_CONTIGUOUS      1
+#define NC_COMPACT         2
+#define NC_UNKNOWN_STORAGE 3
+#define NC_VIRTUAL         4
 /**@}*/
 
 /** In HDF5 files you can set check-summing for each variable.
diff --git a/libhdf5/hdf5create.c b/libhdf5/hdf5create.c
index 27a99405c..747ee8acc 100644
--- a/libhdf5/hdf5create.c
+++ b/libhdf5/hdf5create.c
@@ -121,11 +121,12 @@ nc4_create_file(const char *path, int cmode, size_t initialsz,
     }
 
     /* Need this access plist to control how HDF5 handles open objects
-     * on file close. Setting H5F_CLOSE_SEMI will cause H5Fclose to
-     * fail if there are any open objects in the file. */
+     * on file close. (Setting H5F_CLOSE_WEAK will cause H5Fclose not to
+     * fail if there are any open objects in the file. This may happen when virtual
+     * datasets are opened). */
     if ((fapl_id = H5Pcreate(H5P_FILE_ACCESS)) < 0)
         BAIL(NC_EHDFERR);
-    if (H5Pset_fclose_degree(fapl_id, H5F_CLOSE_SEMI))
+    if (H5Pset_fclose_degree(fapl_id, H5F_CLOSE_WEAK))
         BAIL(NC_EHDFERR);
 
 #ifdef USE_PARALLEL4
diff --git a/libhdf5/hdf5open.c b/libhdf5/hdf5open.c
index 06b5adbae..c642a1c24 100644
--- a/libhdf5/hdf5open.c
+++ b/libhdf5/hdf5open.c
@@ -756,12 +756,13 @@ nc4_open_file(const char *path, int mode, void* parameters, int ncid)
 #endif /* !USE_PARALLEL4 */
 
     /* Need this access plist to control how HDF5 handles open objects
-     * on file close. (Setting H5F_CLOSE_SEMI will cause H5Fclose to
-     * fail if there are any open objects in the file). */
+     * on file close. (Setting H5F_CLOSE_WEAK will cause H5Fclose not to
+     * fail if there are any open objects in the file. This may happen when virtual
+     * datasets are opened). */
     if ((fapl_id = H5Pcreate(H5P_FILE_ACCESS)) < 0)
         BAIL(NC_EHDFERR);
 
-    if (H5Pset_fclose_degree(fapl_id, H5F_CLOSE_SEMI) < 0)
+    if (H5Pset_fclose_degree(fapl_id, H5F_CLOSE_WEAK) < 0)
         BAIL(NC_EHDFERR);
 
 #ifdef USE_PARALLEL4
@@ -1140,6 +1141,16 @@ get_chunking_info(hid_t propid, NC_VAR_INFO_T *var)
     {
 	var->storage = NC_COMPACT;
     }
+#ifdef H5D_VIRTUAL
+    else if (layout == H5D_VIRTUAL)
+    {
+	var->storage = NC_VIRTUAL;
+    }
+#endif
+    else
+    {
+    var->storage = NC_UNKNOWN_STORAGE;
+    }
 
     return NC_NOERR;
 }
diff --git a/libsrc4/nc4internal.c b/libsrc4/nc4internal.c
index 9c9d7ec95..968b2ab08 100644
--- a/libsrc4/nc4internal.c
+++ b/libsrc4/nc4internal.c
@@ -1709,8 +1709,12 @@ rec_print_metadata(NC_GRP_INFO_T *grp, int tab_count)
             strcat(storage_str, "contiguous");
         else if (var->storage == NC_COMPACT)
             strcat(storage_str, "compact");
-        else
+        else if (var->storage == NC_CHUNKED)
             strcat(storage_str, "chunked");
+        else if (var->storage == NC_VIRTUAL)
+            strcat(storage_str, "virtual");
+        else
+            strcat(storage_str, "unknown");
         LOG((2, "%s VARIABLE - varid: %d name: %s ndims: %d "
              "dimids:%s storage: %s", tabs, var->hdr.id, var->hdr.name,
              var->ndims,
diff --git a/nc_perf/tst_attsperf.c b/nc_perf/tst_attsperf.c
index 98f233f72..26a2b4844 100644
--- a/nc_perf/tst_attsperf.c
+++ b/nc_perf/tst_attsperf.c
@@ -137,7 +137,7 @@ readfile_hdf5(char *file_name, long long *delta, int do_inq, int num_vars)
 
    /* Open and close the root group. */
    if ((fapl_id = H5Pcreate(H5P_FILE_ACCESS)) < 0) ERR;
-   if (H5Pset_fclose_degree(fapl_id, H5F_CLOSE_SEMI)) ERR;
+   if (H5Pset_fclose_degree(fapl_id, H5F_CLOSE_WEAK)) ERR;
    if ((hdfid = H5Fopen(file_name, H5F_ACC_RDONLY, fapl_id)) < 0) ERR;
    if ((hdf_grpid = H5Gopen2(hdfid, "/", H5P_DEFAULT)) < 0) ERR;
 
diff --git a/ncdump/ncdump.c b/ncdump/ncdump.c
index e7ac9acf8..4b2231e3c 100644
--- a/ncdump/ncdump.c
+++ b/ncdump/ncdump.c
@@ -988,7 +988,7 @@ pr_att_specials(
     } else if(contig == NC_COMPACT) {
 	    pr_att_name(ncid, varp->name, NC_ATT_STORAGE);
 	    printf(" = \"compact\" ;\n");
-    } else {
+    } else if(contig == NC_CHUNKED) {
  	   size_t *chunkp;
 	   int i;
 	    pr_att_name(ncid, varp->name, NC_ATT_STORAGE);
@@ -1002,6 +1002,12 @@ pr_att_specials(
 		printf("%lu%s", (unsigned long)chunkp[i], i+1 < varp->ndims ? ", " : " ;\n");
 	    }
 	    free(chunkp);
+    } else if(contig == NC_VIRTUAL) {
+	    pr_att_name(ncid, varp->name, NC_ATT_STORAGE);
+	    printf(" = \"virtual\" ;\n");
+    } else {
+	    pr_att_name(ncid, varp->name, NC_ATT_STORAGE);
+	    printf(" = \"unknown\" ;\n");
     }
 
     /* _Filter (including deflate and shuffle) */


Test patch:
diff --git a/nc_test4/CMakeLists.txt b/nc_test4/CMakeLists.txt
index 980dab7fa..0da0cfb1b 100644
--- a/nc_test4/CMakeLists.txt
+++ b/nc_test4/CMakeLists.txt
@@ -46,6 +46,10 @@ ENDIF(ENABLE_FILTER_TESTING)
 
 ENDIF(BUILD_UTILITIES)
 
+IF(${HDF5_VERSION} VERSION_GREATER "1.10.0")
+  SET(NC4_TESTS ${NC4_TESTS} tst_virtual_datasets)
+ENDIF(${HDF5_VERSION} VERSION_GREATER "1.10.0")
+
 ##
 # The shell script, run_empty_vlen_test.sh,
 # depends on the 'tst_empty_vlen_unlim' binary.
diff --git a/nc_test4/tst_h_vl2.c b/nc_test4/tst_h_vl2.c
index 8f44e9a0f..1f2b5cd3e 100644
--- a/nc_test4/tst_h_vl2.c
+++ b/nc_test4/tst_h_vl2.c
@@ -36,7 +36,7 @@ main()
 
       /* Open the file and read the vlen data. */
       if ((fapl_id = H5Pcreate(H5P_FILE_ACCESS)) < 0) ERR;
-      if (H5Pset_fclose_degree(fapl_id, H5F_CLOSE_SEMI)) ERR;
+      if (H5Pset_fclose_degree(fapl_id, H5F_CLOSE_WEAK)) ERR;
       if (H5Pset_libver_bounds(fapl_id, H5F_LIBVER_LATEST, H5F_LIBVER_LATEST) < 0) ERR;
 
       if ((fileid = H5Fopen(FILE_NAME, H5F_ACC_RDONLY, fapl_id)) < 0) ERR;
diff --git a/nc_test4/tst_interops.c b/nc_test4/tst_interops.c
index 64a031947..74353d34b 100644
--- a/nc_test4/tst_interops.c
+++ b/nc_test4/tst_interops.c
@@ -390,7 +390,7 @@ main(int argc, char **argv)
       /* Open the file with HDF5 while netcdf still has it open. */
       if ((fapl_id = H5Pcreate(H5P_FILE_ACCESS)) < 0) ERR;
       /* Turn this off for*/
-      if (H5Pset_fclose_degree(fapl_id, H5F_CLOSE_SEMI)) ERR;
+      if (H5Pset_fclose_degree(fapl_id, H5F_CLOSE_WEAK)) ERR;
       if ((fileid = H5Fopen(FILE_NAME, H5F_ACC_RDONLY, fapl_id)) < 0) ERR;
       if (H5Pclose(fapl_id) < 0) ERR;
       if (H5Fclose(fileid) < 0) ERR;
diff --git a/nc_test4/tst_interops5.c b/nc_test4/tst_interops5.c
index cb4dd0e62..bb22fd0a2 100644
--- a/nc_test4/tst_interops5.c
+++ b/nc_test4/tst_interops5.c
@@ -50,10 +50,10 @@ main(int argc, char **argv)
       yscaleDims[0] = ncolCur;
       if ((xdimSpaceId = H5Screate_simple(1, xscaleDims, NULL)) < 0) ERR;
 
-      /* With the SEMI close degree, the HDF5 file close will fail if
+      /* With the WEAK close degree, the HDF5 file close will not fail if
        * anything is left open. */
       if ((fapl = H5Pcreate(H5P_FILE_ACCESS)) < 0) ERR;
-      if (H5Pset_fclose_degree(fapl, H5F_CLOSE_SEMI)) ERR;
+      if (H5Pset_fclose_degree(fapl, H5F_CLOSE_WEAK)) ERR;
 
       /* Create file */
       if((fileId = H5Fcreate(FILE_NAME, H5F_ACC_TRUNC,
diff --git a/nc_test4/tst_virtual_datasets.c b/nc_test4/tst_virtual_datasets.c
new file mode 100644
index 000000000..c27455713
--- /dev/null
+++ b/nc_test4/tst_virtual_datasets.c
@@ -0,0 +1,87 @@
+#include <hdf5.h>
+#include <hdf5_hl.h>
+#include <nc_tests.h>
+#include "err_macros.h"
+
+#define FILE_NAME_A "tst_virtual_a.nc"
+#define FILE_NAME_B "tst_virtual_b.nc"
+#define VARIABLE_SIZE 5
+#define VARIABLE_NAME "v"
+
+static void
+create_dataset_a() {
+    hsize_t dims[1] = {VARIABLE_SIZE};
+    float data[VARIABLE_SIZE];
+    for(size_t i = 0; i < VARIABLE_SIZE; ++i) {
+        data[i] = (float)i;
+    }
+
+    hid_t file = H5Fcreate(FILE_NAME_A, H5F_ACC_TRUNC, H5P_DEFAULT, H5P_DEFAULT);
+    hid_t space = H5Screate_simple(1, dims, NULL);
+    hid_t dset = H5Dcreate2(file, VARIABLE_NAME, H5T_IEEE_F32LE, space, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT);
+
+    H5Dwrite(dset, H5T_IEEE_F32LE, H5S_ALL, H5S_ALL, H5P_DEFAULT, data);
+    H5DSset_scale(dset, VARIABLE_NAME);
+
+    H5Dclose(dset);
+    H5Sclose(space);
+    H5Fclose(file);
+}
+
+static void
+create_dataset_b() {
+    hsize_t dims[1] = {VARIABLE_SIZE};
+
+    hid_t file = H5Fcreate(FILE_NAME_B, H5F_ACC_TRUNC, H5P_DEFAULT, H5P_DEFAULT);
+    hid_t src_space = H5Screate_simple(1, dims, NULL);
+    hid_t space = H5Screate_simple(1, dims, NULL);
+
+    hid_t dcpl = H5Pcreate(H5P_DATASET_CREATE);
+    H5Pset_virtual(dcpl, space, FILE_NAME_A, VARIABLE_NAME, src_space);
+    hid_t dset = H5Dcreate2(file, VARIABLE_NAME, H5T_IEEE_F32LE, space, H5P_DEFAULT, dcpl, H5P_DEFAULT);
+
+    H5DSset_scale(dset, VARIABLE_NAME);
+
+    H5Dclose(dset);
+    H5Pclose(dcpl);
+    H5Sclose(space);
+    H5Sclose(src_space);
+    H5Fclose(file);
+}
+
+int read_back_contents(const char* filename) {
+    int ncid, varid, ndims;
+    float data[VARIABLE_SIZE];
+
+    char var_name[NC_MAX_NAME+1];
+    int dimids_var[1], var_type, natts;
+
+    printf("\treading back %s.\n", filename);
+
+    if (nc_open(filename, 0, &ncid)) ERR;
+    if (nc_inq_varid(ncid, VARIABLE_NAME, &varid)) ERR;
+    if (nc_inq_var(ncid, varid, var_name, &var_type, &ndims, dimids_var, &natts)) ERR;
+    if (nc_get_var_float(ncid, varid, data)) ERR;
+    printf("\t %s: ", var_name);
+    for (size_t i = 0; i < VARIABLE_SIZE; ++i) {
+        printf("%f, ", data[i]);
+    }
+    printf("\n");
+    if (nc_close(ncid)) ERR;
+    SUMMARIZE_ERR;
+    return 0;
+}
+
+int
+main(int argc, char **argv)
+{
+
+    printf("\n*** Testing reading of virtual datasets.\n");
+
+    create_dataset_a();
+    create_dataset_b();
+
+    int status;
+    if((status = read_back_contents(FILE_NAME_A))) return status;
+    if((status = read_back_contents(FILE_NAME_B))) return status;
+}