Skip to content

fixes: #345 InvokeMethodWithReply segfault #382

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

Merged
merged 2 commits into from
Apr 2, 2020
Merged

Conversation

pchampio
Copy link
Member

@pchampio pchampio commented Apr 2, 2020

The callback function used to retrieve the value of the reply was
free-ed by the Golang GC.
This was quite hard to fix, had to uses runtime.KeepAlive on a struct
that contains the callback function.
runtime.KeepAlive, ensures that the object is not freed, and its
finalizer is not run, before the point in the program where KeepAlive is
called. Since the returned value is obtained with the callback function,
I used the defer keyword to make the function as not used for the gc
once we have the reply value.

The callback function used to retrieve the value of the reply was
free-ed by the Golang GC.
This was quite hard to fix, had to uses runtime.KeepAlive on a struct
that contains the callback function.
runtime.KeepAlive, ensures that the object is not freed, and its
finalizer is not run, before the point in the program where KeepAlive is
called. Since the returned value is obtained with the callback function,
I used the `defer` keyword to make the function as not used for the gc
once we have the reply value.
@pchampio pchampio requested a review from GeertJohan April 2, 2020 13:28
@pchampio pchampio linked an issue Apr 2, 2020 that may be closed by this pull request
Copy link
Member

@GeertJohan GeertJohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find and fix, LGTM!

@GeertJohan
Copy link
Member

The only thing I would add is that the "trick the panic" comment is not really relevant as the bug is fixed. It would be good to add that explanation in the issue for posterity, but not in the code.

@pchampio
Copy link
Member Author

pchampio commented Apr 2, 2020

The "trick the panic" message wasn't for this specific issue.
I've added it because since the start, we shouldn't be able to pass a Go struct to the C layer.
We made extensive use of this trick (which is to basically make the Go runtime thinks we aren't passing a Go strut, but in reality, we are.).
At some point, Golang might get smart enough and managed to throw the error.

I agree, this isn't pretty in the code.

@pchampio pchampio merged commit a526e1d into master Apr 2, 2020
@pchampio pchampio deleted the fix_seg_fault branch April 2, 2020 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

InvokeMethodWithReply throw exception
2 participants