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

Don't assert if trying to open truncated file from memory. #2089

Merged
merged 5 commits into from
Mar 14, 2022

Conversation

d70-t
Copy link
Contributor

@d70-t d70-t commented Aug 24, 2021

This is a follow-up to Unidata/netcdf4-python#1122.

I am downloading a bunch of netCDF files which I keep in memory. I don't know if these files are complete or truncated for some reason. In order to check if those files are correct, I try to open them, but this leads to a failing assertion. This PR changes the assertion to an error condition which can be handled by the calling code.

Previously, I only got an error condition when opening a file on the filesystem, but got an assertion when opening the file from memory.

This test currently fails.
When trying to open an in-memory file which is only available partially
(i.e. by accidental truncation), then the library should fail with an
error in stead of an assertation, such that user code can react on this.
If a memory backed file is closed due to an aborted opening (i.e. a
broken file was attempted to be opened), udata may not be set. In this
case, the assertation checking for udata should not be triggered.
@d70-t d70-t requested a review from WardF as a code owner August 24, 2021 15:23
@DennisHeimbigner
Copy link
Collaborator

Looks ok to me.

I think udata will be freed by the calback as soon as the callbacks are
set and H5Pclose is called, so the backward link should be set up at
that place.

Without this change, local_image_free may be called without a proper
reference to udata, which may raise an assertion:
assert(udata->fapl_image_ptr == udata->app_image_ptr);
while trying to open a broken file.
@d70-t
Copy link
Contributor Author

d70-t commented Aug 26, 2021

The remaining error in AppVeyour seems to be a CMake problem related to ncdap_test. I'd think that I didn't touch anything related to this. Do you know this error or where it may come from?

I've currently no access to a windows machine, so I don't quite know how to iterate on this further.

@DennisHeimbigner
Copy link
Collaborator

I think I fixed the appveyor problem.
In ncdap_test/CMakeLists.txt change lines 38 from

SET_TESTS_PROPERTIES(ncdap_tst_ncdap3 ncdap_testpathcvt PROPERTIES RUN_SERIAL TRUE)

to

SET_TESTS_PROPERTIES(ncdap_tst_ncdap3 PROPERTIES RUN_SERIAL TRUE)
SET_TESTS_PROPERTIES(ncdap_testpathcvt PROPERTIES RUN_SERIAL TRUE)

@WardF
Copy link
Member

WardF commented Aug 26, 2021

I am looking into the appveyor issue now, it is unrelated to your change!

@WardF
Copy link
Member

WardF commented Aug 26, 2021

I believe the appveyor check may be related to how hdf5 reports it’s version information on windows vs *nix systems. I will take a look.

@d70-t
Copy link
Contributor Author

d70-t commented Sep 2, 2021

🎉 @WardF you fixes are successful!

@WardF WardF added this to the 4.9.0 milestone Mar 10, 2022
@WardF WardF self-assigned this Mar 10, 2022
@WardF WardF merged commit 8152a51 into Unidata:main Mar 14, 2022
@d70-t d70-t deleted the open_mem_truncated_file branch March 14, 2022 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants