Avoid writing Go pointers to non Go memory #4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before, we could pass in a *C.double to e.g. cArrToIntSlice, and we'd be none the wiser. Now, that is not possible without some dubious casting into and out of unsafe.Pointer
"Free all C pointers" is somewhat of a misnomer, as holes will leak if you run NewTriangulateIO yourself, because of a double free issue I haven't properly fixed. There are probably tricks one can use to ensure data isn't double free'd. Reference counters are probably the easiest to use, but it feels a bit ugly to wrap that atop of this fix (and also this library). Also, there are fields here that I haven't experimented with, so it's quite likely that using those may result in double frees.
|
Hi @hypirion, glad you found the wrapper useful. Thanks for the fix and the unit test! Your changes/additions look very sensible to me. I haven't been using Go for a while now, so I have to set things up to test this out. I will merge your PR sometime this week when I get some time. |
|
Hi @pradeep-pyro, we are getting something the same as @hypirion described. Are you planning to merge the fix? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi, thank you for this wrapper! I've been using it on and off for a while, and for me it seems to work as intended for the most part. However, in some situations, I ended up with segmentation faults without any good explanation why.
Yesterday I decided to take a look by using cgo and gc debug flags, and found out that this wrapper passes in Go pointers to C. Passing Go pointers to C isn't allowed because Go's garbage collector may kick in at any time. I couldn't see any other backwards compatible way to do this except copying the slice contents into C memory.
I also did a best effort attempt to clean up C memory after use, although I am not aware of how all the options work. At least one of the pointers –
holelist- were copied to the out struct, and that caused issues with double freeing. I wouldn't be surprised if this could be a problem with other allocated memory blocks. There's no double free with the default helper functions, but I guess other flags/options could trigger return values that would be double free'd. This would be a lot simpler if the functions calling triangle either gathered all input/output in a single struct, or did not expose cleanup. However, that would break backwards compatibility.Finally, Go complained so much about no Go module file that I added an empty one – hopefully that's fine.