From 9b7202bf06ff182c2907c4055189932e64bf62ea Mon Sep 17 00:00:00 2001 From: Dennis Heimbigner Date: Sat, 19 Feb 2022 16:47:31 -0700 Subject: [PATCH 1/2] Explicitly disallow variable length type compression re: https://github.com/Unidata/netcdf-c/issues/2189 Compression of a variable whose type is variable length fails for all current filters. This is because at some point, the compression buffer will contain pointers to data instead of the actual data. Compression of pointers of course is meaningless. The PR changes the behavior of nc_def_var_filter so that it will fail with error NC_EFILTER if an attempt is made to add a filter to a variable whose type is variable-length. A variable is variable-length if it is of type string or VLEN or transitively (via a compound type) contains a string or VLEN. Also added a test case for this. ## Misc Changes 1. Turn off a number of debugging statements --- .github/workflows/mingw.yml | 2 +- RELEASE_NOTES.md | 1 + dap4_test/test_data.c | 2 +- libdap2/dceconstraints.c | 2 +- libdispatch/dfilter.c | 14 +- libdispatch/dpathmgr.c | 6 +- libdispatch/ncs3sdk.cpp | 6 +- nc_test4/CMakeLists.txt | 1 + nc_test4/Makefile.am | 2 +- nc_test4/test_filter_misc.c | 16 +- nc_test4/test_filter_vlen.c | 494 +++++++++++++++++++++++++++++ nc_test4/tst_filter.sh | 1 - ncdap_test/t_auth.c | 2 +- ncdump/tst_chunking.c | 2 +- ncdump/tst_nccopy4.sh | 16 +- ncdump/tst_unicode.c | 2 +- ncgen/dump.c | 2 +- nczarr_test/ref_byte.cdl | 4 +- nczarr_test/ref_groups_regular.cdl | 10 +- nczarr_test/run_nczarr_fill.sh | 14 +- nczarr_test/s3util.c | 2 +- nczarr_test/tst_zchunks3.c | 2 +- nczarr_test/ut_json.c | 2 +- 23 files changed, 561 insertions(+), 44 deletions(-) create mode 100644 nc_test4/test_filter_vlen.c diff --git a/.github/workflows/mingw.yml b/.github/workflows/mingw.yml index a6fd8ad39e..0f9c9a2c5d 100644 --- a/.github/workflows/mingw.yml +++ b/.github/workflows/mingw.yml @@ -1,5 +1,5 @@ name: NetCDF-Build MinGW -on: [workflow_dispatch] +on: [workflow_dispatch,push] jobs: build: diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index e64d4f4517..15d45dc546 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -7,6 +7,7 @@ This file contains a high-level description of this package's evolution. Release ## 4.8.2 - TBD +* [Bug Fix] Require that the type of the variable in nc_def_var_filter is not variable length. See [Github #/???](https://github.com/Unidata/netcdf-c/pull/????). * [Enhancement] Add complete bitgroom support to NCZarr. See [Github #2197](https://github.com/Unidata/netcdf-c/pull/2197). * [Bug Fix] Clean up the handling of deeply nested VLEN types. Marks nc_free_vlen() and nc_free_string as deprecated in favor of ncaux_reclaim_data(). See [Github #2179(https://github.com/Unidata/netcdf-c/pull/2179). * [Bug Fix] Make sure that netcdf.h accurately defines the flags in the open/create mode flags. See [Github #2183](https://github.com/Unidata/netcdf-c/pull/2183). diff --git a/dap4_test/test_data.c b/dap4_test/test_data.c index 946c37b2ab..ab9f8f3a8d 100644 --- a/dap4_test/test_data.c +++ b/dap4_test/test_data.c @@ -12,7 +12,7 @@ Test the netcdf-4 data building process. #include #include "netcdf.h" -#define DEBUG +#undef DEBUG static void fail(int code) diff --git a/libdap2/dceconstraints.c b/libdap2/dceconstraints.c index 800e5eb089..b4895768f8 100644 --- a/libdap2/dceconstraints.c +++ b/libdap2/dceconstraints.c @@ -13,7 +13,7 @@ #include "dapincludes.h" #include "dceparselex.h" -#define DEBUG +#undef DEBUG #define LBRACE "{" #define RBRACE "}" diff --git a/libdispatch/dfilter.c b/libdispatch/dfilter.c index e5ea120a9d..e3cf9270da 100644 --- a/libdispatch/dfilter.c +++ b/libdispatch/dfilter.c @@ -116,10 +116,18 @@ nc_inq_var_filter_info(int ncid, int varid, unsigned int id, size_t* nparamsp, u EXTERNL int nc_def_var_filter(int ncid, int varid, unsigned int id, size_t nparams, const unsigned int* params) { + int stat = NC_NOERR; NC* ncp; - int stat = NC_check_id(ncid,&ncp); - if(stat != NC_NOERR) return stat; - TRACE(nc_inq_var_filter_info); + int fixedsize; + nc_type xtype; + + TRACE(nc_inq_var_filter); + if((stat = NC_check_id(ncid,&ncp))) return stat; + /* Get variable' type */ + if((stat = nc_inq_vartype(ncid,varid,&xtype))) return stat; + /* If the variable's type is not fixed-size, then signal error */ + if((stat = NC4_inq_type_fixed_size(ncid, xtype, &fixedsize))) return stat; + if(!fixedsize) return NC_EFILTER; if((stat = ncp->dispatch->def_var_filter(ncid,varid,id,nparams,params))) goto done; done: return stat; diff --git a/libdispatch/dpathmgr.c b/libdispatch/dpathmgr.c index c95273892d..83418dba5f 100644 --- a/libdispatch/dpathmgr.c +++ b/libdispatch/dpathmgr.c @@ -42,7 +42,7 @@ #undef DEBUGPATH static int pathdebug = -1; -#define DEBUG +#undef DEBUG #ifdef DEBUG #define REPORT(e,msg) report((e),(msg),__LINE__) @@ -113,7 +113,9 @@ static int wide2utf8(const wchar_t* u16, char** u8p); #endif /*Forward*/ +#ifdef DEBUG static void report(int stat, const char* msg, int line); +#endif static char* printPATH(struct Path* p); EXTERNL @@ -1227,6 +1229,7 @@ printutf8hex(const char* s, char* sx) *q = '\0'; } +#ifdef DEBUG static void report(int stat, const char* msg, int line) { @@ -1235,3 +1238,4 @@ report(int stat, const char* msg, int line) line,msg,stat,nc_strerror(stat)); } } +#endif diff --git a/libdispatch/ncs3sdk.cpp b/libdispatch/ncs3sdk.cpp index 2c51b3e762..1b0feaec02 100644 --- a/libdispatch/ncs3sdk.cpp +++ b/libdispatch/ncs3sdk.cpp @@ -73,9 +73,8 @@ NC_s3sdkinitialize(void) ncs3_finalized = 0; NCTRACE(11,NULL); Aws::InitAPI(ncs3options); - NCUNTRACE(NC_NOERR); } - return 1; + return NCUNTRACE(NC_NOERR); } EXTERNL int @@ -86,9 +85,8 @@ NC_s3sdkfinalize(void) ncs3_finalized = 1; NCTRACE(11,NULL); Aws::ShutdownAPI(ncs3options); - NCUNTRACE(NC_NOERR); } - return 1; + return NCUNTRACE(NC_NOERR); } static char* diff --git a/nc_test4/CMakeLists.txt b/nc_test4/CMakeLists.txt index f63e83938b..c2eb3be056 100644 --- a/nc_test4/CMakeLists.txt +++ b/nc_test4/CMakeLists.txt @@ -42,6 +42,7 @@ IF(ENABLE_FILTER_TESTING) build_bin_test(tst_multifilter) build_bin_test(test_filter_order) build_bin_test(test_filter_repeat) + build_bin_test(test_filter_vlen) ADD_SH_TEST(nc_test4 tst_filter) IF(ENABLE_BLOSC) ADD_SH_TEST(nc_test4 tst_specific_filters) diff --git a/nc_test4/Makefile.am b/nc_test4/Makefile.am index f853cd81e0..2eb2545b87 100644 --- a/nc_test4/Makefile.am +++ b/nc_test4/Makefile.am @@ -75,7 +75,7 @@ endif # Filter Tests (requires ncdump and ncgen) if ENABLE_FILTER_TESTING extradir = -check_PROGRAMS += test_filter test_filter_misc test_filter_order test_filter_repeat +check_PROGRAMS += test_filter test_filter_misc test_filter_order test_filter_repeat test_filter_vlen check_PROGRAMS += tst_multifilter TESTS += tst_filter.sh if ENABLE_BLOSC diff --git a/nc_test4/test_filter_misc.c b/nc_test4/test_filter_misc.c index 904684ee00..b849e839bb 100644 --- a/nc_test4/test_filter_misc.c +++ b/nc_test4/test_filter_misc.c @@ -131,17 +131,24 @@ verifychunks(void) static int create(void) { - int i; - /* Create a file with one big variable, but whose dimensions arte not a multiple of chunksize (to see what happens) */ CHECK(nc_create(testfile, NC_NETCDF4|NC_CLOBBER, &ncid)); CHECK(nc_set_fill(ncid, NC_NOFILL, NULL)); + return NC_NOERR; +} + +static int +defvar(nc_type xtype) +{ + int i; + + /* Create a file with one big variable, but whose dimensions arte not a multiple of chunksize (to see what happens) */ for(i=0;i +#include +#include + +#include +#include "netcdf.h" +#include "netcdf_aux.h" +#include "netcdf_filter.h" + +#undef TESTODDSIZE + +#undef DEBUG + +#ifndef H5Z_FILTER_FLETCHER32 +#define H5Z_FILTER_FLETCHER32 3 +#endif + +/* The C standard apparently defines all floating point constants as double; + we rely on that in this code. +*/ +#define DBLVAL 12345678.12345678 + +#define TEST_ID 32768 + +#define MAXERRS 8 + +#define MAXPARAMS 32 + +#define NPARAMS 14 + +static unsigned int baseline[NPARAMS]; + +static const char* testfile = NULL; + +#define MAXDIMS 8 + +#define DFALT_TESTFILE "tmp_misc.nc" + +#define spec "32768, -17b, 23ub, -25S, 27US, 77, 93U, 789f, 12345678.12345678d, -9223372036854775807L, 18446744073709551615UL" + +#ifdef TESTODDSIZE +#define NDIMS 1 +static size_t dimsize[NDIMS] = {4}; +static size_t chunksize[NDIMS] = {3}; +#else +#define NDIMS 4 +static size_t dimsize[NDIMS] = {4,4,4,4}; +static size_t chunksize[NDIMS] = {4,4,4,4}; +#endif + +static size_t ndims = NDIMS; + +static size_t totalproduct = 1; /* x-product over max dims */ +static size_t actualproduct = 1; /* x-product over actualdims */ +static size_t chunkproduct = 1; /* x-product over actual chunks */ + +static size_t pattern[MAXDIMS]; + +static int nerrs = 0; + +static int ncid, varid; +static int dimids[MAXDIMS]; +static size_t odom[MAXDIMS]; +static float* array = NULL; +static float* expected = NULL; + +static unsigned int filterid = 0; +static size_t nparams = 0; +static unsigned int params[MAXPARAMS]; + +/* Forward */ +static int test_test1(void); +static void init(int argc, char** argv); +static void reset(void); +static void odom_reset(void); +static int odom_more(void); +static int odom_next(void); +static int odom_offset(void); +static float expectedvalue(void); +static void verifyparams(void); + +#define ERRR do { \ +fflush(stdout); /* Make sure our stdout is synced with stderr. */ \ +fprintf(stderr, "Sorry! Unexpected result, %s, line: %d\n", \ + __FILE__, __LINE__); \ +nerrs++;\ +} while (0) + +static int +check(int err,int line) +{ + if(err != NC_NOERR) { + fprintf(stderr,"fail (%d): %s\n",line,nc_strerror(err)); + } + return NC_NOERR; +} + +static void +report(const char* msg, int lineno) +{ + fprintf(stderr,"fail: line=%d %s\n",lineno,msg); + exit(1); +} + +#define CHECK(x) check(x,__LINE__) +#define REPORT(x) report(x,__LINE__) + +static int +verifychunks(void) +{ + int i; + int store = -1; + size_t localchunks[MAXDIMS]; + memset(localchunks,0,sizeof(localchunks)); + CHECK(nc_inq_var_chunking(ncid, varid, &store, localchunks)); + if(store != NC_CHUNKED) { + fprintf(stderr,"bad chunk store\n"); + return 0; + } + for(i=0;i 0) { + params = (unsigned int*)malloc(sizeof(unsigned int)*nparams); + if(params == NULL) + return NC_ENOMEM; + CHECK(nc_inq_var_filter(ncid,varid,&filterid,&nparams,params)); + } + if(filterid != TEST_ID) { + fprintf(stderr,"open: test id mismatch: %d\n",filterid); + free(params); + return NC_EFILTER; + } + if(nparams != NPARAMS) { + size_t i; + fprintf(stderr,"nparams mismatch\n"); + for(nerrs=0,i=0;i 0) return NC_EFILTER; + + /* Verify chunking */ + if(!verifychunks()) + return 0; + fflush(stderr); + return 1; +} + +static int +setchunking(void) +{ + int store; + + store = NC_CHUNKED; + CHECK(nc_def_var_chunking(ncid,varid,store,chunksize)); + if(!verifychunks()) + return NC_EINVAL; + return NC_NOERR; +} + +static void +fill(void) +{ + odom_reset(); + if(1) { + int i; + if(actualproduct <= 1) abort(); + for(i=0;i= MAXERRS) + break; + } + } + } else + { + odom_reset(); + while(odom_more()) { + int offset = odom_offset(); + float expect = expectedvalue(); + if(array[offset] != expect) { + fprintf(stderr,"data mismatch: array[%d]=%f expected=%f\n", + offset,array[offset],expect); + errs++; + if(errs >= MAXERRS) + break; + } + odom_next(); + } + } + + if(errs == 0) + fprintf(stderr,"no data errors\n"); + return (errs == 0); +} + +static void +showparameters(void) +{ + int i; + fprintf(stderr,"test: nparams=%ld: params=",(unsigned long)nparams); + for(i=0;i=0;i--) { + odom[i] += 1; + if(odom[i] < dimsize[i]) break; + if(i == 0) return 0; /* leave the 0th entry if it overflows*/ + odom[i] = 0; /* reset this position*/ + } + return 1; +} + +static int +odom_offset(void) +{ + int i; + int offset = 0; + for(i=0;i 1) + testfile = argv[1]; + else + testfile = DFALT_TESTFILE; + + /* Setup various variables */ + totalproduct = 1; + actualproduct = 1; + chunkproduct = 1; + for(i=0;i 0?1:0); +} diff --git a/nc_test4/tst_filter.sh b/nc_test4/tst_filter.sh index ccbeb7fc1c..6700588344 100755 --- a/nc_test4/tst_filter.sh +++ b/nc_test4/tst_filter.sh @@ -251,7 +251,6 @@ diff -b -w ${srcdir}/ref_filter_repeat.txt tmp_filterrepeat.txt fi if test "x$ORDER" = x1 ; then - echo "*** Testing multiple filter order of invocation on create" rm -f tmp_crfilterorder.txt ${execdir}/test_filter_order create >tmp_crfilterorder.txt diff --git a/ncdap_test/t_auth.c b/ncdap_test/t_auth.c index e70a068e87..2d6629d22a 100644 --- a/ncdap_test/t_auth.c +++ b/ncdap_test/t_auth.c @@ -16,7 +16,7 @@ See \ref copyright file for more info. #include #endif -#define DEBUG +#undef DEBUG #include "netcdf.h" #include "nctestserver.h" diff --git a/ncdump/tst_chunking.c b/ncdump/tst_chunking.c index 95cd92010b..46c9ddc5ba 100644 --- a/ncdump/tst_chunking.c +++ b/ncdump/tst_chunking.c @@ -9,7 +9,7 @@ #include #include "err_macros.h" -#define DEBUG +#undef DEBUG static int ret = NC_NOERR; diff --git a/ncdump/tst_nccopy4.sh b/ncdump/tst_nccopy4.sh index ad0a2294e2..7b482a78b7 100755 --- a/ncdump/tst_nccopy4.sh +++ b/ncdump/tst_nccopy4.sh @@ -5,8 +5,6 @@ if test "x$srcdir" = x ; then srcdir=`pwd`; fi set -e -export SETX=1 - # For a netCDF-4 build, test nccopy on netCDF files in this directory echo "@@@@@@" @@ -20,9 +18,11 @@ echo "" # These files are actually in $srcdir in distcheck builds, so they # need to be handled differently. # ref_tst_compounds2 ref_tst_compounds3 ref_tst_compounds4 -TESTFILES='tst_comp tst_comp2 tst_enum_data tst_fillbug +TESTFILES0='tst_comp tst_comp2 tst_enum_data tst_fillbug tst_group_data tst_nans tst_opaque_data tst_solar_1 tst_solar_2 - tst_solar_cmp tst_special_atts tst_string_data' + tst_solar_cmp tst_special_atts' + +TESTFILES="$TESTFILES0 tst_string_data" # Causes memory leak; source unknown MEMLEAK="tst_vlen_data" @@ -76,12 +76,12 @@ fi rm tst_deflated.nc tst_inflated.nc tst_inflated4.nc tmp.nc tmp.cdl echo "*** Testing nccopy -d1 -s on ncdump/*.nc files" -for i in $TESTFILES ; do +for i in $TESTFILES0 ; do echo "*** Test nccopy -d1 -s $i.nc copy_of_$i.nc ..." ${NCCOPY} -d1 -s $i.nc copy_of_$i.nc -${NCDUMP} -n copy_of_$i $i.nc > tmp.cdl -${NCDUMP} copy_of_$i.nc > copy_of_$i.cdl -# echo "*** compare " with copy_of_$i.cdl + ${NCDUMP} -n copy_of_$i $i.nc > tmp.cdl + ${NCDUMP} copy_of_$i.nc > copy_of_$i.cdl + # echo "*** compare " with copy_of_$i.cdl diff copy_of_$i.cdl tmp.cdl rm copy_of_$i.nc copy_of_$i.cdl tmp.cdl done diff --git a/ncdump/tst_unicode.c b/ncdump/tst_unicode.c index e742260db5..a3369559b6 100644 --- a/ncdump/tst_unicode.c +++ b/ncdump/tst_unicode.c @@ -23,7 +23,7 @@ #include #endif -#define DEBUG +#undef DEBUG /* The data file we will create. */ static const unsigned char prefix[] = { diff --git a/ncgen/dump.c b/ncgen/dump.c index c4e493dc4d..bd4d775b86 100644 --- a/ncgen/dump.c +++ b/ncgen/dump.c @@ -8,7 +8,7 @@ #include "includes.h" #include "dump.h" -#define DEBUGSRC +#undef DEBUGSRC #define MAXELEM 8 #define MAXDEPTH 4 diff --git a/nczarr_test/ref_byte.cdl b/nczarr_test/ref_byte.cdl index d4b7a680a7..7be7c03d3f 100644 --- a/nczarr_test/ref_byte.cdl +++ b/nczarr_test/ref_byte.cdl @@ -1,8 +1,8 @@ netcdf ref_byte { dimensions: - .zdim_20 = 20 ; + _zdim_20 = 20 ; variables: - ubyte byte(.zdim_20, .zdim_20) ; + ubyte byte(_zdim_20, _zdim_20) ; byte:_Storage = "chunked" ; byte:_ChunkSizes = 20, 20 ; diff --git a/nczarr_test/ref_groups_regular.cdl b/nczarr_test/ref_groups_regular.cdl index 178e9a5818..fd1875427a 100644 --- a/nczarr_test/ref_groups_regular.cdl +++ b/nczarr_test/ref_groups_regular.cdl @@ -1,15 +1,15 @@ netcdf tmp_groups_regular { dimensions: - .zdim_3 = 3 ; - .zdim_2 = 2 ; - .zdim_10 = 10 ; + _zdim_3 = 3 ; + _zdim_2 = 2 ; + _zdim_10 = 10 ; // global attributes: :_Format = "netCDF-4" ; group: MyGroup { variables: - int dset1(.zdim_3, .zdim_3) ; + int dset1(_zdim_3, _zdim_3) ; dset1:_Storage = "chunked" ; dset1:_ChunkSizes = 3, 3 ; dset1:_NoFill = "true" ; @@ -24,7 +24,7 @@ group: MyGroup { group: Group_A { variables: - int dset2(.zdim_2, .zdim_10) ; + int dset2(_zdim_2, _zdim_10) ; dset2:_Storage = "chunked" ; dset2:_ChunkSizes = 2, 10 ; dset2:_NoFill = "true" ; diff --git a/nczarr_test/run_nczarr_fill.sh b/nczarr_test/run_nczarr_fill.sh index d03c425ff3..2176ee3e2d 100755 --- a/nczarr_test/run_nczarr_fill.sh +++ b/nczarr_test/run_nczarr_fill.sh @@ -48,15 +48,17 @@ diff -wb ${srcdir}/ref_byte_fill_value_null.cdl tmp_byte_fill_value_null_$zext.c rm -fr ref_byte_fill_value_null.zarr } -#testcase2059 file -#testcase2062 file + +testcase2062 file testcase2063 file -exit 0 -if test "x$FEATURE_NCZARR_ZIP" = xyes ; then +if test "x$FEATURE_HDF5" = xyes ; then + testcase2059 file + if test "x$FEATURE_NCZARR_ZIP" = xyes ; then testcase2059 zip -fi -if test "x$FEATURE_S3TESTS" = xyes ; then + fi + if test "x$FEATURE_S3TESTS" = xyes ; then testcase2059 s3 + fi fi exit 0 diff --git a/nczarr_test/s3util.c b/nczarr_test/s3util.c index 03ec172a23..07e03ef1c9 100644 --- a/nczarr_test/s3util.c +++ b/nczarr_test/s3util.c @@ -27,7 +27,7 @@ #undef NODELETE -#define DEBUG +#undef DEBUG #define DATANAME "data" diff --git a/nczarr_test/tst_zchunks3.c b/nczarr_test/tst_zchunks3.c index b3d238d88a..fa0c575194 100644 --- a/nczarr_test/tst_zchunks3.c +++ b/nczarr_test/tst_zchunks3.c @@ -8,7 +8,7 @@ #include "ut_includes.h" #include "test_nczarr_utils.h" -#define DEBUG +#undef DEBUG static int ret = NC_NOERR; #define FILE_NAME "tmp_chunks3.nc" diff --git a/nczarr_test/ut_json.c b/nczarr_test/ut_json.c index 22600fcb56..37ab65d231 100644 --- a/nczarr_test/ut_json.c +++ b/nczarr_test/ut_json.c @@ -5,7 +5,7 @@ #include "ut_includes.h" -#define DEBUG +#undef DEBUG typedef enum Cmds { cmd_none = 0, From 78f8dad82b340b5279b053ff5136c2a61a2d457a Mon Sep 17 00:00:00 2001 From: Dennis Heimbigner Date: Sat, 19 Feb 2022 17:22:11 -0700 Subject: [PATCH 2/2] Update Release Notes --- RELEASE_NOTES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 15d45dc546..43beb7b919 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -7,7 +7,7 @@ This file contains a high-level description of this package's evolution. Release ## 4.8.2 - TBD -* [Bug Fix] Require that the type of the variable in nc_def_var_filter is not variable length. See [Github #/???](https://github.com/Unidata/netcdf-c/pull/????). +* [Bug Fix] Require that the type of the variable in nc_def_var_filter is not variable length. See [Github #/2231](https://github.com/Unidata/netcdf-c/pull/2231). * [Enhancement] Add complete bitgroom support to NCZarr. See [Github #2197](https://github.com/Unidata/netcdf-c/pull/2197). * [Bug Fix] Clean up the handling of deeply nested VLEN types. Marks nc_free_vlen() and nc_free_string as deprecated in favor of ncaux_reclaim_data(). See [Github #2179(https://github.com/Unidata/netcdf-c/pull/2179). * [Bug Fix] Make sure that netcdf.h accurately defines the flags in the open/create mode flags. See [Github #2183](https://github.com/Unidata/netcdf-c/pull/2183).