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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gumbo): use C struct pointer directly instead of wrapping #2290

Merged
merged 1 commit into from Jul 12, 2021

Conversation

flavorjones
Copy link
Member

What problem is this PR intended to solve?

#2276 reports some valgrind warnings coming from the object used to wrap gumbo parse arguments.

This PR avoids the creation of an object and passes the C struct pointer directly, casting it as a VALUE when necessary.

Have you included adequate test coverage?

I spent a few hours trying to reliably reproduce the #2276 warning and couldn't. 馃し

Does this change affect the behavior of either the C or the Java implementations?

No functional changes.

This should avoid the stack-address valgrind warning from #2276
@stevecheckoway
Copy link
Contributor

I wrote this code and it wouldn't shock me to learn that I misunderstood the requirements of these functions. The problem that I was trying to solve, if I recall correctly, was that an exception could be thrown in the middle of the parse which would leak memory.

I opted not to take the approach in #2276 because it wasn't clear to me what the Ruby runtime was going to do with the VALUE. If it's expecting it to be a real VALUE and trying to access the underlying components of it, then treating a pointer as a VALUE seems bad.

I would not expect the stack to unwind beyond the rb_ensure() before calling the second function. And I don't see why the runtime would hold on to the passed in VALUE beyond those two function calls. Maybe this needs to be paired with rb_rescue() to work correctly?

@larskanis
Copy link
Member

As described in #2276 (comment) this looks good to me.

@stevecheckoway
Copy link
Contributor

Oh, I see what happened. I wrote a comment in the PR, not in the linked issue and mixed up the numbers. Sorry for the confusion.

This also looks good to me after @larskanis's explanation. It'd be great if there were documentation for the Ruby API, but indeed casting a pointer to a VALUE for use in rb_ensure does seem like it works. E.g., https://github.com/ruby/ruby/blob/d09988502ed75cae65b787865465361b675cf1ee/ext/monitor/monitor.c#L180

@flavorjones flavorjones merged commit 9e6e6ac into main Jul 12, 2021
@flavorjones flavorjones deleted the 2276-gumbo-parseargs branch July 12, 2021 17:50
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.

None yet

3 participants