debug/pe: parse the import directory correctly #25193

arizvisa · 2018-05-01T05:47:04Z

This parses the import table properly which allows for debug/pe
to extract import symbols from pecoffs linked with an import
table in a section named something other than ".idata"

The section names in a pecoff object aren't guaranteed to actually
mean anything, so hardcoding a search for the ".idata" section
is not guaranteed to find the import table in all shared libraries.
This resulted in debug/pe being unable to read import symbols
from some libraries.

The proper way to locate the import table is to validate the
number of data directory entries, locate the import entry, and
then use the va to identify the section containing the import
table. This patch does exactly this.

Fixes #16103.

Updated file.go to determine the section containing the import table correctly instead of just assuming that it's in the ".idata" section. This is done by looking at the import data directory entry, grabbing the va and finding the section that matches it. This methodology is more generic (and proper) as section names aren't guaranteed to actually mean anything in pecoff. This will allow for other linkers to be used as only the gnu linkers hardcode the ".idata" section for storing the import directory. debug/pe/file.go: use the va in the import dd to determine section

Updated file.go to validate the number of data directory entries before assuming the import data directory entry exists. This is not too common, but it _is_ possible so let's be defensive jic. debug/pe/file.go: validate NumberOfRvaAndSizes includes imports

googlebot · 2018-05-01T05:47:08Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

arizvisa · 2018-05-01T05:49:18Z

signed contributor's license agreement.

googlebot · 2018-05-01T05:49:21Z

CLAs look good, thanks!

gopherbot · 2018-05-01T05:56:31Z

This PR (HEAD: 4360777) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/go/+/110555 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-05-01T05:57:08Z

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-01T10:20:41Z

Message from Alex Brainman:

Patch Set 1:

(6 comments)

Thank you for fixing this problem.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-02T02:10:44Z

Message from Ali Rizvi-Santiago:

Patch Set 1:

(4 comments)

Is the proper procedure for changing the commit logs to amend the commit and force push to the branch? Or should I just squash it all into a single commit?

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-02T07:12:20Z

Message from Ali Rizvi-Santiago:

Patch Set 1:

Patch Set 1:

(4 comments)

Is the proper procedure for changing the commit logs to amend the commit and force push to the branch? Or should I just squash it all into a single commit?

I didn't see this message in the actual review page for the commit, but wrt ->

Line 28:
This needs a test. Can you think of a way to test your change? Do you know of a file that ?>exist on every Windows that we could parse with this? Can we build a file that we could parse >with this with gcc?

I did a scan of all PE files on my current system (2k12 server), and it seems that the atmfd.dll font driver has an import table and doesn't have an .idata section. If someone else wants to verify this on some other systems, I can write a testcase really quick.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-02T10:14:34Z

Message from Alex Brainman:

Patch Set 1:

(2 comments)

I did a scan of all PE files on my current system (2k12 server),
and it seems that the atmfd.dll font driver has an import table and
doesn't have an .idata section. If someone else wants to verify
this on some other systems, I can write a testcase really quick.

Yes I have multiple copies of atmfd.dll on my Windows 10. In c:\windows\system32 and c:\windows\syswow64 and many others. That sounds like a plan.

Thank you.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

This adds the definitions of all of the available IMAGE_DIRECTORY_ENTRY types. This is only used in one place though. Updated some of the comments to correspond to the current state of the PE import table parsing code. src/debug/pe/pe.go: added definitions src/debug/pe/file.go: used definitions

This adds a test against atmfd as suggested by Alex Brainman that proves that file.go has the ability to parse an import table residing in a section not named .idata. This test is windows-only and since the atmfd driver itself is actually provided by Adobe but bundled with all versions of windows, we're forced to search for the file in the current PATH and then try to read the symbols from it. If the file isn't found, the test fails. src/debug/pe/file_test.go: added test

gopherbot · 2018-05-04T05:19:01Z

This PR (HEAD: 8eb7304) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/go/+/110555 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-05-04T05:27:35Z

Message from Ali Rizvi-Santiago:

Patch Set 3: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-04T05:28:00Z

Message from Gerrit Bot:

Uploaded patch set 4: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-04T05:30:32Z

Message from Ali Rizvi-Santiago:

Patch Set 5: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-04T05:30:54Z

Message from Gerrit Bot:

Uploaded patch set 6: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-04T05:37:34Z

Message from Ali Rizvi-Santiago:

Patch Set 6:

Patch Set 1:

(4 comments)

Is the proper procedure for changing the commit logs to amend the commit and force push to the branch? Or should I just squash it all into a single commit?

So again, what is the proper procedure for changing the commit logs?

As you see in the patchset history, the past two times that I've tried to update the commit message to include your suggestions the bot has restored it back to the original.

Regardless of what the commit message says though, patch set 3 includes the atmfd.dll test and your suggestion of including all the data directory entries.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-04T10:17:21Z

Message from Alex Brainman:

Patch Set 6:

So again, what is the proper procedure for changing the commit
logs?

Sorry, I am not very helpful. I use git-codereview tool myself from https://golang.org/doc/contribute.html

You could, probably, go to

https://go-review.googlesource.com/#/c/go/+/110555

and click on "Download" button and checkout your latest patch set into git repo. And then you can update commit message and just push back into the same remote branch. The only important thing is that you keep Change-Id line in your commit message, otherwise Gerrit will create new CL for you. If you do everything correctly, you will see your changes here as new patch set.

I have never used Github PR method to create CLs here. I have CC-ed Andrew - hopefully he can help you to update commit message with PR.

As you see in the patchset history, the past two times that I've
tried to update the commit message to include your suggestions the
bot has restored it back to the original.

Regardless of what the commit message says though, patch set 3
includes the atmfd.dll test and your suggestion of including all
the data directory entries.

I will wait for your updated commit message before reviewing your changes since patch set 1. Let me know,if I should review this as is.

Thank you.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-04T14:44:12Z

Message from Brad Fitzpatrick:

Patch Set 6:

See the bottom section of this page:

https://github.com/golang/go/wiki/CommitMessage#github-pull-requests

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-05T18:50:31Z

Message from Gerrit Bot:

Uploaded patch set 7: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-05T18:51:46Z

Message from Ali Rizvi-Santiago:

Patch Set 7:

Patch Set 6:

See the bottom section of this page:

https://github.com/golang/go/wiki/CommitMessage#github-pull-requests

Ok. Got it changed. Thanks guys.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-05T18:52:32Z

Message from Gerrit Bot:

Uploaded patch set 8: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-06T00:13:17Z

Message from Alex Brainman:

Patch Set 8:

(8 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-06T22:01:29Z

Message from Ali Rizvi-Santiago:

Patch Set 8:

Patch Set 8:

(8 comments)

Ah those vars in line 540 were left because I had to write the test oob and then moved it into file_test.go. In that case, how do you get past the test error "use of internal package not allowed"?

[550] user@skinny ~/build/go1.10.2/src/debug/pe$ go test -v
file_test.go:9:2: use of internal package not allowed
FAIL /C/users/user/build/go1.10.2/src/debug/pe [setup failed]

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-06T22:13:09Z

Message from Ali Rizvi-Santiago:

Patch Set 8:

(7 comments)

Will

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

It was pointed out that os/exec.LookPath can be used to search the executable path instead of manually parsing it out of the environment. Updated the test to use this and a few other suggestions. The test was also updated to ensure that at least one symbol was being returned from .ImportedSymbols(). src/debug/pe/file.go: check the dd length against const src/debug/pe/file_test.go: updated test src/debug/pe/pe.go: removed RESERVED const

gopherbot · 2018-05-07T20:56:49Z

This PR (HEAD: fd671dd) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/go/+/110555 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-05-13T06:23:12Z

Message from Alex Brainman:

Patch Set 9: Run-TryBot+1

(2 comments)

I think you should add check for some common symbol in your test, and that will be it.

Thank you.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-13T06:23:21Z

Message from Gobot Gobot:

Patch Set 9:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=b42681be

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-13T06:32:09Z

Message from Gobot Gobot:

Patch Set 9: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

Requested by Alex Brainman to look for a common symbol in atmfd.dll to verify that ImportedSymbols actually works. EngMulDiv was chosen. This adds a loop to the test that searches for that particular symbol. src/debug/pe/file_test.go: updated test

gopherbot · 2018-05-15T18:02:49Z

This PR (HEAD: 83d8179) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/go/+/110555 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-05-15T18:05:27Z

Message from Ali Rizvi-Santiago:

Patch Set 10:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

TestImportTableInUnknownSection was emitting just the filename when parts of the test failed. This modifies the tests so that it emits the full path when a failure occurs. That should make it more accurate when troubleshooting if there's another instance of the driver within the user's path. src/debug/pe/file_test.go: modified filename to path

gopherbot · 2018-05-15T19:06:14Z

This PR (HEAD: 82e6a28) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/go/+/110555 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-05-17T09:54:34Z

Message from Alex Brainman:

Patch Set 11: Run-TryBot+1

(2 comments)

Nearly there.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-17T09:54:46Z

Message from Gobot Gobot:

Patch Set 11:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=d0184277

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-17T09:57:48Z

Message from Gobot Gobot:

Patch Set 11:

Build is still in progress...
This change failed on windows-386-2008:
See https://storage.googleapis.com/go-build-log/d0184277/windows-386-2008_8b9629d8.log

Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-17T10:02:01Z

Message from Gobot Gobot:

Patch Set 11: TryBot-Result-1

2 of 17 TryBots failed:
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/d0184277/windows-386-2008_8b9629d8.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/d0184277/windows-amd64-2016_e1bb1fdd.log

Consult https://build.golang.org/ to see whether they are new failures.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-17T10:07:20Z

Message from Alex Brainman:

Patch Set 11:

Patch Set 11: TryBot-Result-1

2 of 17 TryBots failed:
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/d0184277/windows-386-2008_8b9629d8.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/d0184277/windows-amd64-2016_e1bb1fdd.log

Consult https://build.golang.org/ to see whether they are new failures.

There goes my idea of searching for a predefined symbol. Please remove that part of your code.

Thank you.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

It was suggested to search for the "EngMulDiv" import for "atmfd.dll" in order to prove that ImportedSymbols actually works. Unfortunately not all instances of "atmfd.dll" actually import the "EngMulDiv" symbol as shown by TryBots. This essentially reverts 83d8179. src/debug/pe/file_test.go: removed symbolname from test

gopherbot · 2018-05-18T18:48:48Z

This PR (HEAD: ce8077c) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/go/+/110555 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2018-05-18T18:49:21Z

Message from Ali Rizvi-Santiago:

Patch Set 11:

(2 comments)

I removed the explicit test for the EngMulDiv symbol too as you suggested.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-19T05:48:41Z

Message from Alex Brainman:

Patch Set 12: Run-TryBot+1

LGTM once builders are happy.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-19T05:48:56Z

Message from Gobot Gobot:

Patch Set 12:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=a4554af4

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-19T06:05:37Z

Message from Gobot Gobot:

Patch Set 12: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2018-05-19T06:06:47Z

Message from Alex Brainman:

Patch Set 12: Code-Review+2

Please don’t reply on this GitHub thread. Visit golang.org/cl/110555.
After addressing review feedback, remember to publish your drafts!

This parses the import table properly which allows for debug/pe to extract import symbols from pecoffs linked with an import table in a section named something other than ".idata" The section names in a pecoff object aren't guaranteed to actually mean anything, so hardcoding a search for the ".idata" section is not guaranteed to find the import table in all shared libraries. This resulted in debug/pe being unable to read import symbols from some libraries. The proper way to locate the import table is to validate the number of data directory entries, locate the import entry, and then use the va to identify the section containing the import table. This patch does exactly this. Fixes #16103. Change-Id: I3ab6de7f896a0c56bb86c3863e504e8dd4c8faf3 GitHub-Last-Rev: ce8077c GitHub-Pull-Request: #25193 Reviewed-on: https://go-review.googlesource.com/110555 Run-TryBot: Alex Brainman <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Alex Brainman <[email protected]>

gopherbot · 2018-05-19T06:07:19Z

This PR is being closed because golang.org/cl/110555 has been merged.

This parses the import table properly which allows for debug/pe to extract import symbols from pecoffs linked with an import table in a section named something other than ".idata" The section names in a pecoff object aren't guaranteed to actually mean anything, so hardcoding a search for the ".idata" section is not guaranteed to find the import table in all shared libraries. This resulted in debug/pe being unable to read import symbols from some libraries. The proper way to locate the import table is to validate the number of data directory entries, locate the import entry, and then use the va to identify the section containing the import table. This patch does exactly this. Fixes #16103. Change-Id: I3ab6de7f896a0c56bb86c3863e504e8dd4c8faf3 GitHub-Last-Rev: ce8077cb154f18ada7a86e152ab03de813937816 GitHub-Pull-Request: golang/go#25193 Reviewed-on: https://go-review.googlesource.com/110555 Run-TryBot: Alex Brainman <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Alex Brainman <[email protected]>

arizvisa added 2 commits April 30, 2018 23:40

googlebot added the cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. label May 1, 2018

googlebot added cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. and removed cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. labels May 1, 2018

gopherbot force-pushed the master branch 3 times, most recently from 6a8cff5 to 5d9c782 Compare May 1, 2018 15:59

arizvisa added 2 commits May 4, 2018 00:08

gopherbot force-pushed the master branch from a8a60ac to 87412a1 Compare May 6, 2018 04:30

gopherbot closed this May 19, 2018

debug/pe: parse the import directory correctly #25193

debug/pe: parse the import directory correctly #25193

Uh oh!

Conversation

arizvisa commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented May 1, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

arizvisa commented May 1, 2018

Uh oh!

googlebot commented May 1, 2018

Uh oh!

gopherbot commented May 1, 2018

Uh oh!

gopherbot commented May 1, 2018

Uh oh!

gopherbot commented May 1, 2018

Uh oh!

gopherbot commented May 2, 2018

Uh oh!

gopherbot commented May 2, 2018

Uh oh!

gopherbot commented May 2, 2018

Uh oh!

gopherbot commented May 4, 2018

Uh oh!

gopherbot commented May 4, 2018

Uh oh!

gopherbot commented May 4, 2018

Uh oh!

gopherbot commented May 4, 2018

Uh oh!

gopherbot commented May 4, 2018

Uh oh!

gopherbot commented May 4, 2018

Uh oh!

gopherbot commented May 4, 2018

Uh oh!

gopherbot commented May 4, 2018

Uh oh!

gopherbot commented May 5, 2018

Uh oh!

gopherbot commented May 5, 2018

Uh oh!

gopherbot commented May 5, 2018

Uh oh!

gopherbot commented May 6, 2018

Uh oh!

gopherbot commented May 6, 2018

Uh oh!

gopherbot commented May 6, 2018

Uh oh!

gopherbot commented May 7, 2018

Uh oh!

gopherbot commented May 13, 2018

Uh oh!

gopherbot commented May 13, 2018

Uh oh!

gopherbot commented May 13, 2018

Uh oh!

gopherbot commented May 15, 2018

Uh oh!

gopherbot commented May 15, 2018

Uh oh!

gopherbot commented May 15, 2018

Uh oh!

gopherbot commented May 17, 2018

Uh oh!

gopherbot commented May 17, 2018

Uh oh!

gopherbot commented May 17, 2018

Uh oh!

gopherbot commented May 17, 2018

Uh oh!

gopherbot commented May 17, 2018

Uh oh!

arizvisa commented May 1, 2018 •

edited

Loading