Repository: Unidata/netcdf-c
Pull request #1442 (2019-07-17 23:32:11)

NC4_get_vars(): fix out-of-bounds write with unlimited dimension
This fixes an issue hit by GDAL, and that is found in netcdf 4.6.3
and 4.7.0

git bisect pointed the problem to have started with

```
77ab979c5fd7151f8583c7698498bb8cb0dafb63 is the first bad commit
commit 77ab979c5fd7151f8583c7698498bb8cb0dafb63
Author: Ed Hartnett <edwardjameshartnett@gmail.com>
Date:   Sat Jun 16 09:58:48 2018 -0600

    using get_vars but not put_vars

:040000 040000 8611e77aaea4f130ca769a852e2c3b26373a78fa fc9ffd1d13d8f5db5ab888044e0cec4def26eba1 M	libsrc4
```

where nc_get_vara_double() started using nc4_get_vars() underneath.

It turns out that nc4_get_vars() was buggy in the situation exercised by GDAL.

This can be reproduced with the following simple test case:

```

int main()
{
    int status;
    int cdfid = -1;
    int first_dim;
    int varid;
    int other_var;
    size_t anStart[NC_MAX_DIMS];
    size_t anCount[NC_MAX_DIMS];
    double* val = (double*)calloc(3, sizeof(double));

    status = nc_create("foo.nc", NC_NETCDF4, &cdfid);
    assert( status == NC_NOERR );

    status = nc_def_dim(cdfid, "unlimited_dim", NC_UNLIMITED, &first_dim);
    assert( status == NC_NOERR );

    status = nc_def_var(cdfid, "my_var", NC_DOUBLE, 1, &first_dim, &varid);
    assert( status == NC_NOERR );

    status = nc_def_var(cdfid, "other_var", NC_DOUBLE, 1, &first_dim, &other_var);
    assert( status == NC_NOERR );

    status = nc_enddef(cdfid);
    assert( status == NC_NOERR );

    /* Write 3 elements to set the size of the unlimited dim to 3 */
    anStart[0] = 0;
    anCount[0] = 3;
    status = nc_put_vara_double(cdfid, other_var, anStart, anCount, val);
    assert( status == NC_NOERR );

    /* Read 2 elements starting with index=1 */
    anStart[0] = 1;
    anCount[0] = 2;
    status = nc_get_vara_double(cdfid, varid, anStart, anCount, val);
    assert( status == NC_NOERR );

    status = nc_close(cdfid);
    assert( status == NC_NOERR );

    free(val);

    return 0;
}
```

Running it under Valgrind without this patch leads to
```
==19637==
==19637== Invalid write of size 8
==19637==    at 0x4C326CB: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19637==    by 0x4EDBE3D: NC4_get_vars (hdf5var.c:2131)
==19637==    by 0x4EDA24C: NC4_get_vara (hdf5var.c:1342)
==19637==    by 0x4E68878: NC_get_vara (dvarget.c:104)
==19637==    by 0x4E69FDB: nc_get_vara_double (dvarget.c:815)
==19637==    by 0x400C08: main (in /home/even/netcdf-c/build/test)
==19637==  Address 0xb70e3e8 is 8 bytes before a block of size 24 alloc'd
==19637==    at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19637==    by 0x4009E8: main (in /home/even/netcdf-c/build/test)
==19637==
```


Patch:
diff --git a/configure.ac b/configure.ac
index f311281ee..ac41bf36d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1224,10 +1224,8 @@ if test "x$enable_pnetcdf" = xyes; then
       AC_MSG_ERROR([PNetCDF must be built with relax-coord-bound])
    fi
 else
-   if test "x$enable_erange_fill" = xauto; then
-      # if --enable-erange-fill is not used, default setting is no
-      enable_erange_fill=no
-   fi
+   # default setting
+   enable_erange_fill=no
 fi
 
 if test "x$enable_zero_length_coord_bound" = xyes; then
diff --git a/libdispatch/ncuri.c b/libdispatch/ncuri.c
index 8e9fe29ac..35de90d74 100644
--- a/libdispatch/ncuri.c
+++ b/libdispatch/ncuri.c
@@ -529,7 +529,11 @@ ncuribuild(NCURI* duri, const char* prefix, const char* suffix, int flags)
 {
     char* newuri = NULL;
     NCbytes* buf = ncbytesnew();
+#ifdef NEWESCAPE
     const int encode = (flags&NCURIENCODE ? 1 : 0);
+#else
+    const int encode = 0;
+#endif
 
     if(prefix != NULL)
 	ncbytescat(buf,prefix);
@@ -570,18 +574,12 @@ ncuribuild(NCURI* duri, const char* prefix, const char* suffix, int flags)
     if(suffix != NULL)
 	ncbytescat(buf,suffix);
 
-    /* The query and the querylist are assumed to be unencoded */
-    if(flags & NCURIQUERY && duri->querylist != NULL) {
+    if((flags & NCURIQUERY) && duri->querylist != NULL) {
 	char** p;
 	int first = 1;
 	for(p=duri->querylist;*p;p+=2,first=0) {
 	    ncbytescat(buf,(first?"?":"&"));
-	    if(encode) {
-		char* encoded = ncuriencodeonly(p[0],queryallow);
-		ncbytescat(buf,encoded);
-	        nullfree(encoded);
-	    } else
-	        ncbytescat(buf,p[0]);
+	    ncbytescat(buf,p[0]);
 	    if(p[1] != NULL && strlen(p[1]) > 0) {
 		ncbytescat(buf,"=");
 		if(encode) {
diff --git a/libhdf5/hdf5var.c b/libhdf5/hdf5var.c
index 3ff19c4f6..2bf94f044 100644
--- a/libhdf5/hdf5var.c
+++ b/libhdf5/hdf5var.c
@@ -2092,7 +2092,7 @@ NC4_get_vars(int ncid, int varid, const size_t *startp, const size_t *countp,
         /* Skip past the real data we've already read. */
         if (!no_read)
             for (real_data_size = file_type_size, d2 = 0; d2 < var->ndims; d2++)
-                real_data_size *= (count[d2] - start[d2]);
+                real_data_size *= count[d2];
 
         /* Get the fill value from the HDF5 variable. Memory will be
          * allocated. */
diff --git a/oc2/ocinternal.c b/oc2/ocinternal.c
index c481392c1..e5d4fd9f0 100644
--- a/oc2/ocinternal.c
+++ b/oc2/ocinternal.c
@@ -48,6 +48,7 @@
 /*Forward*/
 static OCerror ocextractddsinmemory(OCstate*,OCtree*,int);
 static OCerror ocextractddsinfile(OCstate*,OCtree*,int);
+static char* constraintescape(const char* url);
 static OCerror createtempfile(OCstate*,OCtree*);
 static int dataError(XXDR* xdrs, OCstate*);
 static void ocremovefile(const char* path);
@@ -164,7 +165,9 @@ ocfetch(OCstate* state, const char* constraint, OCdxd kind, OCflags flags,
     memset((void*)tree,0,sizeof(OCtree));
     tree->dxdclass = kind;
     tree->state = state;
-    tree->constraint = nulldup(constraint);
+    tree->constraint = constraintescape(constraint);
+    if(tree->constraint == NULL)
+	tree->constraint = nulldup(constraint);
 
     /* Set per-fetch curl properties */
 #if 0 /* temporarily make per-link */
@@ -454,6 +457,40 @@ fprintf(stderr,"missing bod: ddslen=%lu bod=%lu\n",
     return OCTHROW(stat);
 }
 
+/* Allow these (non-alpha-numerics) to pass thru */
+static const char okchars[] = "&/:;,.=?@'\"<>{}!|\\^[]`~";
+static const char hexdigits[] = "0123456789abcdef";
+
+/* Modify constraint to use %XX escapes */
+static char*
+constraintescape(const char* url)
+{
+    size_t len;
+    char* p;
+    int c;
+    char* eurl;
+
+    if(url == NULL) return NULL;
+    len = strlen(url);
+    eurl = ocmalloc(1+3*len); /* worst case: c -> %xx */
+    MEMCHECK(eurl,NULL);
+    p = eurl;
+    *p = '\0';
+    while((c=*url++)) {
+	if(c >= '0' && c <= '9') {*p++ = c;}
+	else if(c >= 'a' && c <= 'z') {*p++ = c;}
+	else if(c >= 'A' && c <= 'Z') {*p++ = c;}
+	else if(strchr(okchars,c) != NULL) {*p++ = c;}
+	else {
+	    *p++ = '%';
+	    *p++ = hexdigits[(c & 0xf0)>>4];
+	    *p++ = hexdigits[(c & 0xf)];
+	}
+    }
+    *p = '\0';
+    return eurl;
+}
+
 OCerror
 ocupdatelastmodifieddata(OCstate* state)
 {


Test patch:
diff --git a/nc_test4/CMakeLists.txt b/nc_test4/CMakeLists.txt
index db048656c..38d433824 100644
--- a/nc_test4/CMakeLists.txt
+++ b/nc_test4/CMakeLists.txt
@@ -20,7 +20,7 @@ SET(NC4_TESTS tst_dims tst_dims2 tst_dims3 tst_files tst_files4
   tst_files6 tst_sync tst_h_strbug tst_h_refs tst_h_scalar tst_rename
   tst_rename2 tst_rename3 tst_h5_endians tst_atts_string_rewrite tst_put_vars_two_unlim_dim
   tst_hdf5_file_compat tst_fill_attr_vanish tst_rehash tst_types tst_bug324
-  tst_atts3 tst_put_vars tst_elatefill tst_udf)
+  tst_atts3 tst_put_vars tst_elatefill tst_udf tst_bug1442)
 
 # Note, renamegroup needs to be compiled before run_grp_rename
 
diff --git a/nc_test4/Makefile.am b/nc_test4/Makefile.am
index 1396aaf4c..4f56492f1 100644
--- a/nc_test4/Makefile.am
+++ b/nc_test4/Makefile.am
@@ -33,7 +33,7 @@ tst_vars2 tst_files5 tst_files6 tst_sync tst_h_scalar tst_rename	\
 tst_rename2 tst_rename3 tst_h5_endians tst_atts_string_rewrite		\
 tst_hdf5_file_compat tst_fill_attr_vanish tst_rehash tst_filterparser	\
 tst_bug324 tst_types tst_atts3 tst_put_vars tst_elatefill tst_udf	\
-tst_put_vars_two_unlim_dim
+tst_put_vars_two_unlim_dim tst_bug1442
 
 # Temporary I hoped, but hoped in vain.
 if !ISCYGWIN
diff --git a/nc_test4/tst_bug1442.c b/nc_test4/tst_bug1442.c
new file mode 100644
index 000000000..25e6aa268
--- /dev/null
+++ b/nc_test4/tst_bug1442.c
@@ -0,0 +1,84 @@
+/*! \file
+
+Copyright 2019
+University Corporation for Atmospheric Research/Unidata.
+
+Author: Even Rouault
+
+See \ref copyright file for more info.
+
+*/
+#include <nc_tests.h>
+#include "err_macros.h"
+#include <stdio.h>
+#include <stdlib.h>
+#include <netcdf.h>
+
+#define FILENAME "tst_bug1442.nc"
+
+int
+main(int argc, char **argv)
+{
+    /* Test bugfix for https://github.com/Unidata/netcdf-c/pull/1442 */
+    /* that is using nc_get_vara_xxx() with a offset[] != 0 on a variable */
+    /* indexed by a unlimited dimension, and the size taken by that variable */
+    /* does not reach the actual size of that dimension */
+    int status;
+    int cdfid = -1;
+    int unlimited_dim;
+    int varid;
+    int other_var;
+    size_t start[1];
+    size_t count[1];
+    double five_zeros[5] = {0, 0, 0, 0, 0};
+    double one_two[2] = {1, 2};
+    double* four_double = (double*)calloc(4, sizeof(double));
+
+    status = nc_create(FILENAME, NC_NETCDF4, &cdfid);
+    if( status ) ERR;
+
+    status = nc_def_dim(cdfid, "unlimited_dim", NC_UNLIMITED, &unlimited_dim);
+    if( status ) ERR;
+
+    status = nc_def_var(cdfid, "my_var", NC_DOUBLE, 1, &unlimited_dim, &varid);
+    if( status ) ERR;
+
+    status = nc_def_var(cdfid, "other_var", NC_DOUBLE, 1, &unlimited_dim, &other_var);
+    if( status ) ERR;
+
+    status = nc_enddef(cdfid);
+    if( status ) ERR;
+
+    /* Write 5 elements to set the size of the unlimited dim to 5 */
+    start[0] = 0;
+    count[0] = 5;
+    status = nc_put_vara_double(cdfid, other_var, start, count, five_zeros);
+    if( status ) ERR;
+
+    /* Write 2 elements in my_var */
+    start[0] = 0;
+    count[0] = 2;
+    status = nc_put_vara_double(cdfid, varid, start, count, one_two);
+    if( status ) ERR;
+
+    /* Read 4 elements starting with index=1 */
+    start[0] = 1;
+    count[0] = 4;
+    status = nc_get_vara_double(cdfid, varid, start, count, four_double);
+    if( status ) ERR;
+
+    if( four_double[0] != 2 ) ERR;
+    if( four_double[1] != NC_FILL_DOUBLE ) ERR;
+    if( four_double[2] != NC_FILL_DOUBLE ) ERR;
+    if( four_double[3] != NC_FILL_DOUBLE ) ERR;
+
+    status = nc_close(cdfid);
+    if( status ) ERR;
+
+    free(four_double);
+
+    SUMMARIZE_ERR;
+    FINAL_RESULTS;
+
+    return 0;
+}