Invalid argument error with libnetcdf 4.5.0 but not 4.4.1

[spencerkclark]

Per the recommendation of @dopplershift (cc: @spencerahill), I am migrating an issue (Unidata/netcdf4-python#742) initially posted in netcdf4-python over to here.

Environment Information

• I have reproduced this behavior on my local machine (using Mac OS version 10.13)
• This behavior was first identified with our test suite run using Travis CI (see here for the platform information)
• On both platforms libnetcdf was installed from the conda-forge channel (e.g. conda install -c conda-forge libnetcdf).

Summary of Issue

Since libnetcdf version 4.5.0 appeared on conda-forge, we have encountered some puzzling errors in our CI test suite. Namely, some files that we used to be able to open with xarray using netCDF4 now lead to errors like the following:

aospy/test/test_calc_basic.py:60: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
aospy/test/test_calc_basic.py:35: in _test_files_and_attrs
    _test_output_attrs(calc, dtype_out)
aospy/test/test_calc_basic.py:18: in _test_output_attrs
    data = xr.open_dataset(calc.path_out[dtype_out])
../../../miniconda/envs/test_env/lib/python3.6/site-packages/xarray/backends/api.py:282: in open_dataset
    autoclose=autoclose)
../../../miniconda/envs/test_env/lib/python3.6/site-packages/xarray/backends/netCDF4_.py:210: in __init__
    self.ds = opener()
../../../miniconda/envs/test_env/lib/python3.6/site-packages/xarray/backends/netCDF4_.py:185: in _open_netcdf4_group
    ds = nc4.Dataset(filename, mode=mode, **kwargs)
netCDF4/_netCDF4.pyx:2015: in netCDF4._netCDF4.Dataset.__init__
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
>   ???
E   OSError: [Errno -36] NetCDF: Invalid argument: b'/home/travis/build/spencerahill/aospy/aospy/test/data/objects/test-files/example_proj/example_model/example_run/condensation_rain/condensation_rain.ann.av.from_monthly_ts.example_model.example_run.0004-0006.nc'

In doing some investigation offline, it seems that the command line tool ncdump provided by libnetcdf version 4.5.0 is also unable to open these files. E.g. I get the following response:

$ ncdump sphum.ann.reg.av.vert_int.from_monthly_ts_sigma.example_model.example_run.0006.nc
ncdump: sphum.ann.reg.av.vert_int.from_monthly_ts_sigma.example_model.example_run.0006.nc: NetCDF: Invalid argument

However, using libnetcdf version 4.4.1.1, ncdump is able to handle the file (and using version 4.4.1.1 also enables downstream tools like netcdf4-python to work):

$ ncdump -h sphum.ann.reg.av.vert_int.from_monthly_ts_sigma.example_model.example_run.0006.nc
netcdf sphum.ann.reg.av.vert_int.from_monthly_ts_sigma.example_model.example_run.0006 {
variables:
	double raw_data_start_date ;
		raw_data_start_date:units = "days since 1673-01-01" ;
		raw_data_start_date:long_name = "time axis boundaries" ;
		raw_data_start_date:calendar = "NOLEAP" ;
		raw_data_start_date:_FillValue = 1.e+20 ;
	int subset_end_date ;
		subset_end_date:units = "days since 1678-01-31 00:00:00" ;
		subset_end_date:calendar = "proleptic_gregorian" ;
	double globe ;
		globe:units = "(vertical integral of quantity with unspecified units)" ;
		globe:_FillValue = NaN ;
		globe:description = "specific humidity" ;
	double raw_data_end_date ;
		raw_data_end_date:units = "days since 1673-01-01" ;
		raw_data_end_date:long_name = "time axis boundaries" ;
		raw_data_end_date:calendar = "NOLEAP" ;
		raw_data_end_date:_FillValue = 1.e+20 ;
	int subset_start_date ;
		subset_start_date:units = "days since 1678-01-01 00:00:00" ;
		subset_start_date:calendar = "proleptic_gregorian" ;

// global attributes:
		:coordinates = "raw_data_start_date subset_end_date raw_data_end_date subset_start_date" ;
}

I have posted a link to this file as an example. It would be helpful to know if you can reproduce the issues we've encountered here with files like the one provided, and what about them led to this problem. Thanks in advance for your help!

Steps to reproduce the behavior

Using the latest version of libnetcdf from the conda-forge channel, download the linked file and try to run ncdump.

[dopplershift]

I can reproduce with conda-forge libnetcdf 4.5.0 on my osx machine as well.

[WardF]

I believe we have fixed this with a pull to revert to 4.4.1.1 behavior based on a bug report issued by @czender. Let me confirm and follow up with specifics.

[WardF]

It is not corrected, looking at it now.

[WardF]

Ok, the issue relates to how padding is being calculated in v1hpg.c:v1h_get_NC_string(). This code was contributed to us by @wkliao so taking a bit to read it and understand it.

[spencerkclark]

Many thanks for investigating this @dopplershift and @WardF. We have no issues sticking with version 4.4.1.1 for now, but we just wanted to bring this to your attention (also in case it impacts anyone else).

[dopplershift]

Thanks for the report!

[wkliao]

FYI. @WardF is referring to PR #332, which checks the null-byte padding in file header.

After reading this PR again, I found that returning error code NC_ENOTNC is better than NC_EINVAL in this case, as the error message corresponds to NC_ENOTNC is "NetCDF: Unknown file format", which includes the case when file fails to conform with the classic CDF file format specification. In this case, it is the null-byte padding.

FYI. PnetCDF returns NC_ENOTNC.
@WardF, let me know if you agree with this change. I can create a PR.

[dopplershift]

But...the file is a valid, so I'm not clear on why it's an error at all.

[WardF]

@wkliao I can make that change if need be but I need to determine why the file is invalid and, if it is, is it invalid in a way that contradicts the spec but became valid through common usage and the spec not being enforced historically. I will follow up when I get in to the office and can inspect the file more closely.

[wkliao]

I guess the nc file in question violates only the null-byte padding requirement by the CDF format specification. The reason it can be read by ncdump of earlier versions is those ncdump programs (and most likely other NetCDF utility programs) fail to check null-byte padding.

I consider this is not a serious bug in NetCDF library and its utilities, but strictly speaking the file does NOT conform with the CDF specification.

FYI, the files generated by NetCDF with non-null byte padding will get NC_ENOTNC from PnetCDF. Because PnetCDF was developed independently from NetCDF, sometimes PnetCDF conforms with the CDF specification more strictly. In fact, as early as the first release of PnetCDF 0.8.0, this null-byte padding checking has been implemented.

One possible solution is to develop a new utility program to detect and fix this padding for nc files.

CCed to @roblatham00

[wkliao]

Forgot to mention that there is a PnetCDF utility program named ncmpivalid. It can be used to validate a CDF file and report the exact location that fails to conform with the file specification.
https://trac.mcs.anl.gov/projects/parallel-netcdf/browser/tags/v1-9-0-pre1/src/utils/ncmpivalid

[WardF]

Given that netCDF hasn't enforced this up to this point, and that apparently enforcing this causes some issues with extant workflows, I am wondering if we need to revert this. I really want to avoid causing disruption to our users, especially when the practical positive impact seems to be limited. It can be reimplemented once we have an easy way for users to fix otherwise valid files, and will be the least disruptive to existing workflows, etc.

[WardF]

Alternatively this could be made an option instead of removed altogether, and have it turned off for now. I would also add a more specific error message, NC_EHDRPAD or some such, to make it clear what the issue is if the option is turned on. Just thinking out loud.

--enable-enforce-null-byte-padding perhaps.

[wkliao]

The problem is the fact NetCDF has partially enforced this null-byte padding from 3.3 release as early as I can tell. For example, null-byte padding is enforced when writing strings, say names of dimension, variables, etc. (see ncx_pad_putn_text() called through v1h_put_NC_string() or ncx_put_NC_string() in earlier versions). Similar padding has been done for attributes of type size less than 4 bytes (e.g. NC_CHAR, NC_BYTE, and NC_SHORT). Same for variables, but in data section, instead of header.

PR #332 was created based on the observation of inconsistency in this padding implementation. Specifically, it fixes the only place that fails to enforce null-byte padding, which is when writing the attribute values.

[WardF]

In a vacuum, I think it's great to enforce this documented standard, but in practice I think we need to accommodate files generated before this was enforced. I'm very hesitant to penalize people, especially without being able to suggest an easy/turn-key way to fix the file.

I like the idea of making this a configuration option (for now) that can, when appropriate, be flipped from 'off' by default to 'on'.

[edhartnett]

@wkliao may I ask whether there is benefit to enforcing this other than resolving an inconsistency?

Also may I ask how these files are created? How are files with incorrect null padding coming into existence?

[WardF]

I'm going to add this option while managing the other pull requests, but I am curious as well @wkliao . I assume the answer is 'via libnetcdf', in which case I think the burden is definitely on us to not penalize people (as we apparently are now, oops) without warning. I'll also add a test using the file linked above which illustrate the issue. It will be fixed in the 4.5.1 release which I'm hoping to get out as a bugfix release very quickly.

[edhartnett]

If these files were generated by libnetcdf, then I agree with @WardF that there is a moral obligation to continue reading them. Even if the files were generated elsewhere, it would be sad to stop reading files that previously could be read.

(We have been discussing the utility of logging - this would be a great place to log a warning but not return an error.)

However, my concern is also that we understand, fix, and test the bug which caused invalid files to be produced, if they were produced by libnetcdf.

In other words, have we stopped the problem from re-occurring? Or does it still happen?

[WardF]

@edhartnett #332 appears to also correct behavior when writing netcdf files, and I infer that means the files were generated by libnetcdf. I don't intend to gate the writing fix behind an option, only whether or not files should return an error when read or preserve the extant behavior.

Given the emphasis on netCDF as an archival format, I want to fix this ASAP. It does make me wonder how much archived data there is out there that would fail when a null byte padded buffer is enforced. That then begs the following question: is there a situation where, even in the presence of a utility to 'fix' the file, it is ok for archival data to default to returning an error when it is read?

Adding the option is going to be a great way for people to check their data archives for files that don't adhere to the format, and then potentially correct them to adhere to the format. I suspect this option may never default to 'on', however, for the reason I listed above. I don't want archives to suddenly break and require post-processing to work again.

[edhartnett]

Agree wholeheartedly. Existing archives need to be readable.

Since pnetcdf rejects these files, I suspect we would be hearing a lot more if these files were very common.

However, this may explain one of the reasons that the PIO library is written to first try the user-specified format (usually pnetcdf), and the fall back to using classic netcdf if that didn't work. So for the CESM people, this might be happening frequently, but they don't know.

[wkliao]

I hate to see files generated by NetCDF got rejected by PnetCDF. I believe this is a perfect example of why two independent implementations of the same standard is necessary, like in in MPI world, MPICH vs. OpenMPI.

I guess those files in question were created by NetCDF prior to 4.5.0, as I am not aware of other software that creates CDF files without NetCDF or PnetCDF.

@WardF, I hate to say this but your configure-time option will still not solve the problem of PnetCDF being unable to read the problematic files. Note this applies to all PnetCDF releases. I would guess the reverse may also exist, waiting for us to discover. At that time, what guideline should be use to resolve the conflict?. This is why, in my personal opinion, abiding to the standard is important.

On the other hand, I also hate to see those archive files rejected by the newer releases of NetCDF. I am not insisting the padding checking, but would like to point out the dilemma. I hope a perfect solution can come out from our discussion. At this moment, for automatic solutions, I do not see a better option than your approach, particularly in NetCDF realm. For non-automatic solutions, I propose in my earlier post a new utility program to fix the padding of existing files, still not an elegant one.

[wkliao]

FYI. I will change PnetCDF to skip the padding check for reading and the change will appear in 1.9.0 release. But, I will keep the utility, ncmpivalid intact, which can be useful to report this padding issue. (Wish I can change this behavior for all previous releases.)

[WardF]

I agree it is a dilemma, and if we could go back and ensure the standard were adhered to from the start, that would be best. At this point, however, rejecting archived files is pretty much a non-starter. I would prefer to find a better way and will continue to think about it.

[spencerkclark]

Also may I ask how these files are created?

To give a little more context, these files were created by our test suite using xarray with the scipy backend with versions:

• scipy: 0.18.1
• xarray: 0.9.6

When we try to read them using the netCDF4 backend we get the invalid argument error.

A minimal working example is:

In [1]: import xarray as xr

In [2]: da = xr.DataArray([1., 2., 3.], dims=['x'])

In [3]: da.to_netcdf('test_scipy.nc', engine='scipy')

In [4]: ds = xr.open_dataset('test_scipy.nc', engine='netcdf4')
---------------------------------------------------------------------------
IOError                                   Traceback (most recent call last)
<ipython-input-4-59fb1fb64f63> in <module>()
----> 1 ds = xr.open_dataset('test_scipy.nc')

//anaconda/envs/research/lib/python2.7/site-packages/xarray/backends/api.pyc in open_dataset(filename_or_obj, group, decode_cf, mask_and_scale, decode_times, autoclose, concat_characters, decode_coords, engine, chunks, lock, cache, drop_variables)
    280         if engine == 'netcdf4':
    281             store = backends.NetCDF4DataStore(filename_or_obj, group=group,
--> 282                                               autoclose=autoclose)
    283         elif engine == 'scipy':
    284             store = backends.ScipyDataStore(filename_or_obj,

//anaconda/envs/research/lib/python2.7/site-packages/xarray/backends/netCDF4_.pyc in __init__(self, filename, mode, format, group, writer, clobber, diskless, persist, autoclose)
    208                                    diskless=diskless, persist=persist,
    209                                    format=format)
--> 210         self.ds = opener()
    211         self._autoclose = autoclose
    212         self._isopen = True

//anaconda/envs/research/lib/python2.7/site-packages/xarray/backends/netCDF4_.pyc in _open_netcdf4_group(filename, mode, group, **kwargs)
    183     import netCDF4 as nc4
    184
--> 185     ds = nc4.Dataset(filename, mode=mode, **kwargs)
    186
    187     with close_on_error(ds):

netCDF4/_netCDF4.pyx in netCDF4._netCDF4.Dataset.__init__()

netCDF4/_netCDF4.pyx in netCDF4._netCDF4._ensure_nc_success()

IOError: [Errno -36] NetCDF: Invalid argument: '/Users/skc/test_scipy.nc'

We experience no issues when using the netCDF4 backend to save the files. Please let me know if you need any more information.

[dopplershift]

So the scipy backend is an independent implementation of netCDF3. That code has been around for quite awhile, so while it may be out of spec, there are probably quite a few files in the wild that have been created using it.

[wkliao]

@dopplershift, you are right. I can see an implementation of NetCDF3 in https://github.com/scipy/scipy/blob/master/scipy/io/netcdf.py
It probably does not check the padding either, but I am glad to know there is another NetCDF I/O developed independently from Unidata and PnetCDF.

[DennisHeimbigner]

FYI, netcdf has two. There is also a pure java implementation inside netcdf-Java.
No idea what it does vis-a-vis padding.

[DennisHeimbigner]

I do not understand this comment:

@WardF, I hate to say this but your configure-time option will still not solve the problem of
PnetCDF being unable to read the problematic files.

Is the issue that Ward;s proposed fix can of necessity only apply to the netcdf-c library and not pnetcdf
since the latter is under your control?

[shoyer]

It looks like the SciPy implementation of netCDF3 has a (longstanding) bug: it pads with the ASCII character 0 (byte \x30) rather than the null byte \x00
https://github.com/scipy/scipy/blob/73f723a6ec2e7fc93314b1407336e71bb9ab6de0/scipy/io/netcdf.py#L532 (and in a few other places in this file)

I'm happy to submit a PR to fix SciPy, but I agree that there are likely quite a few archived netCDF3 files out there in the world that were written with SciPy, so it would be great if this check can be relaxed.

[dopplershift]

Yeah, should probably fix SciPy, since that's just silly.

[wkliao]

Hi, @DennisHeimbigner, Glad to know more independent implementations of CDF formats.
My comments just explained PnetCDF of all releases will reject the problematic files. The same proposed solution even applied to PnetCDF will not change this fact.

[wkliao]

I just now took a closer look at the locations in the problematic file that violate the null-byte padding. I found two places. One is the name of global attribute "coordinates" and the other is the attribute values of "coordinates".

If this file was generated by NetCDF-C library, then the first violation should never occur. Null-byte padding was enforced for name objects by NetCDF at all time (at least since 3.6.3). Therefore, if Scipy was used to generate the file, as described by @spencerkclark, then SciPy may want to fix this as well.

Hi, @shoyer, the location you pointed out is for data section (i.e. when writing variables) not header section. The padding for variables has a different requirement by CDF specifications. It says "padding uses variable's fill value". I guess neither NetCDF-C or PnetCDF enforces this. Currently, both NetCDF-C and PnetCDF uses null-byte padding for variables.

[shoyer]

@wkliao True, but it also uses 0 rather than \x00 for padding in _pack_string as well, which is used for variable names:
https://github.com/scipy/scipy/blob/73f723a6ec2e7fc93314b1407336e71bb9ab6de0/scipy/io/netcdf.py#L773-L777

[wkliao]

I confirm.
I just made ncmpivalid to print the padding values used for names of all attribute and variables from that file in question and they are all 48 (or \x30).

[wkliao]

@shoyer, FYI, variable names are stored in the file header section, not data section. So the null-byte padding applies to variable names.

[WardF]

I understand the fix I propose will work for NetCDF but not pnetcdf; I am open to a better solution that does not require any modification of existing/archived data.

[roblatham00]

Postel's law "Be conservative in what you do, be liberal in what you accept from others" seems like good advice here: create files as closely adhering to the standard as possible, but if one or two implementations were a bit sloppy "invalid file" is harsh -- if there's any way the library can carry on with a best guess, do that. Sounds like that's the resolution here.

ncvalid as Wek-keng suggests should be the one piece that complains about any violations.

[wkliao]

Hi @WardF, since this issue has been closed by #675, I assume you have a good solution. I can see the configure-time option as you mentioned earlier. Do you mind letting us know a summary of your fix?

In PnetCDF, I simply skip the checking for null-byte padding at file open for reading and this will appear in the next release.

I also improved a PnetCDF utility program named ncvalidator that can be used to detect the locations of a classic CDF files that fail to conform the file specification, including this null-byte padding. To test ncvalidator, several "bad" classic files have been created artificially in the same folder for testing various header format errors. The file names should be able to describe the kinds of expected errors. For example, file bad_padding_name_dim.nc1, a CDF-1 file, violates null-byte padding at the dimension name. You are welcomed to use them to test NetCDF or SciPy. A few more "bad" files are also available in here created for testing other kinds of header errors.

Utility program ncvalidator also can repair the null-byte padding for the legacy files, if the command-line option "-x" is used and the file has write permission. Feedbacks are welcomed.

[WardF]

@wkliao My fix is essentially what you described; unless the option is specified at configure time, the null-byte padding check is skipped when opening the file to read.

[shoyer]

This will also be fixed in the next release of SciPy: scipy/scipy#8170

[spencerkclark]

Thanks @WardF and @shoyer!