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

Fix undefined behavior due to XPath.SetResolver taking address of GC-able interface #89

Closed
wants to merge 1 commit into from

Conversation

magnushiie
Copy link

XPath.GetResolver takes address of it VariableScope interface parameter and passes that to cgo function for storage in non-GC memory - if this interface is collected by the GC before XPath is evaluated, SIGSEGV or other bad things can occur. This PR stores the VariableScope interface in the XPath struct.

@magnushiie
Copy link
Author

As mentioned in #88, PR #79 by @grahamking already contains a fix by @kisielk almost identical to mine (didn't know to check it because the title of the PR is "Fixes for Go 1.4" - I pretty much started with Go 1.5, so didn't think it was relevant). I'm keeping this PR open for now - seems #79 is somewhat controversial (according to @twojtasz comment) but I don't think this one is.

@jbowtie jbowtie mentioned this pull request Feb 21, 2016
@thanzen
Copy link

thanzen commented Feb 24, 2016

@jbowtie, is this going to get merged?

@jbowtie
Copy link
Contributor

jbowtie commented Feb 24, 2016

@thanzen I would hope so, but I don't have commit rights. The ThomsonReutersEikon/gokogiri fork incorporates a nearly identical fix (see #79), so you may need to switch to that fork until there's an active maintainer again.

@thanzen
Copy link

thanzen commented Feb 25, 2016

@jbowtie I see. Thanks.

@magnushiie
Copy link
Author

Closing this as seems there is no interest by the maintainers.

@magnushiie magnushiie closed this Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants