Merge pull request #2309 from Shopify/run-valgrind-on-ci

Run Valgrind on CI and fix memory leaks

Author: soutaro

.github/workflows/ruby.yml

@@ -80,3 +80,29 @@ jobs:
       - name: Run test
         run: |
           bundle exec rake ${{ matrix.job }}
+  valgrind:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - uses: ruby/setup-ruby@v1
+        with:
+          ruby-version: "3.4"
+          bundler-cache: none
+      - name: Set working directory as safe
+        run: git config --global --add safe.directory $(pwd)
+      - name: Set up permission
+        run: chmod -R o-w /opt/hostedtoolcache/Ruby
+      - name: Install dependencies
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y libdb-dev curl autoconf automake m4 libtool python3 valgrind
+      - name: Update rubygems & bundler
+        run: |
+          ruby -v
+          gem update --system
+      - name: bin/setup
+        run: |
+          bin/setup
+      - run: bundle exec rake test:valgrind
+        env:
+          RUBY_FREE_AT_EXIT: 1

Gemfile

@@ -37,6 +37,7 @@ group :profilers do
   gem 'stackprof'
   gem 'memory_profiler'
   gem 'benchmark-ips'
+  gem "ruby_memcheck", platform: :ruby
 end
 
 # Test gems

Gemfile.lock

@@ -60,13 +60,17 @@ GEM
     logger (1.6.6)
     marcel (1.0.4)
     memory_profiler (1.1.0)
+    mini_portile2 (2.8.8)
     minitest (5.25.4)
     mutex_m (0.3.0)
     net-protocol (0.2.2)
       timeout
     net-smtp (0.5.1)
       net-protocol
     nkf (0.2.0)
+    nokogiri (1.18.3)
+      mini_portile2 (~> 2.8.2)
+      racc (~> 1.4)
     ostruct (0.6.1)
     parallel (1.26.3)
     parser (3.3.7.1)
@@ -126,6 +130,8 @@ GEM
       lint_roller (~> 1.1)
       rubocop (~> 1.72, >= 1.72.1)
     ruby-progressbar (1.13.0)
+    ruby_memcheck (3.0.1)
+      nokogiri
     securerandom (0.4.1)
     stackprof (0.2.27)
     steep (1.9.4)
@@ -193,6 +199,7 @@ DEPENDENCIES
   rubocop
   rubocop-on-rbs
   rubocop-rubycw
+  ruby_memcheck
   stackprof
   steep
   tempfile

Rakefile

@@ -11,14 +11,24 @@ bin = File.join(__dir__, "bin")
 
 Rake::ExtensionTask.new("rbs_extension")
 
-Rake::TestTask.new(:test => :compile) do |t|
+test_config = lambda do |t|
   t.libs << "test"
   t.libs << "lib"
   t.test_files = FileList["test/**/*_test.rb"].reject do |path|
     path =~ %r{test/stdlib/}
   end
 end
 
+Rake::TestTask.new(test: :compile, &test_config)
+
+unless Gem.win_platform?
+  require "ruby_memcheck"
+
+  namespace :test do
+    RubyMemcheck::TestTask.new(valgrind: :compile, &test_config)
+  end
+end
+
 multitask :default => [:test, :stdlib_test, :typecheck_test, :rubocop, :validate, :test_doc]
 
 task :lexer do

ext/rbs_extension/parserstate.c

@@ -363,6 +363,7 @@ parserstate *alloc_parser(VALUE buffer, lexstate *lexer, int start_pos, int end_
 
   if (!NIL_P(variables)) {
     if (!RB_TYPE_P(variables, T_ARRAY)) {
+      free_parser(parser);
       rb_raise(rb_eTypeError,
                "wrong argument type %"PRIsVALUE" (must be array or nil)",
                rb_obj_class(variables));
@@ -387,11 +388,24 @@ parserstate *alloc_parser(VALUE buffer, lexstate *lexer, int start_pos, int end_
   return parser;
 }
 
+void free_typevar_tables(id_table *table) {
+  while (table != NULL) {
+    id_table *next = table->next;
+    if (table->ids != NULL) {
+      free(table->ids);
+    }
+    free(table);
+    table = next;
+  }
+}
+
 void free_parser(parserstate *parser) {
   free(parser->lexstate);
   if (parser->last_comment) {
     free_comment(parser->last_comment);
   }
+
+  free_typevar_tables(parser->vars);
   rbs_constant_pool_free(&parser->constant_pool);
   free(parser);
 }

test/rbs/test/runtime_test_test.rb

@@ -20,6 +20,10 @@ def test_runtime_test_with_sample_size
   end
 
   def test_runtime_test_error_with_invalid_sample_size
+    # Skip this test if running under Valgrind because `RUBY_FREE_AT_EXIT` has a bug.
+    # See: https://bugs.ruby-lang.org/issues/21173
+    omit if ENV["RUBY_MEMCHECK_RUNNING"]
+
     string_err_msg = refute_test_success(other_env: {"RBS_TEST_SAMPLE_SIZE" => 'yes'})
     assert_match(/E, .+ ERROR -- rbs: Sample size should be a positive integer: `.+`\n/, string_err_msg)
 
@@ -93,6 +97,9 @@ def refute_test_success(other_env: {}, rbs_content: nil, ruby_content: nil)
   end
 
   def test_test_target
+    # Skip this test if running under Valgrind because `RUBY_FREE_AT_EXIT` has a bug.
+    # See: https://bugs.ruby-lang.org/issues/21173
+    omit if ENV["RUBY_MEMCHECK_RUNNING"]
     output = refute_test_success(other_env: { "RBS_TEST_TARGET" => nil })
     assert_match "test/setup handles the following environment variables:", output
   end