code review 6587048: debug/pe: support PE files which contain no symbol tabl...

debug/pe: support PE files which contain no symbol table (if NumberOfSymbols is equal to 0 in the IMAGE_FILE_HEADER structure).

No longer assume that e_lfanew (in the IMAGE_DOS_HEADER strcuture) is always one byte. It is now regarded as a 4 byte uint32.

Fixes issue 4177.

[u, 2012-09-30 20:15:18]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[brainman, 2012-10-01 02:21:30]

Test would be nice. In http://code.google.com/p/go/issues/detail?id=4177 you
say, that "... Notepad.exe contains no symbol table ...". Please, make test out
of that. Just use exec/os/LookPath to find it on the system.

Thank you.

Alex

http://codereview.appspot.com/6587048/diff/5001/src/pkg/debug/pe/file.go
File src/pkg/debug/pe/file.go (left):

http://codereview.appspot.com/6587048/diff/5001/src/pkg/debug/pe/file.go#oldc...
src/pkg/debug/pe/file.go:135: r.ReadAt(sign[0:], int64(dosheader[0x3c]))
s/0:/:/

http://codereview.appspot.com/6587048/diff/5001/src/pkg/debug/pe/file.go
File src/pkg/debug/pe/file.go (right):

http://codereview.appspot.com/6587048/diff/5001/src/pkg/debug/pe/file.go#newc...
src/pkg/debug/pe/file.go:134: peStart :=
int64(binary.LittleEndian.Uint32(dosheader[0x3c:]))
I do not like your variable name. peStart implies what? According to ms doco, it
is "offset to the PE signature". Maybe call it off or signoff? I would also be
happy, if you make it simple "i" or something like that.

[u, 2012-10-01 08:04:25]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2012-10-02 02:41:09]

On 2012/10/01 08:04:25, u wrote:
> ...
> Please take another look.

You didn't create test. Something like:

diff -r b09f4985724f src/pkg/debug/pe/file_test.go
--- a/src/pkg/debug/pe/file_test.go	Tue Oct 02 08:35:20 2012 +1000
+++ b/src/pkg/debug/pe/file_test.go	Tue Oct 02 12:38:06 2012 +1000
@@ -5,7 +5,9 @@
 package pe
 
 import (
+	"os/exec"
 	"reflect"
+	"runtime"
 	"testing"
 )
 
@@ -125,3 +127,19 @@
 		t.Errorf("open %s: succeeded unexpectedly", filename)
 	}
 }
+
+func TestFileWithNoSymbolTable(t *testing.T) {
+	if runtime.GOOS != "windows" {
+		t.Logf("skipping test on %q", runtime.GOOS)
+		return
+	}
+	p, err := exec.LookPath("notepad")
+	if err != nil {
+		t.Fatalf("exec.LookPath failed: %v", err)
+	}
+	f, err := Open(p)
+	if err != nil {
+		t.Fatalf("Open failed: %v", err)
+	}
+	defer f.Close()
+}

should do. Please add this to your CL. Feel free to change it as you see fit.

Thank you.

Alex

[u, 2012-10-02 06:27:32]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[u, 2012-10-02 06:29:31]

Thank you Alex for helping me with this CL.

Added the test file cl-386-exec which contains no symbol table. Below are the
test results.
I did not use the notepad example since we can't be sure that it lacks a symbol
table on all systems.

Result before:

=== RUN TestOpen
--- FAIL: TestOpen (0.01 seconds)
file_test.go:97: 	EOF
=== RUN TestOpenFailure
--- PASS: TestOpenFailure (0.00 seconds)
=== RUN TestOpenFileWithNoSymbolTable
--- FAIL: TestOpenFileWithNoSymbolTable (0.00 seconds)
file_test.go:144: 	open testdata/cl-386-exec failed: EOF
FAIL
exit status 1
FAIL	debug/pe	0.017s

Result after:

=== RUN TestOpen
--- PASS: TestOpen (0.00 seconds)
=== RUN TestOpenFailure
--- PASS: TestOpenFailure (0.00 seconds)
=== RUN TestOpenFileWithNoSymbolTable
--- PASS: TestOpenFileWithNoSymbolTable (0.00 seconds)
PASS
ok  	debug/pe	0.013s

[brainman, 2012-10-02 23:55:56]

LGTM

But I am concerned about cl-386-exec file - its size and the fact that it is
binary and is of unknown origin. I will leave this CL for a few days to give
others a chance to comment. I propose, we go and use my test instead, if we get
more reservations like mine.

Meanwhile, you must complete one of the contributor license agreements
http://golang.org/doc/contribute.html#copyright before we can submit your
change.

Thank you.

Alex

[dave_cheney.net, 2012-10-02 23:57:48]

-1 to additional binaries in the source repo, I can already see the
bug reports from persnickety anti virus software.

On Wed, Oct 3, 2012 at 9:55 AM,  <alex.brainman@gmail.com> wrote:
> LGTM
>
> But I am concerned about cl-386-exec file - its size and the fact that
> it is binary and is of unknown origin. I will leave this CL for a few
> days to give others a chance to comment. I propose, we go and use my
> test instead, if we get more reservations like mine.
>
> Meanwhile, you must complete one of the contributor license agreements
> http://golang.org/doc/contribute.html#copyright before we can submit
> your change.
>
>
> Thank you.
>
> Alex
>
> http://codereview.appspot.com/6587048/

[brainman, 2012-10-03 00:08:36]

On 2012/10/02 23:57:48, dfc wrote:
> -1 to additional binaries in the source repo, I can already see the
> bug reports from persnickety anti virus software.
> 

Fair enough. I will wait for someone from Go team to confirm then.

@u,

Alternatively, if you delete cl-386-exec and use notepad.exe, I will submit.

Alex

[u, 2012-10-03 08:21:44]

On 2012/10/03 00:08:36, brainman wrote:
> On 2012/10/02 23:57:48, dfc wrote:
> > -1 to additional binaries in the source repo, I can already see the
> > bug reports from persnickety anti virus software.
> > 
> 
> Fair enough. I will wait for someone from Go team to confirm then.
> 
> @u,
> 
> Alternatively, if you delete cl-386-exec and use notepad.exe, I will submit.
> 
> Alex

I can see the reason for not including unknown binaries, and will remove it from
the CL, but I don't think that using Notepad.exe is a solution. We can't be sure
that it lacks a symbol table on all systems, just because it did so on mine. An
unreliable test case is worse than no test case at all.

Btw, I have already signed the contributor license agreement, so no worries.

For now I will remove the test case all togeather until we can find a good
solution.

Thanks for the help!

Cheers /u

[u, 2012-10-03 08:41:08]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com, dave@cheney.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[minux1, 2012-10-03 13:32:03]

can we build the symbol table-less binary with gcc or just copy and strip
(or objcopy) an existing exe test file at test time (if gcc/strip/objcopy
etc. is in PATH)?
this file won't be difficult to build at test time,
and i think only testing this on windows suffices
as we don't use platform dependent features in debug/pe.

[brainman, 2012-10-04 00:18:19]

On 2012/10/03 08:21:44, u wrote:
> 
> ... I don't think that using Notepad.exe is a solution. We can't be sure
> that it lacks a symbol table on all systems, just because it did so on mine.
...

I disagree. I think notepad.exe test is fine. I tried it on many different
systems:

Windows XP SP3
Windows 2000 SP4
Windows Server 2003
Windows 7 Pro (64bit)
Windows Server 2008 Datacenter
Windows Server 2008 R2 (64bit)

and it fails everywhere without your patch. In fact, I haven't seen a system
where it succeeds.

> ... An
> unreliable test case is worse than no test case at all.

I disagree again. Any test is better then no test. "Sometimes only" failure is
better then "never fails". I checked our go builders, and it fails there. So we
will notice the problem, if it ever resurface. Alternatively, some users will
complain.

> 
> ..., I have already signed the contributor license agreement, ...

Thank you. I will look for it.

> For now I will remove the test case all together until we can find a good
> solution.

I suggest, we use notepad.exe test. Until we find something better.

On 2012/10/03 13:32:03, minux wrote:
> can we build the symbol table-less binary with gcc or just copy and strip
> (or objcopy) an existing exe test file at test time (if gcc/strip/objcopy
> etc. is in PATH)?
> this file won't be difficult to build at test time,
> and i think only testing this on windows suffices
> as we don't use platform dependent features in debug/pe.

I do not know how to do it. Feel free to post code. I do not think your
suggestion is better then using notepad.exe. It relies on tools we have no
control over. I do not know how they work, and I am not convince they will
always work (different version of compilers / libs). I also would like to keep
complexity to the minimum. I think we have invested too much time into this bit
of code already.

Alex

[brainman, 2012-10-09 00:15:59]

*** Submitted as http://code.google.com/p/go/source/detail?r=5a8c4552dd85 ***

debug/pe: support PE files which contain no symbol table (if NumberOfSymbols is
equal to 0 in the IMAGE_FILE_HEADER structure).

No longer assume that e_lfanew (in the IMAGE_DOS_HEADER strcuture) is always one
byte. It is now regarded as a 4 byte uint32.

Fixes issue 4177.

R=golang-dev, alex.brainman, dave, minux.ma
CC=golang-dev
http://codereview.appspot.com/6587048

Committer: Alex Brainman <alex.brainman@gmail.com>