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

Prevent potential crashes in HDF5 #4069

Merged
merged 2 commits into from Sep 16, 2017

Conversation

Projects
None yet
3 participants
@saudet
Copy link
Member

saudet commented Sep 14, 2017

What changes were proposed in this pull request?

When calling .asCommonFG(), the original objects may get out of scope, and garbage collected prematurely, causing corruption in native memory. Here we make sure to deallocate them manually at the right time.

How was this patch tested?

All unit tests pass, manually loading a Darknet model works.

@saudet saudet requested a review from maxpumperla Sep 14, 2017

@maxpumperla
Copy link
Contributor

maxpumperla left a comment

LGTM, really cool!

@huitseeker
Copy link
Contributor

huitseeker left a comment

Very neat. a few comments.

return groupArray;
}

private void closeGroups(hdf5.Group[] groupArray) {

This comment has been minimized.

@huitseeker

huitseeker Sep 14, 2017

Contributor

What are your thoughts on

hdf5.Group thisGroup = groupArray[i];
thisGroup._close();

?

This comment has been minimized.

@saudet

saudet Sep 14, 2017

Member

Examples from the HDF5 documentation uses the delete operator and that's called with dellocate()...

This comment has been minimized.

@huitseeker

This comment has been minimized.

@saudet

saudet Sep 15, 2017

Member

That refers to the C API. The C++ API seems to be a bit wonky...

This comment has been minimized.

@huitseeker

huitseeker Sep 15, 2017

Contributor

Haha, fair enough.

@@ -61,6 +61,21 @@ public Hdf5Archive(String archiveFilename) {
this.file = new hdf5.H5File(archiveFilename, H5F_ACC_RDONLY);
}

private hdf5.Group[] openGroups(String ... groups) {

This comment has been minimized.

@huitseeker

huitseeker Sep 14, 2017

Contributor

Neat. How do you see the reference pattern to this array evolve? If it's always the last element being accessed, could we make it a stack and save ourselves the potential OutOfBounds issues?

This comment has been minimized.

@saudet

saudet Sep 14, 2017

Member

Don't know, it hasn't really evolved, maybe that's all we'll ever need from HDF5, @maxpumperla ? @turambar ?

@saudet

This comment has been minimized.

Copy link
Member

saudet commented Sep 15, 2017

Hum, I'm getting other kinds of crashes now sometimes. It might be that HDF5 needs to have everything deallocated in order, including everything called in the other methods...

@saudet

This comment has been minimized.

Copy link
Member

saudet commented Sep 15, 2017

Ok, that seems to do it. Not getting any more random crashes for now...

@huitseeker
Copy link
Contributor

huitseeker left a comment

Bold behavior, but should not be harmful. Thanks!

@saudet

This comment has been minimized.

Copy link
Member

saudet commented Sep 16, 2017

Eh, that's why we should do the least possible in C/C++ :) Thanks!

@saudet saudet merged commit fad874b into master Sep 16, 2017

@maxpumperla maxpumperla deleted the sa_kerasfix branch Sep 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment