Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect handling of NULL count for nc_get/put_vars*() for netCDF-4 files #1029

Closed
edhartnett opened this issue Jun 25, 2018 · 1 comment · Fixed by #1112
Closed

Incorrect handling of NULL count for nc_get/put_vars*() for netCDF-4 files #1029

edhartnett opened this issue Jun 25, 2018 · 1 comment · Fixed by #1112

Comments

@edhartnett
Copy link
Contributor

In nc4_get_vars() (in libhdf5/nc4hdf5.c) we have:

 /* Open this dataset if necessary, also checking for a weird case:
   * a non-coordinate (and non-scalar) variable that has the same
   * name as a dimension. */
  if (var->hdf5_name && strlen(var->hdf5_name) >= strlen(NON_COORD_PREPEND) &&
      strncmp(var->hdf5_name, NON_COORD_PREPEND, strlen(NON_COORD_PREPEND)) == 0 &&
      var->ndims)
     name_to_use = var->hdf5_name;
  else
     name_to_use = var->hdr.name;

Test coverage shows we are not testing the first half of this if statement. We need to test the case of a dataset which has the same name as a dimension, but is not a coordinate variable. I will add this test.

@edhartnett edhartnett changed the title Case of netcdf-4 var name same as dim name needs test Incorrect handling of NULL count for nc_get/put_vars*() for netCDF-4 files Aug 8, 2018
@edhartnett
Copy link
Contributor Author

After some investigation, it turns out this code was never executed anyway. In all cases var->hdf4_datasetid was already set by the time the if statement is reached. I have replaced it with an assert.

However, this lead to another discovery. We also had this code in both put and get vars:

      start[i] = (startp == NULL ? 0 : startp[i]);
      count[i] = (countp == NULL ? 1 : countp[i]);
      stride[i] = (stridep == NULL ? 1 : stridep[i]);

This is correct for the start and stride, but not the count. If countp is NULL, then count[i] needs to be set to the full extent of the dim, not to 1.

This was also untested by current tests. I have fixed the code and added a test. I will put a PR up in a while with the fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant