[go] debug/pe: avoid panic in File.ImportedSymbols
Alex Brainman (Gerrit)
Alex Brainman has uploaded the change for review![]( "Open in Gerrit")
Commit message
debug/pe: avoid panic in File.ImportedSymbols
This change skips symbols where dt.OriginalFirstThunk is less than
ds.VirtualAddress.
The variable types are uint32, and
(dt.OriginalFirstThunk-ds.VirtualAddress) becomes very large number when
dt.OriginalFirstThunk < ds.VirtualAddress.
Fixes #73548.
Change diff
diff --git a/src/debug/pe/file.go b/src/debug/pe/file.go
index ed63a11..8d78493 100644
--- a/src/debug/pe/file.go
+++ b/src/debug/pe/file.go
@@ -408,6 +408,9 @@
dt.dll, _ = getString(names, int(dt.Name-ds.VirtualAddress))
d, _ = ds.Data()
// seek to OriginalFirstThunk
+ if dt.OriginalFirstThunk < ds.VirtualAddress {
+ continue
+ }
d = d[dt.OriginalFirstThunk-ds.VirtualAddress:]
for len(d) > 0 {
if pe64 { // 64bit
diff --git a/src/debug/pe/file_test.go b/src/debug/pe/file_test.go
index 3d960ab..b1da0e9 100644
--- a/src/debug/pe/file_test.go
+++ b/src/debug/pe/file_test.go
@@ -688,50 +688,76 @@
}
func TestImportedSymbolsNoPanicWithSliceOutOfBound(t *testing.T) {
- // https://golang.org/issue/30253
- // ImportedSymbols shouldn't panic with slice out of bounds
- // Input generated by gofuzz
- data := []byte("L\x01\b\x00regi\x00\x00\x00\x00\x00\x00\x00\x00\xe0\x00\x0f\x03" +
- "\v\x01\x02\x18\x00\x0e\x00\x00\x00\x1e\x00\x00\x00\x02\x00\x00\x80\x12\x00\x00" +
- "\x00\x10\x00\x00\x00 \x00\x00\x00\x00@\x00\x00\x10\x00\x00\x00\x02\x00\x00" +
- "\x04\x00\x00\x00\x01\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x90\x00\x00" +
- "\x00\x04\x00\x00\x06S\x00\x00\x03\x00\x00\x00\x00\x00 \x00\x00\x10\x00\x00" +
- "\x00\x00\x10\x00\x00\x10\x00\x00\x00\x00\x00\x00\x10\x00\x00\x00\x00\x00\x00\x00" +
- "\x00\x00\x00\x00\x00`\x00\x00x\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
- "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
- "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
- "\x00\x00\x00\x00\x00\x00\x00\x00\x04\x80\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00" +
- "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xb8`\x00\x00|\x00\x00\x00" +
- "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
- "\x00\x00\x00\x00.text\x00\x00\x00d\f\x00\x00\x00\x10\x00\x00" +
- "\x00\x0e\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
- "`\x00P`.data\x00\x00\x00\x10\x00\x00\x00\x00 \x00\x00" +
- "\x00\x02\x00\x00\x00\x12\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
- "@\x000\xc0.rdata\x00\x004\x01\x00\x00\x000\x00\x00" +
- "\x00\x02\x00\x00\x00\x14\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
- "@\x000@.eh_fram\xa0\x03\x00\x00\x00@\x00\x00" +
- "\x00\x04\x00\x00\x00\x16\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
- "@\x000@.bss\x00\x00\x00\x00`\x00\x00\x00\x00P\x00\x00" +
- "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
- "\x80\x000\xc0.idata\x00\x00x\x03\x00\x00\x00`\x00\x00" +
- "\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00@\x00" +
- "0\xc0.CRT\x00\x00\x00\x00\x18\x00\x00\x00\x00p\x00\x00\x00\x02" +
- "\x00\x00\x00\x1e\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00@\x00" +
- "0\xc0.tls\x00\x00\x00\x00 \x00\x00\x00\x00\x80\x00\x00\x00\x02" +
- "\x00\x00\x00 \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x001\xc9" +
- "H\x895\x1d")
-
- f, err := NewFile(bytes.NewReader(data))
- if err != nil {
- t.Fatal(err)
+ tests := []struct {
+ name string
+ data []byte
+ }{
+ {
+ // https://golang.org/issue/30253
+ // ImportedSymbols shouldn't panic with slice out of bounds
+ // Input generated by gofuzz
+ name: "issue/30253",
+ data: []byte("L\x01\b\x00regi\x00\x00\x00\x00\x00\x00\x00\x00\xe0\x00\x0f\x03" +
+ "\v\x01\x02\x18\x00\x0e\x00\x00\x00\x1e\x00\x00\x00\x02\x00\x00\x80\x12\x00\x00" +
+ "\x00\x10\x00\x00\x00 \x00\x00\x00\x00@\x00\x00\x10\x00\x00\x00\x02\x00\x00" +
+ "\x04\x00\x00\x00\x01\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x90\x00\x00" +
+ "\x00\x04\x00\x00\x06S\x00\x00\x03\x00\x00\x00\x00\x00 \x00\x00\x10\x00\x00" +
+ "\x00\x00\x10\x00\x00\x10\x00\x00\x00\x00\x00\x00\x10\x00\x00\x00\x00\x00\x00\x00" +
+ "\x00\x00\x00\x00\x00`\x00\x00x\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+ "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+ "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+ "\x00\x00\x00\x00\x00\x00\x00\x00\x04\x80\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00" +
+ "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xb8`\x00\x00|\x00\x00\x00" +
+ "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+ "\x00\x00\x00\x00.text\x00\x00\x00d\f\x00\x00\x00\x10\x00\x00" +
+ "\x00\x0e\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+ "`\x00P`.data\x00\x00\x00\x10\x00\x00\x00\x00 \x00\x00" +
+ "\x00\x02\x00\x00\x00\x12\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+ "@\x000\xc0.rdata\x00\x004\x01\x00\x00\x000\x00\x00" +
+ "\x00\x02\x00\x00\x00\x14\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+ "@\x000@.eh_fram\xa0\x03\x00\x00\x00@\x00\x00" +
+ "\x00\x04\x00\x00\x00\x16\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+ "@\x000@.bss\x00\x00\x00\x00`\x00\x00\x00\x00P\x00\x00" +
+ "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +
+ "\x80\x000\xc0.idata\x00\x00x\x03\x00\x00\x00`\x00\x00" +
+ "\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00@\x00" +
+ "0\xc0.CRT\x00\x00\x00\x00\x18\x00\x00\x00\x00p\x00\x00\x00\x02" +
+ "\x00\x00\x00\x1e\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00@\x00" +
+ "0\xc0.tls\x00\x00\x00\x00 \x00\x00\x00\x00\x80\x00\x00\x00\x02" +
+ "\x00\x00\x00 \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x001\xc9" +
+ "H\x895\x1d"),
+ },
+ {
+ // https://golang.org/issue/73548
+ // ImportedSymbols shouldn't panic with slice out of bounds
+ // Input is provided on the issue https://github.com/golang/go/issues/73548#issue-3030856705
+ // in https://go.dev/play/p/K1_mQcc4Fbd
+ name: "issue/73548",
+ //data: nil,
+ data: func() []byte {
+ data, err := os.ReadFile("testdata/first_32k_of_winword_exe")
+ if err != nil {
+ t.Fatal(err)
+ }
+ return data
+ }(),
+ },
}
+ for _, test := range tests {
+ t.Run(test.name, func(t *testing.T) {
+ f, err := NewFile(bytes.NewReader(test.data))
+ if err != nil {
+ t.Fatal(err)
+ }
- syms, err := f.ImportedSymbols()
- if err != nil {
- t.Fatal(err)
- }
+ syms, err := f.ImportedSymbols()
+ if err != nil {
+ t.Fatal(err)
+ }
- if len(syms) != 0 {
- t.Fatalf("expected len(syms) == 0, received len(syms) = %d", len(syms))
+ if len(syms) != 0 {
+ t.Fatalf("expected len(syms) == 0, received len(syms) = %d", len(syms))
+ }
+ })
}
}
diff --git a/src/debug/pe/testdata/first_32k_of_winword_exe b/src/debug/pe/testdata/first_32k_of_winword_exe
new file mode 100644
index 0000000..4ae0255
--- /dev/null
+++ b/src/debug/pe/testdata/first_32k_of_winword_exe
Binary files differ
Change information
- M src/debug/pe/file.go
- M src/debug/pe/file_test.go
- A src/debug/pe/testdata/first_32k_of_winword_exe
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman added 1 comment![]( "Open in Gerrit")
Not sure if it is OK to merge largish (32kb) binary file src/debug/pe/testdata/first_32k_of_winword_exe .
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emmanuel Odeke (Gerrit)
Emmanuel Odeke added 1 comment![]( "Open in Gerrit")
Thank you Alex for this change and long time! Kindly please rebase from master and I am going to kindly tag Ian to help review.
Related details
- Alex Brainman
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emmanuel Odeke (Gerrit)
Emmanuel Odeke added 2 comments![]( "Open in Gerrit")
Thank you for this fix Alex! I am including a suggestion for hardening against further crashes by tightening that check with `seek >= uint32(len(d))` given https://go.dev/play/p/jo0icuZyv1M. Kindly please update it.
if dt.OriginalFirstThunk < ds.VirtualAddress {
continue
}I have a vector https://go.dev/play/p/jo0icuZyv1M that still crashes the line below and the correct check should be ensuring that we don't index with an overflown value so
```
if seek := dt.OriginalFirstThunk - ds.VirtualAddress; seek >= uint32(len(d)) {
continue
} else {
d = d[seek:]
}
```
Related details
- Alex Brainman
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Alex Brainman
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 3 comments![]( "Open in Gerrit")
continueWhy continue rather than returning an error?
name: "issue/73548/extra",Our current policy for debug/elf,debug/macho,debug/pe is to not bother adding a test case for invalid object files. There is a vast number of invalid object files and we don't need copies of all them. Instead, we rely on the fuzzer to find problems.
Related details
- Alex Brainman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman voted and added 7 comments![]( "Open in Gerrit")
Votes added by Alex Brainman
| Commit-Queue | +1 |
7 comments
Alex BrainmanThank you Alex for this change and long time! Kindly please rebase from master and I am going to kindly tag Ian to help review.
Rebased.
Thank you for this fix Alex! I am including a suggestion for hardening against further crashes by tightening that check with `seek >= uint32(len(d))` given https://go.dev/play/p/jo0icuZyv1M. Kindly please update it.
Updated my CL.
Thank you Emmanuel and Ian for review.
PTAL (hopefully TryBots will pass).
Alex
if dt.OriginalFirstThunk < ds.VirtualAddress {
continue
}I have a vector https://go.dev/play/p/jo0icuZyv1M that still crashes the line below and the correct check should be ensuring that we don't index with an overflown value so
```
if seek := dt.OriginalFirstThunk - ds.VirtualAddress; seek >= uint32(len(d)) {
continue
} else {
d = d[seek:]
}
```
I used https://go.dev/play/p/jo0icuZyv1M in my adjusted CL.
continueWhy continue rather than returning an error?
I do not understand how this code works. I never did.
So I am trying to change the code as little as I can.
But I will return error, if you insist.
} else {You don't need an "else" here.
I do need that "else", otherwise I cannot use "seek" variable in
```
d = d[seek:]
```
on the next line.
Let me know if you want me to rewrite that code.
name: "issue/73548/extra",Our current policy for debug/elf,debug/macho,debug/pe is to not bother adding a test case for invalid object files. There is a vast number of invalid object files and we don't need copies of all them. Instead, we rely on the fuzzer to find problems.
What do you propose I do to stop people making this code panic again in the future?
Related details
- Emmanuel Odeke
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 3 comments![]( "Open in Gerrit")
continueAlex BrainmanWhy continue rather than returning an error?
I do not understand how this code works. I never did.
So I am trying to change the code as little as I can.
But I will return error, if you insist.
I'm not insisting. You should do what you think is best.
It's just that it seems to me that if the seek value is too large, then something is wrong. The file is corrupt.
} else {Alex BrainmanYou don't need an "else" here.
I do need that "else", otherwise I cannot use "seek" variable in
```
d = d[seek:]
```
on the next line.
Let me know if you want me to rewrite that code.
Personally I would write it as
seek := ...
if seek >= uint32(len(d)) {
return error
}
d = d[seek:]
But you should write it as you see best.
name: "issue/73548/extra",Alex BrainmanOur current policy for debug/elf,debug/macho,debug/pe is to not bother adding a test case for invalid object files. There is a vast number of invalid object files and we don't need copies of all them. Instead, we rely on the fuzzer to find problems.
What do you propose I do to stop people making this code panic again in the future?
People run fuzzers on these packages. That will catch any errors introduced in the future. It doesn't scale for us to add every possible invalid file. There are too many.
Related details
- Alex Brainman
- Emmanuel Odeke
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emmanuel Odeke (Gerrit)
Emmanuel Odeke added 3 comments![]( "Open in Gerrit")
Thank you Alex for the fixes! Thank you Ian for the code review. Alex, I agree with the insights from Ian's review and kindly ask that we apply them, then LGTM.
continueAlex BrainmanWhy continue rather than returning an error?
Ian Lance TaylorI do not understand how this code works. I never did.
So I am trying to change the code as little as I can.
But I will return error, if you insist.
I'm not insisting. You should do what you think is best.
It's just that it seems to me that if the seek value is too large, then something is wrong. The file is corrupt.
Yeah I agree with Ian's insight about this being a corrupted file and think too that perhaps we should return an error.
} else {Alex BrainmanYou don't need an "else" here.
Ian Lance TaylorI do need that "else", otherwise I cannot use "seek" variable in
```
d = d[seek:]
```
on the next line.
Let me know if you want me to rewrite that code.
Personally I would write it as
seek := ...
if seek >= uint32(len(d)) {
return error
}
d = d[seek:]But you should write it as you see best.
Sure, @alex.b...@gmail.com the composite statement and check has become long
so perhaps let's make that break. Thank you, Ian!
Related details
- Alex Brainman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |