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 memory leaks #2345

Merged
merged 4 commits into from
Oct 22, 2021
Merged

fix memory leaks #2345

merged 4 commits into from
Oct 22, 2021

Conversation

flavorjones
Copy link
Member

What problem is this PR intended to solve?

Playing with ruby_memcheck led to finding a few easy-to-fix memory leaks.

  • Document#canonicalize leaked the namespaces array
  • Document#canonicalize leaked the output buffer when argument type errors were raised
  • iconv encoding handlers were leaked
  • Strings passed into xpath custom handlers were leaked

Have you included adequate test coverage?

#2344 is a parallel effort to automate the leak checking that I ran on the existing test suite to find these.

in xml_xpath_context.c, both functions `evaluate` and
`Nokogiri_marshal_xpath_funcall_and_return_values` had code to convert
xpath objects to Ruby objects.

`Nokogiri_marshal_xpath_funcall_and_return_values` had a memory leak
while `evaluate` did not.

I've extracted this logic to a single method, and called it from both
places, thereby eliminating the memory leak along with DRYing things up.

Also, I've added a bit of test coverage to some of the marshalling.
- namespaces array was not being freed
- argument type errors led to output buffer not being freed

Also note that I cleaned up this function a bit.
@flavorjones flavorjones added this to the v1.13.0 milestone Oct 22, 2021
@flavorjones flavorjones merged commit 3aadf16 into main Oct 22, 2021
@flavorjones flavorjones deleted the flavorjones-fix-memory-leaks branch October 22, 2021 18:00
flavorjones added a commit that referenced this pull request Oct 22, 2021
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.

1 participant