Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

debug/elf: use saferio.SliceCap when decoding ELF sections #56405

Closed
wants to merge 1 commit into from

Conversation

ZekeLu
Copy link
Contributor

@ZekeLu ZekeLu commented Oct 24, 2022

This avoids allocating an overly large slice for corrupt input.

No test case because the problem can only happen for invalid data. Let
the fuzzer find cases like this.

Updates #33121.

This avoids allocating an overly large slice for corrupt input.

No test case because the problem can only happen for invalid data. Let
the fuzzer find cases like this.
@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/go/+/445076 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
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 1: Run-TryBot+1


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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1:

(1 comment)


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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1: TryBot-Result+1

(1 comment)


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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 1: Run-TryBot+1 Auto-Submit+1 Code-Review+2

(1 comment)


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

Copy link

@rockdaboot rockdaboot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I confirm that this change fixes the issue for me.

gopherbot pushed a commit that referenced this pull request Oct 25, 2022
This avoids allocating an overly large slice for corrupt input.

No test case because the problem can only happen for invalid data. Let
the fuzzer find cases like this.

Updates #33121.

Change-Id: Ie2d947a3865d3499034286f2d08d3e3204015f3e
GitHub-Last-Rev: 6c62fc3
GitHub-Pull-Request: #56405
Reviewed-on: https://go-review.googlesource.com/c/go/+/445076
Run-TryBot: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>
@gopherbot
Copy link
Contributor

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

@gopherbot gopherbot closed this Oct 25, 2022
@rockdaboot
Copy link

@ZekeLu Just out of curiosity (and for planning): Will the fix go into 1.19.3 or only into 1.20 ?
Because without the fix we can not always recover. Depending on the shnum value, there is either a recoverable panic or a fatal error (fatal error: runtime: out of memory). The error can not be recover()'ed from.

@ZekeLu
Copy link
Contributor Author

ZekeLu commented Oct 25, 2022

Hi @rockdaboot, no backport request is created, so it won't go into 1.19.x.

If you want to make it capable for fuzzing, I think the following commits should be backported too:

I'm not sure whether they are qualified for backporting to the 1.19 branch. Maybe @ianlancetaylor can answer your question.

@ianlancetaylor
Copy link
Member

I don't think these kinds of changes meet our backport criteria (https://go.dev/wiki/MinorReleases). Sorry.

@rockdaboot
Copy link

@ZekeLu Thanks GTN. I fuzz using latest master. But I think we have to roll out our own code to parse ELF files (we are mostly interested in getting symbols).

@ianlancetaylor
Copy link
Member

it shouldn't be too hard to use your own copy of the master version of debug/elf for now.

@ZekeLu ZekeLu deleted the elf-shnum branch October 27, 2022 01:31
romaindoumenc pushed a commit to TroutSoftware/go that referenced this pull request Nov 3, 2022
This avoids allocating an overly large slice for corrupt input.

No test case because the problem can only happen for invalid data. Let
the fuzzer find cases like this.

Updates golang#33121.

Change-Id: Ie2d947a3865d3499034286f2d08d3e3204015f3e
GitHub-Last-Rev: 6c62fc3
GitHub-Pull-Request: golang#56405
Reviewed-on: https://go-review.googlesource.com/c/go/+/445076
Run-TryBot: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants