Improve "unrecognized binary" error messages (#408)

Improve "unrecognized binary" error messages

Previously it would just print "unrecognized binary" no matter what the underlying error message was. This was partly because it was unable to know the actual file type except by trial and error. This commit uses the magic number instead. It also gives a nicer error for fat Mach-O binaries which are currently unsupported.

internal/binutils/binutils.go

@@ -18,7 +18,9 @@ package binutils
 import (
 	"debug/elf"
 	"debug/macho"
+	"encoding/binary"
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -173,12 +175,8 @@ func (bu *Binutils) Open(name string, start, limit, offset uint64) (plugin.ObjFi
 	b := bu.get()
 
 	// Make sure file is a supported executable.
-	// The pprof driver uses Open to sniff the difference
-	// between an executable and a profile.
-	// For now, only ELF is supported.
-	// Could read the first few bytes of the file and
-	// use a table of prefixes if we need to support other
-	// systems at some point.
+	// This uses magic numbers, mainly to provide better error messages but
+	// it should also help speed.
 
 	if _, err := os.Stat(name); err != nil {
 		// For testing, do not require file name to exist.
@@ -188,13 +186,48 @@ func (bu *Binutils) Open(name string, start, limit, offset uint64) (plugin.ObjFi
 		return nil, err
 	}
 
-	if f, err := b.openELF(name, start, limit, offset); err == nil {
+	// Read the first 4 bytes of the file.
+
+	f, err := os.Open(name)
+	if err != nil {
+		return nil, fmt.Errorf("error opening %s: %s", name, err)
+	}
+	defer f.Close()
+
+	var header [4]byte
+	if _, err = io.ReadFull(f, header[:]); err != nil {
+		return nil, fmt.Errorf("error reading magic number from %s: %s", name, err)
+	}
+
+	elfMagic := string(header[:])
+
+	// Match against supported file types.
+	if elfMagic == elf.ELFMAG {
+		f, err := b.openELF(name, start, limit, offset)
+		if err != nil {
+			return nil, fmt.Errorf("error reading ELF file %s: %s", name, err)
+		}
 		return f, nil
 	}
-	if f, err := b.openMachO(name, start, limit, offset); err == nil {
+
+	// Mach-O magic numbers can be big or little endian.
+	machoMagicLittle := binary.LittleEndian.Uint32(header[:])
+	machoMagicBig := binary.BigEndian.Uint32(header[:])
+
+	if machoMagicLittle == macho.Magic32 || machoMagicLittle == macho.Magic64 ||
+		machoMagicBig == macho.Magic32 || machoMagicBig == macho.Magic64 {
+		f, err := b.openMachO(name, start, limit, offset)
+		if err != nil {
+			return nil, fmt.Errorf("error reading Mach-O file %s: %s", name, err)
+		}
 		return f, nil
 	}
-	return nil, fmt.Errorf("unrecognized binary: %s", name)
+	if machoMagicLittle == macho.MagicFat || machoMagicBig == macho.MagicFat {
+		// TODO: #1033
+		return nil, fmt.Errorf("fat Mach-O archives are currently unsupported")
+	}
+
+	return nil, fmt.Errorf("unrecognized binary format: %s", name)
 }
 
 func (b *binrep) openMachO(name string, start, limit, offset uint64) (plugin.ObjFile, error) {

internal/binutils/binutils_test.go

@@ -22,6 +22,7 @@ import (
 	"reflect"
 	"regexp"
 	"runtime"
+	"strings"
 	"testing"
 
 	"github.com/google/pprof/internal/plugin"
@@ -361,3 +362,31 @@ func TestLLVMSymbolizer(t *testing.T) {
 		}
 	}
 }
+
+func TestOpenMalformedELF(t *testing.T) {
+	// Test that opening a malformed ELF file will report an error containing
+	// the word "ELF".
+	bu := &Binutils{}
+	_, err := bu.Open(filepath.Join("testdata", "malformed_elf"), 0, 0, 0)
+	if err == nil {
+		t.Fatalf("Open: unexpected success")
+	}
+
+	if !strings.Contains(err.Error(), "ELF") {
+		t.Errorf("Open: got %v, want error containing 'ELF'", err)
+	}
+}
+
+func TestOpenMalformedMachO(t *testing.T) {
+	// Test that opening a malformed Mach-O file will report an error containing
+	// the word "Mach-O".
+	bu := &Binutils{}
+	_, err := bu.Open(filepath.Join("testdata", "malformed_macho"), 0, 0, 0)
+	if err == nil {
+		t.Fatalf("Open: unexpected success")
+	}
+
+	if !strings.Contains(err.Error(), "Mach-O") {
+		t.Errorf("Open: got %v, want error containing 'Mach-O'", err)
+	}
+}

internal/binutils/testdata/malformed_elf

@@ -0,0 +1 @@
+ELF˙˙˙˙˙˙˙˙

internal/binutils/testdata/malformed_macho

@@ -0,0 +1 @@
+销睨��������