Writing attributes with length 0: `buf parameter can't be NULL` about hdf5r HOT 16 CLOSED

I created a PR (#209) which contains a unit test for this edge case.

from hdf5r.

I think the issue occurs whenever there's a function from the HDF5 library that expects a buffer.

  if(XLENGTH(R_buf) == 0) {
    buf = NULL;
  }
  else {
    buf = ...
  }
  // ...
  ... = H5SomeFunction(..., buf, ...);

It appears this buffer shouldn't be NULL (otherwise the error H5SomeFunction(): buf parameter can't be NULL is thrown).

Question is, with what should each of these buf = NULL be replaced? I've had some success by replacing buf = NULL; with buf = R_alloc(1, 1); // allocate 1 byte, but that causes other problems down the road.

from hdf5r.

I managed to solve the issue by replacing the XLENGTH(.) == 0 condition with TYPEOF(.) == NILSXP:

  const void* buf;
  if(TYPEOF(R_buf) == NILSXP) {
    buf = NULL;
  }
  else {
    buf = (void *) VOIDPTR(R_buf);
  }

I created PR #211 which allows the unit tests I added in #209 to pass. It should be noted that I only performed the substitution only where needed for allowing the tests in #209 to pass.

from hdf5r.

sorry for the late reply. Will try to look at the fix over the Christmas break and thanks for the PR.

from hdf5r.

No worries and thanks! Is there anything I can help out with? Would it help to schedule a call?

from hdf5r.

just a quick update. I had an initial look at this, but I need to dig deeper. A purely technical issue is that the places where you made the fix all need to be moved. The reason is that this code is auto-generated, so the way the code is generated has to be adapted upstream. As that code is somewhat ... ugly, I need to do that adaptation on my end.

Stay tuned

from hdf5r.

Thanks for looking into this!

this code is auto-generated

It's good to hear that the code is auto-generated. I couldn't imagine maintaining this code base across different versions of HDF5 library.

Out of curiosity, is the script that generates the code in a public repository?

from hdf5r.

Fixed in #214.

You can look for the for the code generates the wrapper files under /inst/CWrappers... but beware, it is not pretty.

from hdf5r.

Aha, that's what the patches are for! Makes sense :)

Thanks for fixing the issue!

from hdf5r.

@rcannood : I had to revert the changes for now and reopen the issue. The hdf5r.Extra package is causing a reverse dependency error.

ycli1995/hdf5r.Extra#1

The code for the fix is located in the branch

https://github.com/hhoeflin/hdf5r/tree/bug/fix_empty_attrs_again

from hdf5r.

It seems @ycli1995 fixed ycli1995/hdf5r.Extra#1 ☺️

Could we get this fix merged and released? 🙇

from hdf5r.

Hi @hhoeflin!

I ran a revdepcheck on this branch:

> revdepcheck::revdep_check(timeout = as.difftime(600, units = "mins"), num_workers = 8)
── CHECK ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── 13 packages ──
✔ readNSx 0.0.2                          ── E: 0     | W: 0     | N: 0          
✔ hdf5r.Extra 0.0.5                      ── E: 0     | W: 0     | N: 0          
✔ rblt 0.2.4.6                           ── E: 0     | W: 0     | N: 0          
✔ dynutils 1.0.11                        ── E: 0     | W: 0     | N: 0   

I didn't let it finish completely, but I can already confirm that this PR does not introduce new errors to hdf5r.Extra anymore.

Would you be able to merge the PR? 🙇

from hdf5r.

from hdf5r.

Awesome, thanks!

from hdf5r.

Hi @hhoeflin !

I see that branch bug/fix_empty_attrs_again has not yet been merged.

Might I inquire about the status of this fix?

The described issue has been a blocking issue for months on end now, and it's saddening to see that a fix for it exists but is simply not being merged / released.

Happy to help out with whatever you would need help with!

Kind regards,
Robrecht

from hdf5r.

Hey @hhoeflin !

Thanks for releasing hdf5r 1.3.11! 🙇 🙇 🙇

I can confirm this has fixed my issue.

from hdf5r.