From fc4f7170046df2c2195e8ee545c9ede8b9e4dc35 Mon Sep 17 00:00:00 2001 From: Thejus Paul Date: Mon, 14 Sep 2026 19:39:22 +0530 Subject: [PATCH 1/2] style: clear outstanding rubocop offences Three kinds of change, none of which alter behaviour. Most were autocorrections: semicolons, argument alignment, redundant returns, File.size(x).zero? to File.empty?, and similar. One overlong warning string is now split across two lines. Two methods were genuinely doing several jobs at once, and extracting the named steps out of them brought both back under the metrics thresholds on their own: collecting and copying installed binaries in the platform gem builder, and the path normalisation and safety guard in the extractor. The remaining offences were answered without changing code. Class length is now excluded for tests, matching how method length and ABC size are already excluded there, and the cache cascade in the extension builder patch carries a disable comment explaining why splitting it would be worse than leaving it long. --- .rubocop.yml | 2 + lib/prebake/async_publisher.rb | 3 +- lib/prebake/elf_inspector.rb | 7 +++- lib/prebake/ext_builder_patch.rb | 12 +++++- lib/prebake/extension_validator.rb | 2 +- lib/prebake/extractor.rb | 49 +++++++++++++---------- lib/prebake/platform_gem_builder.rb | 62 ++++++++++++++++------------- test/extension_validator_test.rb | 11 +++-- 8 files changed, 87 insertions(+), 61 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 9b47f07..41477a1 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -12,6 +12,8 @@ Style/StringLiterals: Metrics/ClassLength: Max: 150 + Exclude: + - "test/**/*" Metrics/AbcSize: Max: 30 diff --git a/lib/prebake/async_publisher.rb b/lib/prebake/async_publisher.rb index 472e8d7..902b683 100644 --- a/lib/prebake/async_publisher.rb +++ b/lib/prebake/async_publisher.rb @@ -76,7 +76,8 @@ def self.build_gem(spec, backend) end if !Prebake.libruby_available? && ElfInspector.libruby_needed_for_gem?(gem_path) - Logger.warn "Skipping push of #{cache_key}: binary requires libruby.so (dynamic Ruby) but this is a static Ruby build; binary would crash on this platform" + Logger.warn "Skipping push of #{cache_key}: binary requires libruby.so (dynamic Ruby) " \ + "but this is a static Ruby build; binary would crash on this platform" FileUtils.rm_f(gem_path) return nil end diff --git a/lib/prebake/elf_inspector.rb b/lib/prebake/elf_inspector.rb index 2904d26..10e32cd 100644 --- a/lib/prebake/elf_inspector.rb +++ b/lib/prebake/elf_inspector.rb @@ -10,7 +10,10 @@ module Prebake module ElfInspector def self.required_glibc_for_gem(gem_path) versions = [] - each_gem_binary(gem_path) { |binary| v = required_glibc(binary); versions << v if v } + each_gem_binary(gem_path) do |binary| + v = required_glibc(binary) + versions << v if v + end versions.empty? ? nil : versions.max_by { |v| Gem::Version.new(v) } rescue StandardError => e # Malformed gem, missing objdump, or I/O error — treat as unknown, let @@ -69,7 +72,7 @@ def self.run_objdump(path) Dir.mktmpdir("prebake-gem") do |tmpdir| Gem::Package.new(gem_path).extract_files(tmpdir) Dir.glob(File.join(tmpdir, "**/*.{so,bundle,dll}")).each do |binary| - next if File.symlink?(binary) || File.size(binary).zero? + next if File.symlink?(binary) || File.empty?(binary) yield binary end diff --git a/lib/prebake/ext_builder_patch.rb b/lib/prebake/ext_builder_patch.rb index 0931f93..ebeb7f5 100644 --- a/lib/prebake/ext_builder_patch.rb +++ b/lib/prebake/ext_builder_patch.rb @@ -11,6 +11,14 @@ module Prebake module ExtBuilderPatch + # The metrics cops are off for this method deliberately. It is a cascade of + # cache checks where every failure path falls through to `super`, the real + # source build. Extracting any stage would mean returning a sentinel for the + # caller to turn back into `super`, which reads worse than the cascade and + # risks silently installing a gem with no compiled binaries if a branch is + # mistranslated. + # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity + # rubocop:disable Metrics/MethodLength, Metrics/PerceivedComplexity def build_extensions return super unless @spec.extensions.any? return super unless Prebake.enabled? @@ -59,7 +67,7 @@ def build_extensions unless installed Prebake.backend.delete(cache_key) - return super + super end else super @@ -67,6 +75,8 @@ def build_extensions ensure FileUtils.rm_f(cached_gem) if cached_gem end + # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity + # rubocop:enable Metrics/MethodLength, Metrics/PerceivedComplexity private diff --git a/lib/prebake/extension_validator.rb b/lib/prebake/extension_validator.rb index e4be565..a278553 100644 --- a/lib/prebake/extension_validator.rb +++ b/lib/prebake/extension_validator.rb @@ -24,7 +24,7 @@ def self.validate(spec) nested.each do |binary| next if File.symlink?(binary) - next if File.size(binary).zero? + next if File.empty?(binary) dest = File.join(ext_dir, File.basename(binary)) next if File.exist?(dest) diff --git a/lib/prebake/extractor.rb b/lib/prebake/extractor.rb index 65b7d4e..f72b4ed 100644 --- a/lib/prebake/extractor.rb +++ b/lib/prebake/extractor.rb @@ -17,27 +17,9 @@ def self.install(gem_path, spec) # Copy only binary files (.so, .bundle, .dll) to extension_dir Dir.glob(File.join(tmpdir, "**/*.{so,bundle,dll}")).each do |binary| - # Reject symlinks and empty files - next if File.symlink?(binary) - next if File.size(binary).zero? - - # Verify path is within tmpdir (prevent traversal) - real_binary = File.realpath(binary) - real_tmpdir = File.realpath(tmpdir) - next unless real_binary.start_with?("#{real_tmpdir}/") - - relative = binary.sub("#{tmpdir}/", "") - - # Normalize paths from cached gems where binaries were packaged - # from gem_dir build artifacts or dirty extension_dirs. - # ext//.so → .so (build artifact) - # lib//.so → /.so (gem lib path) - # extension///.so → .so (extension_dir artifact) - relative = relative.sub(%r{\Aext/[^/]+/}, "") if relative.start_with?("ext/") - relative = relative.sub(%r{\Alib/}, "") if relative.start_with?("lib/") - relative = relative.sub(%r{\Aextensions?/[^/]+/[^/]+/}, "") if relative.start_with?("extension/", "extensions/") - - dest = File.join(spec.extension_dir, relative) + next unless safe_binary?(binary, tmpdir) + + dest = File.join(spec.extension_dir, normalized_path(binary, tmpdir)) FileUtils.mkdir_p(File.dirname(dest)) FileUtils.cp(binary, dest) extracted_count += 1 @@ -45,7 +27,7 @@ def self.install(gem_path, spec) end # Mark this extension_dir as prebake-managed for post-install validation - FileUtils.touch(File.join(spec.extension_dir, ".prebake")) if extracted_count > 0 + FileUtils.touch(File.join(spec.extension_dir, ".prebake")) if extracted_count.positive? Logger.info "Installed precompiled #{File.basename(gem_path)} " \ "(#{extracted_count} binary files)" @@ -55,5 +37,28 @@ def self.install(gem_path, spec) Logger.warn "Extraction failed for #{File.basename(gem_path)}: #{e.message}" raise end + + # Reject symlinks, empty files, and anything resolving outside tmpdir + # (path traversal via a crafted gem). + def self.safe_binary?(binary, tmpdir) + return false if File.symlink?(binary) + return false if File.empty?(binary) + + File.realpath(binary).start_with?("#{File.realpath(tmpdir)}/") + end + + # Normalize paths from cached gems where binaries were packaged from + # gem_dir build artifacts or dirty extension_dirs. + # ext//.so → .so (build artifact) + # lib//.so → /.so (gem lib path) + # extension///.so → .so (extension_dir artifact) + def self.normalized_path(binary, tmpdir) + relative = binary.sub("#{tmpdir}/", "") + relative = relative.sub(%r{\Aext/[^/]+/}, "") if relative.start_with?("ext/") + relative = relative.sub(%r{\Alib/}, "") if relative.start_with?("lib/") + return relative unless relative.start_with?("extension/", "extensions/") + + relative.sub(%r{\Aextensions?/[^/]+/[^/]+/}, "") + end end end diff --git a/lib/prebake/platform_gem_builder.rb b/lib/prebake/platform_gem_builder.rb index 92a5ad0..15c453b 100644 --- a/lib/prebake/platform_gem_builder.rb +++ b/lib/prebake/platform_gem_builder.rb @@ -54,34 +54,7 @@ def build_platform_spec(build_dir) Dir.glob(File.join(build_dir, "**/*.{so,bundle,dll}")).each { |f| File.delete(f) } ext_dir = @spec.extension_dir - if ext_dir && File.directory?(ext_dir) - # Collect binaries at root and one level deep (e.g., nokogiri/nokogiri.so). - binaries = Dir.glob(File.join(ext_dir, "*.{so,bundle,dll}")) + - Dir.glob(File.join(ext_dir, "*/*.{so,bundle,dll}")) - - # Ruby 4.0+ places compiled extensions in extension/// - # within extension_dir. Collect these too, normalizing their paths - # to root level so the cached gem is layout-agnostic. - Dir.glob(File.join(ext_dir, "extension/*/*/*.{so,bundle,dll}")).each do |binary| - relative = binary.delete_prefix("#{ext_dir}/") - normalized = relative.sub(%r{\Aextension/[^/]+/[^/]+/}, "") - # Skip if a root-level binary with the same name already exists - next if binaries.any? { |b| b.delete_prefix("#{ext_dir}/") == normalized } - - binaries << binary - end - - binaries.each do |binary| - next if File.symlink?(binary) - next if File.size(binary).zero? - relative = binary.delete_prefix("#{ext_dir}/") - # Normalize extension/// paths to root level - relative = relative.sub(%r{\Aextension/[^/]+/[^/]+/}, "") - dest = File.join(build_dir, relative) - FileUtils.mkdir_p(File.dirname(dest)) - FileUtils.cp(binary, dest) - end - end + copy_installed_binaries(ext_dir, build_dir) if ext_dir && File.directory?(ext_dir) prefix = "#{build_dir}/" compiled = Dir.glob(File.join(build_dir, "**/*.{so,bundle,dll}")) @@ -90,5 +63,38 @@ def build_platform_spec(build_dir) platform_spec end + + def copy_installed_binaries(ext_dir, build_dir) + installed_binaries(ext_dir).each do |binary| + next if File.symlink?(binary) + next if File.empty?(binary) + + dest = File.join(build_dir, root_level_path(binary, ext_dir)) + FileUtils.mkdir_p(File.dirname(dest)) + FileUtils.cp(binary, dest) + end + end + + # Binaries at root and one level deep (e.g., nokogiri/nokogiri.so), plus the + # extension/// layout Ruby 4.0+ installs into. A nested + # binary is skipped when a root-level one already claims its normalized + # path, so the cached gem stays layout-agnostic. + def installed_binaries(ext_dir) + binaries = Dir.glob(File.join(ext_dir, "*.{so,bundle,dll}")) + + Dir.glob(File.join(ext_dir, "*/*.{so,bundle,dll}")) + + Dir.glob(File.join(ext_dir, "extension/*/*/*.{so,bundle,dll}")).each do |binary| + normalized = root_level_path(binary, ext_dir) + next if binaries.any? { |b| b.delete_prefix("#{ext_dir}/") == normalized } + + binaries << binary + end + + binaries + end + + def root_level_path(binary, ext_dir) + binary.delete_prefix("#{ext_dir}/").sub(%r{\Aextension/[^/]+/[^/]+/}, "") + end end end diff --git a/test/extension_validator_test.rb b/test/extension_validator_test.rb index f2e21ce..fa82b95 100644 --- a/test/extension_validator_test.rb +++ b/test/extension_validator_test.rb @@ -27,7 +27,7 @@ def test_skips_without_marker Prebake::ExtensionValidator.validate(spec) refute File.exist?(File.join(@extension_dir, "foo.so")), - "Should not copy binaries when .prebake marker is absent" + "Should not copy binaries when .prebake marker is absent" end def test_skips_correct_layout @@ -45,7 +45,7 @@ def test_skips_correct_layout Prebake::ExtensionValidator.validate(spec) refute File.exist?(File.join(@extension_dir, "bar.so")), - "Should not copy nested binaries when root-level binaries already exist" + "Should not copy nested binaries when root-level binaries already exist" end def test_fixes_extension_pattern @@ -96,7 +96,7 @@ def test_preserves_nested_lib_subdirectory Prebake::ExtensionValidator.validate(spec) refute File.exist?(File.join(@extension_dir, "foo.so")), - "Should not copy binaries from lib// (only direct children of lib/)" + "Should not copy binaries from lib// (only direct children of lib/)" end def test_skips_symlinks @@ -115,7 +115,7 @@ def test_skips_symlinks Prebake::ExtensionValidator.validate(spec) refute File.exist?(File.join(@extension_dir, "linked.so")), - "Should not copy symlinked binaries" + "Should not copy symlinked binaries" end def test_skips_empty_files @@ -131,7 +131,7 @@ def test_skips_empty_files Prebake::ExtensionValidator.validate(spec) refute File.exist?(File.join(@extension_dir, "empty.so")), - "Should not copy zero-byte binaries" + "Should not copy zero-byte binaries" end def test_idempotent @@ -155,5 +155,4 @@ def test_idempotent root_binaries = Dir.glob(File.join(@extension_dir, "*.so")) assert_equal 1, root_binaries.length, "Should not duplicate files on second run" end - end From a7bab60c085669d2a6aee1c399c3a2b57859bea3 Mon Sep 17 00:00:00 2001 From: Thejus Paul Date: Mon, 14 Sep 2026 19:39:29 +0530 Subject: [PATCH 2/2] ci: add a rubocop job --- .github/workflows/ci.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f9f86f2..2181fcf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,6 +25,21 @@ jobs: - run: bundle exec rake test + lint: + name: "Rubocop" + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + + - uses: ruby/setup-ruby@v1 + with: + # Matches TargetRubyVersion in .rubocop.yml. + ruby-version: "3.2" + bundler-cache: true + + - run: bundle exec rubocop + worker: name: "Worker" runs-on: ubuntu-latest