FAQ
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#oldcode135
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#newcode134
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.

http://codereview.appspot.com/6587048/

Search Discussions

  • R Eklind 87 at Oct 1, 2012 at 8:18 am
    Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6587048/
  • Alex Brainman at Oct 2, 2012 at 2:41 am

    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

    http://codereview.appspot.com/6587048/
  • R Eklind 87 at Oct 2, 2012 at 6:32 am
    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


    https://codereview.appspot.com/6587048/
  • R Eklind 87 at Oct 2, 2012 at 6:32 am
    Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6587048/
  • Alex Brainman at Oct 2, 2012 at 11:55 pm
    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/
  • Dave Cheney at Oct 2, 2012 at 11:57 pm
    -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, 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/
  • Alex Brainman at Oct 3, 2012 at 12:08 am

    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

    http://codereview.appspot.com/6587048/
  • R Eklind 87 at Oct 3, 2012 at 1:02 pm

    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

    https://codereview.appspot.com/6587048/
  • R Eklind 87 at Oct 3, 2012 at 1:02 pm
    Hello golang-dev@googlegroups.com, alex.brainman@gmail.com,
    dave@cheney.net (cc: golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6587048/
  • Minux at Oct 3, 2012 at 1:32 pm
    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.
  • Alex Brainman at Oct 4, 2012 at 12:18 am

    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

    http://codereview.appspot.com/6587048/
  • Alex Brainman at Oct 9, 2012 at 12:24 am
    *** 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>


    http://codereview.appspot.com/6587048/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 1, '12 at 2:21a
activeOct 9, '12 at 12:24a
posts13
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase