Merge pull request #22 from Shopify/pz-auto-detect-binary

Automatically detect Ruby native extensions

.github/workflows/test.yml

@@ -7,7 +7,6 @@ jobs:
       fail-fast: false
       matrix:
         entry:
-          - { ruby: '2.7', allowed-failure: false }
           - { ruby: '3.0', allowed-failure: false }
           - { ruby: '3.1', allowed-failure: false }
           - { ruby: '3.2', allowed-failure: false }

README.md

@@ -84,12 +84,6 @@ The easiest way to use this gem is to use it on your test suite (minitest or RSp
       require "ruby_memcheck/rspec/rake_task"
       ```
 
-1. Configure the gem by calling `RubyMemcheck.config`. You must pass it your binary name. This is the same value you passed into `create_makefile` in your `extconf.rb` file. Make sure this value is correct or it will filter out almost everything as a false-positive!
-
-    ```ruby
-    RubyMemcheck.config(binary_name: "your_binary_name")
-    ```
-
 1. Setup the test task for your test framework.
     - **minitest**
 
@@ -145,18 +139,17 @@ The easiest way to use this gem is to use it on your test suite (minitest or RSp
 
 ## Configuration
 
-When you run `RubyMemcheck.config`, you are creating a default `RubyMemcheck::Configuration`. By default, the Rake tasks for minitest and RSpec will use this configuration. You can also manually pass in a `Configuration` object as the first argument to the constructor of `RubyMemcheck::TestTask` or `RubyMemcheck::RSpec::RakeTask` to use a different `Configuration` object rather than the default one.
+If you want to override any of the default configurations you can call `RubyMemcheck.config` after `require "ruby_memcheck"`. This will create a default `RubyMemcheck::Configuration`. By default, the Rake tasks for minitest and RSpec will use this configuration. You can also manually pass in a `Configuration` object as the first argument to the constructor of `RubyMemcheck::TestTask` or `RubyMemcheck::RSpec::RakeTask` to use a different `Configuration` object rather than the default one.
 
 `RubyMemcheck::Configuration` accepts a variety of keyword arguments. Here are all the arguments:
 
-- `binary_name`: Required. The binary name of your native extension gem. This is the same value you passed into `create_makefile` in your `extconf.rb` file.
 - `ruby`: Optional. The command to run to invoke Ruby. Defaults to the Ruby that is currently being used.
 - `valgrind`: Optional. The command to run to invoke Valgrind. Defaults to the string `"valgrind"`.
 - `valgrind_options`: Optional. Array of options to pass into Valgrind. This is only present as an escape hatch, so avoid using it. This may be deprecated or removed in future versions.
 - `valgrind_suppressions_dir`: Optional. The string path of the directory that stores suppression files for Valgrind. See the [`Suppression files`](#suppression-files) section for more details. Defaults to `suppressions`.
 - `valgrind_generate_suppressions`: Optional. Whether suppressions should also be outputted along with the errors. the [`Suppression files`](#suppression-files) section for more details. Defaults to `false`.
 - `skipped_ruby_functions`: Optional. Ruby functions that are ignored because they are considered a call back into Ruby. This is only present as an escape hatch, so avoid using it. If you find another Ruby function that is a false positive because it calls back into Ruby, please send a patch into this repo. Otherwise, use a Valgrind suppression file.
-- `valgrind_xml_dir`: Optional. The directory to store temporary XML files for Valgrind. It defaults to a temporary directory. This is present for development debugging, so you shouldn't have to use it.
+- `temp_dir`: Optional. The directory to store temporary files. It defaults to a temporary directory. This is present for development debugging, so you shouldn't have to use it.
 - `output_io`: Optional. The `IO` object to output Valgrind errors to. Defaults to standard error.
 - `filter_all_errors`: Optional. Whether to filter all kinds of Valgrind errors (not just memory leaks). This feature should only be used if you're encountering a large number of illegal memory accesses coming from Ruby. If you need to use this feature, you may have found a bug inside of Ruby. Consider reporting it to the [Ruby bug tracker](https://bugs.ruby-lang.org/projects/ruby-master/issues/new). Defaults to `false`.
 

Rakefile

@@ -11,9 +11,16 @@ Rake::TestTask.new(test: "test:compile") do |t|
 end
 
 namespace :test do
-  Rake::ExtensionTask.new("ruby_memcheck_c_test") do |ext|
+  Rake::ExtensionTask.new("ruby_memcheck_c_test_one") do |ext|
     ext.ext_dir = "test/ruby_memcheck/ext"
     ext.lib_dir = "test/ruby_memcheck/ext"
+    ext.config_script = "extconf_one.rb"
+  end
+
+  Rake::ExtensionTask.new("ruby_memcheck_c_test_two") do |ext|
+    ext.ext_dir = "test/ruby_memcheck/ext"
+    ext.lib_dir = "test/ruby_memcheck/ext"
+    ext.config_script = "extconf_two.rb"
   end
 end
 

lib/ruby_memcheck.rb

@@ -19,11 +19,7 @@ def config(**opts)
     end
 
     def default_configuration
-      unless @default_configuration
-        raise "RubyMemcheck is not configured with a default configuration. "\
-          "Please run RubyMemcheck.config before using it."
-      end
-      @default_configuration
+      @default_configuration ||= config
     end
   end
 end

lib/ruby_memcheck/configuration.rb

@@ -31,33 +31,34 @@ class Configuration
       /\Arb_yield/,
     ].freeze
 
-    attr_reader :binary_name
     attr_reader :ruby
     attr_reader :valgrind
     attr_reader :valgrind_options
     attr_reader :valgrind_suppression_files
     attr_reader :valgrind_generate_suppressions
     attr_reader :skipped_ruby_functions
-    attr_reader :valgrind_xml_dir
+    attr_reader :temp_dir
+    attr_reader :loaded_features_file
     attr_reader :output_io
     attr_reader :filter_all_errors
 
     alias_method :valgrind_generate_suppressions?, :valgrind_generate_suppressions
     alias_method :filter_all_errors?, :filter_all_errors
 
     def initialize(
-      binary_name:,
+      binary_name: nil,
       ruby: FileUtils::RUBY,
       valgrind: DEFAULT_VALGRIND,
       valgrind_options: DEFAULT_VALGRIND_OPTIONS,
       valgrind_suppressions_dir: DEFAULT_VALGRIND_SUPPRESSIONS_DIR,
       valgrind_generate_suppressions: false,
       skipped_ruby_functions: DEFAULT_SKIPPED_RUBY_FUNCTIONS,
-      valgrind_xml_dir: Dir.mktmpdir,
+      temp_dir: Dir.mktmpdir,
       output_io: $stderr,
       filter_all_errors: false
     )
-      @binary_name = binary_name
+      warn("ruby_memcheck: binary_name is no longer required for configuration") if binary_name
+
       @ruby = ruby
       @valgrind = valgrind
       @valgrind_options = valgrind_options
@@ -69,18 +70,18 @@ def initialize(
       @output_io = output_io
       @filter_all_errors = filter_all_errors
 
-      if valgrind_xml_dir
-        valgrind_xml_dir = File.expand_path(valgrind_xml_dir)
-        FileUtils.mkdir_p(valgrind_xml_dir)
-        @valgrind_xml_dir = valgrind_xml_dir
-        @valgrind_options += [
-          "--xml=yes",
-          # %p will be replaced with the PID
-          # This prevents forking and shelling out from generating a corrupted XML
-          # See --log-file from https://valgrind.org/docs/manual/manual-core.html
-          "--xml-file=#{File.join(valgrind_xml_dir, "%p.out")}",
-        ]
-      end
+      temp_dir = File.expand_path(temp_dir)
+      FileUtils.mkdir_p(temp_dir)
+      @temp_dir = temp_dir
+      @valgrind_options += [
+        "--xml=yes",
+        # %p will be replaced with the PID
+        # This prevents forking and shelling out from generating a corrupted XML
+        # See --log-file from https://valgrind.org/docs/manual/manual-core.html
+        "--xml-file=#{File.join(temp_dir, "%p.xml")}",
+      ]
+
+      @loaded_features_file = Tempfile.create("", @temp_dir)
     end
 
     def command(*args)
@@ -112,7 +113,9 @@ def get_valgrind_suppression_files(dir)
       (0..3).reverse_each { |i| versions << full_ruby_version.split(".")[0, i].join(".") }
       versions << RUBY_ENGINE
 
-      versions.map { |version| Dir[File.join(dir, "#{version}.supp")] }.flatten
+      versions.map do |version|
+        Dir[File.join(dir, "#{version}.supp")]
+      end.flatten
     end
   end
 end

lib/ruby_memcheck/frame.rb

@@ -2,10 +2,12 @@
 
 module RubyMemcheck
   class Frame
-    attr_reader :configuration, :fn, :obj, :file, :line
+    attr_reader :configuration, :loaded_binaries, :fn, :obj, :file, :line
 
-    def initialize(configuration, frame_xml)
+    def initialize(configuration, loaded_binaries, frame_xml)
       @configuration = configuration
+      @loaded_binaries = loaded_binaries
+
       @fn = frame_xml.at_xpath("fn")&.content
       @obj = frame_xml.at_xpath("obj")&.content
       # file and line may not be available
@@ -24,8 +26,14 @@ def in_ruby?
     def in_binary?
       return false unless obj
 
-      binary_name_without_ext = File.join(File.dirname(obj), File.basename(obj, ".*"))
-      binary_name_without_ext.end_with?(File.join("", configuration.binary_name))
+      loaded_binaries.include?(obj)
+    end
+
+    def binary_init_func?
+      return false unless in_binary?
+
+      binary_name = File.basename(obj, ".*")
+      fn == "Init_#{binary_name}"
     end
 
     def to_s

lib/ruby_memcheck/rspec/rake_task.rb

@@ -5,9 +5,8 @@
 module RubyMemcheck
   module RSpec
     class RakeTask < ::RSpec::Core::RakeTask
-      include TestTaskReporter
-
       attr_reader :configuration
+      attr_reader :reporter
 
       def initialize(*args)
         @configuration =
@@ -23,15 +22,14 @@ def initialize(*args)
       def run_task(verbose)
         error = nil
 
-        begin
+        @reporter = TestTaskReporter.new(configuration)
+        @reporter.run_ruby_with_valgrind do
           # RSpec::Core::RakeTask#run_task calls Kernel.exit on failure
           super
         rescue SystemExit => e
           error = e
         end
 
-        report_valgrind_errors
-
         raise error if error
       end
 

lib/ruby_memcheck/stack.rb

@@ -4,30 +4,28 @@ module RubyMemcheck
   class Stack
     attr_reader :configuration, :frames
 
-    def initialize(configuration, stack_xml)
+    def initialize(configuration, loaded_binaries, stack_xml)
       @configuration = configuration
-      @frames = stack_xml.xpath("frame").map { |frame| Frame.new(configuration, frame) }
+      @frames = stack_xml.xpath("frame").map { |frame| Frame.new(configuration, loaded_binaries, frame) }
     end
 
     def skip?
       in_binary = false
 
       frames.each do |frame|
-        fn = frame.fn
-
         if frame.in_ruby?
           # If a stack from from the binary was encountered first, then this
           # memory leak did not occur from Ruby
           unless in_binary
             # Skip this stack because it was called from Ruby
-            return true if configuration.skipped_ruby_functions.any? { |r| r.match?(fn) }
+            return true if configuration.skipped_ruby_functions.any? { |r| r.match?(frame.fn) }
           end
         elsif frame.in_binary?
           in_binary = true
 
           # Skip the Init function because it is only ever called once, so
           # leaks in it cannot cause memory bloat
-          return true if fn == "Init_#{configuration.binary_name}"
+          return true if frame.binary_init_func?
         end
       end
 

lib/ruby_memcheck/test_helper.rb

@@ -1,3 +1,9 @@
 # frozen_string_literal: true
 
-at_exit { GC.start }
+at_exit do
+  File.open(ENV["RUBY_MEMCHECK_LOADED_FEATURES_FILE"], "w") do |f|
+    f.write($LOADED_FEATURES.join("\n"))
+  end
+
+  GC.start
+end

lib/ruby_memcheck/test_task.rb

@@ -2,9 +2,8 @@
 
 module RubyMemcheck
   class TestTask < Rake::TestTask
-    include TestTaskReporter
-
     attr_reader :configuration
+    attr_reader :reporter
 
     def initialize(*args)
       @configuration =
@@ -19,8 +18,13 @@ def initialize(*args)
 
     def ruby(*args, **options, &block)
       command = configuration.command(args)
+
+      @reporter = TestTaskReporter.new(configuration)
+
+      @reporter.setup
+
       sh(command, **options) do |ok, res|
-        report_valgrind_errors
+        @reporter.report_valgrind_errors
 
         yield ok, res if block_given?
       end

lib/ruby_memcheck/test_task_reporter.rb

@@ -1,53 +1,79 @@
 # frozen_string_literal: true
 
 module RubyMemcheck
-  module TestTaskReporter
+  class TestTaskReporter
     VALGRIND_REPORT_MSG = "Valgrind reported errors (e.g. memory leak or use-after-free)"
 
+    attr_reader :configuration
     attr_reader :errors
 
-    private
+    def initialize(configuration)
+      @configuration = configuration
+      @loaded_binaries = nil
+    end
+
+    def run_ruby_with_valgrind(&block)
+      setup
+      yield
+      report_valgrind_errors
+    end
+
+    def setup
+      ENV["RUBY_MEMCHECK_LOADED_FEATURES_FILE"] = File.expand_path(configuration.loaded_features_file)
+      ENV["RUBY_MEMCHECK_RUNNING"] = "1"
+    end
 
     def report_valgrind_errors
-      if configuration.valgrind_xml_dir
-        xml_files = valgrind_xml_files
-        parse_valgrind_output(xml_files)
-        remove_valgrind_xml_files(xml_files)
-
-        unless errors.empty?
-          output_valgrind_errors
-          raise VALGRIND_REPORT_MSG
-        end
+      parse_valgrind_output
+      remove_valgrind_xml_files
+
+      unless errors.empty?
+        output_valgrind_errors
+        raise VALGRIND_REPORT_MSG
       end
     end
 
+    private
+
+    def loaded_binaries
+      return @loaded_binaries if @loaded_binaries
+
+      loaded_features = File.readlines(configuration.loaded_features_file, chomp: true)
+      @loaded_binaries = loaded_features.keep_if do |feat|
+        # Keep only binaries (ignore Ruby files).
+        File.extname(feat) == ".so"
+      end.freeze
+
+      @loaded_binaries
+    end
+
     def valgrind_xml_files
-      Dir[File.join(configuration.valgrind_xml_dir, "*")]
+      @valgrind_xml_files ||= Dir[File.join(configuration.temp_dir, "*.xml")].freeze
     end
 
-    def parse_valgrind_output(xml_files)
+    def parse_valgrind_output
       require "nokogiri"
 
       @errors = []
 
-      xml_files.each do |file|
+      valgrind_xml_files.each do |file|
         reader = Nokogiri::XML::Reader(File.open(file)) do |config| # rubocop:disable Style/SymbolProc
           config.huge
         end
         reader.each do |node|
           next unless node.name == "error" && node.node_type == Nokogiri::XML::Reader::TYPE_ELEMENT
 
           error_xml = Nokogiri::XML::Document.parse(node.outer_xml).root
-          error = ValgrindError.new(configuration, error_xml)
+          error = ValgrindError.new(configuration, loaded_binaries, error_xml)
           next if error.skip?
 
           @errors << error
         end
       end
     end
 
-    def remove_valgrind_xml_files(xml_files)
-      xml_files.each do |file|
+    def remove_valgrind_xml_files
+      valgrind_xml_files.each do |file|
         File.delete(file)
       end
     end