Validate segment nsects against cmdsize and file bounds

[alebcay]

• Check length against available file data and that section table fits cmdsize
• Raise LoadCommandSizeError on invalid nsects values

A quick PoC script (tested on Linux, haven't checked if rss_mb works on macOS) which loads a modified variant of the test fixture (manipulating nsects to be invalidly large) and can cause a large memory allocation:

#!/usr/bin/env ruby
# frozen_string_literal: true
# PoC: reach bad nsects via SegmentCommand#to_h (public caller).
require "macho"

def rss_mb
  File.read("/proc/self/status")[/VmRSS:\s+(\d+) kB/, 1].to_i / 1024
rescue
  0
end

path = "./test/bin/x86_64/hello.bin"
raw = File.binread(path)
f = MachO::MachOFile.new(path)
seg = f.segments.find { |s| s.segname == "__TEXT" }
puts "valid file contains: nsects=#{seg.nsects} sections=#{seg.to_h["sections"].size}"

[1000, 1_000_000].each do |n|
  bad = raw.dup
  bad[seg.view.offset + 64, 4] = [n].pack("L<")
  print "\nnsects=#{n}: "
  t0 = Time.now
  before = rss_mb
  begin
    MachO::MachOFile.new_from_bin(bad).segments
      .find { |s| s.segname == "__TEXT" }.to_h
  rescue MachO::MachOError => e
    puts "#{e.class} caught"
  end
  puts "rss #{before} MiB -> #{rss_mb} MiB"
end

[Copilot]

🟡 Changes recommended

The security-sensitive bounds checks need regression test coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds bounds validation to prevent malicious nsects values from causing excessive allocations.

Changes:

• Validates section-table length against file data and cmdsize.
• Raises LoadCommandSizeError for invalid section counts.

File summaries

File	Review
lib/macho/load_commands.rb	Adds section-table bounds checks. Regression tests for corrupted 32-bit and 64-bit nsects values are required.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[p-linnane]

The bounds check itself looks correct, but this security-sensitive parser change needs regression coverage before merge.

Please add tests for both LC_SEGMENT and LC_SEGMENT_64. Set nsects to one more than the number of sections permitted by cmdsize, then assert that sections raises MachO::LoadCommandSizeError. Please use the smallest invalid value rather than an enormous count, so a future regression fails safely without risking an OOM in CI.

The existing Copilot comment captures this requirement and can be resolved once those tests are added.

Separately, the same count-controlled allocation pattern exists for LC_BUILD_VERSION#ntools and LC_TWOLEVEL_HINTS#nhints. Those are pre-existing issues and do not need to expand this PR’s scope, but they should be tracked separately. Until addressed, this PR should be described specifically as fixing nsects, not malicious count handling generally.