Merge pull request #22 from Homebrew/open-optimization

Optimization: Replace control-flow-by-exception with magic testing and incremental reads.

lib/macho/fat_file.rb

@@ -4,6 +4,9 @@ module MachO
   # @see https://en.wikipedia.org/wiki/Mach-O#Multi-architecture_binaries
   # @see MachO::MachOFile
   class FatFile
+    # @return [String] the filename loaded from, or nil if loaded from a binary string
+    attr_accessor :filename
+
     # @return [MachO::FatHeader] the file's header
     attr_reader :header
 

lib/macho/macho_file.rb

@@ -5,6 +5,9 @@ module MachO
   # @see https://en.wikipedia.org/wiki/Mach-O
   # @see MachO::FatFile
   class MachOFile
+    # @return [String] the filename loaded from, or nil if loaded from a binary string
+    attr_accessor :filename
+
     # @return [MachO::MachHeader] if the Mach-O is 32-bit
     # @return [MachO::MachHeader64] if the Mach-O is 64-bit
     attr_reader :header

lib/macho/open.rb

@@ -3,14 +3,24 @@ module MachO
   # @param filename [String] the file being opened
   # @return [MachO::MachOFile] if the file is a Mach-O
   # @return [MachO::FatFile] if the file is a Fat file
+  # @raise [ArgumentError] if the given file does not exist
+  # @raise [MachO::TruncatedFileError] if the file is too small to have a valid header
+  # @raise [MachO::MagicError] if the file's magic is not valid Mach-O magic
   def self.open(filename)
+    raise ArgumentError.new("#{filename}: no such file") unless File.file?(filename)
+    raise TruncatedFileError.new unless File.stat(filename).size >= 4
+
+    magic = File.open(filename, "rb") { |f| f.read(4) }.unpack("N").first
+
     # open file and test magic instead of using exceptions for control?
-    begin
-      file = MachOFile.new(filename)
-    rescue FatBinaryError
+    if MachO.fat_magic?(magic)
       file = FatFile.new(filename)
+    elsif MachO.magic?(magic)
+      file = MachOFile.new(filename)
+    else
+      raise MagicError.new(magic)
     end
 
     file
   end
-end
+end

test/test_open.rb

@@ -5,6 +5,32 @@
 class MachOOpenTest < Minitest::Test
   include Helpers
 
+  def test_nonexistent_file
+    assert_raises ArgumentError do
+      MachO.open("/this/is/a/file/that/cannot/possibly/exist")
+    end
+  end
+
+  # MachO.open has slightly looser qualifications for truncation than
+  # either MachOFile.new or FatFile.new - it just makes sure that there are
+  # enough magic bytes to read, and lets the actual parser raise a
+  # TruncationError later on if required.
+  def test_truncated_file
+    tempfile_with_data("truncated_file", "\x00\x00") do |truncated_file|
+      assert_raises MachO::TruncatedFileError do
+        MachO.open(truncated_file.path)
+      end
+    end
+  end
+
+  def test_bad_magic
+    tempfile_with_data("junk_file", "\xFF\xFF\xFF\xFF") do |junk_file|
+      assert_raises MachO::MagicError do
+        MachO.open(junk_file.path)
+      end
+    end
+  end
+
   def test_open
     file = MachO.open("test/bin/libhello.dylib")