Fix memory leaks found by ruby_memcheck #105
Merged
Conversation
This file contains 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
Closed
See #104 Specifically, ensure we delete the previous input inside an RE2::Scanner before replacing it and check whether inputs are strings as early as possible to avoid raising an exception after allocating memory. Thanks to @peterzhu2118 for both authoring ruby_memcheck and helping find the source of these leaks.
mudge
force-pushed
the
plug-memory-leaks
branch
3 times, most recently
from
064d49a to
b37b07a
Compare
stanhu
reviewed
Sep 22, 2023
mudge
force-pushed
the
plug-memory-leaks
branch
from
b37b07a to
152d478
Compare
|
@stanhu can you please see if you can run this locally and if it also gives a clean ruby_memcheck run? |
See #104 When we raise an exception in re2_set_add, the memory used by the std::string used to store the error message is never freed. Fix this by ensuring it goes out of scope before we call rb_raise. However, we also need a copy of what is inside it to return to the user so we take a copy of its contents as a C string first. The current longest error message inside RE2 is 35 characters long so 100 characters gives us some headroom in case new releases of RE2 add longer messages. Thanks to @peterzhu2118 for both authoring ruby_memcheck and helping find the source of these leaks.
mudge
force-pushed
the
plug-memory-leaks
branch
from
152d478 to
befe65b
Compare
|
I'm getting a clean |
|
That doesn’t sound good. CI seems happy so far, can you reproduce it after clobbering, etc.? |
|
It doesn't seg fault on this branch, but when I apply these local changes I seem to be getting seg faults: diff --git a/Rakefile b/Rakefile
index 4712797..42517e6 100644
--- a/Rakefile
+++ b/Rakefile
@@ -6,6 +6,8 @@ require 'rake_compiler_dock'
require 'yaml'
require_relative 'ext/re2/recipes'
+require "ruby_memcheck"
+require "ruby_memcheck/rspec/rake_task"
CLEAN.include FileList['**/*{.o,.so,.dylib,.bundle}'],
FileList['**/extconf.h'],
@@ -129,4 +131,9 @@ task gem_build_path do
add_vendored_libraries
end
+RSpec::Core::RakeTask.new(spec: :compile)
+namespace :spec do
+ RubyMemcheck::RSpec::RakeTask.new(valgrind: :compile)
+end
+
task default: [:compile, :spec]
diff --git a/ext/re2/extconf.rb b/ext/re2/extconf.rb
index f8fd706..c8c6681 100644
--- a/ext/re2/extconf.rb
+++ b/ext/re2/extconf.rb
@@ -119,8 +119,8 @@ end
def build_extension(static_p = false)
# Enable optional warnings but disable deprecated register warning for Ruby 2.6 support
- $CFLAGS << " -Wall -Wextra -funroll-loops"
- $CPPFLAGS << " -Wno-register"
+ $CFLAGS << " -Wall -Wextra -funroll-loops -ggdb3 -g"
+ $CPPFLAGS << " -Wno-register -ggdb3"
# Pass -x c++ to force gcc to compile the test program
# as C++ (as it will end in .c by default).
@@ -395,11 +395,11 @@ def build_with_vendored_libraries
abseil_recipe, re2_recipe = load_recipes
process_recipe(abseil_recipe) do |recipe|
- recipe.configure_options += ['-DABSL_PROPAGATE_CXX_STD=ON', '-DCMAKE_CXX_VISIBILITY_PRESET=hidden']
+ recipe.configure_options += ['-DABSL_PROPAGATE_CXX_STD=ON', '-DCMAKE_CXX_VISIBILITY_PRESET=hidden', '-DCMAKE_CXXFLAGS=-ggdb3']
end
process_recipe(re2_recipe) do |recipe|
- recipe.configure_options += ["-DCMAKE_PREFIX_PATH=#{abseil_recipe.path}", '-DCMAKE_CXX_FLAGS=-DNDEBUG',
+ recipe.configure_options += ["-DCMAKE_PREFIX_PATH=#{abseil_recipe.path}", '-DCMAKE_CXX_FLAGS=-DNDEBUG', '-DCMAKE_CXX_FLAGS=-ggdb3',
'-DCMAKE_CXX_VISIBILITY_PRESET=hidden']
end
diff --git a/re2.gemspec b/re2.gemspec
index dbb64de..32e7a55 100644
--- a/re2.gemspec
+++ b/re2.gemspec
@@ -40,5 +40,6 @@ Gem::Specification.new do |s|
s.add_development_dependency("rake-compiler", "~> 1.2.1")
s.add_development_dependency("rake-compiler-dock", "~> 1.3.0")
s.add_development_dependency("rspec", "~> 3.2")
+ s.add_development_dependency("ruby_memcheck")
s.add_runtime_dependency("mini_portile2", "~> 2.8.4") # keep version in sync with extconf.rb
end |
|
|
|
Oh, right, I think my patch overrides the |
|
All working now. Nice fixes here. I was too quick to think these were false positives. |
|
The (slightly terrifying) lesson here is the havoc |
|
A quick skim of other places to investigate that might be lacking test coverage: creating an RE2::Set with options (therefore creating an RE2::Options object internally) but also an invalid anchor (which will raise). |
mudge
commented
Sep 23, 2023
| { | ||
| std::string err; | ||
| index = s->set->Add(regex, &err); | ||
| strncpy(msg, err.c_str(), sizeof(msg)); |
There was a problem hiding this comment.
We should also guard against the error message being truncated (and therefore not null terminated) with something like:
strncpy(msg, err.c_str(), sizeof(msg) - 1);
msg[sizeof(msg) - 1] = '\0';There was a problem hiding this comment.
Or better yet, use Ruby’s strlcpy: https://github.com/ruby/ruby/blob/ruby_2_6/missing/strlcpy.c
I tested this but there’s no leak. |
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.
See #104
Specifically:
rb_raise