Skip to content

debug/elf: return error on reading from SHT_NOBITS sections #54994

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

debug/elf: return error on reading from SHT_NOBITS sections #54994

wants to merge 6 commits into from

Conversation

ZekeLu
Copy link
Contributor

@ZekeLu ZekeLu commented Sep 10, 2022 •
edited
Loading

An SHT_NOBITS section contains no bytes and occupies no space in the
file. This change makes it return an error on reading from this section
so that it will force the caller to check for an SHT_NNOBITS section.

We have considered another option to return "nil, nil" for the Data
method. It's abandoned because it might lead a program to simply do
the wrong thing, thinking that the section is empty.

Please note that it breaks programs which expect a byte slice with the
length described by the sh_size field. There are two reasons to
introduce this breaking change: 1. SHT_NOBITS means no data and it's
unnecessary to allocate memory for it; 2. it could result in an OOM if
the file is corrupted and has a huge sh_size.

Fixes #54967.

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/go/+/429601 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 Dan Kortschak:

Patch Set 1:

(1 comment)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/go/+/429601 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 Zeke Lu:

Patch Set 1:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 2:

(1 comment)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/go/+/429601 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 Zeke Lu:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Zeke Lu:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Zeke Lu:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Zeke Lu:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Dan Kortschak:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Dan Kortschak:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Zeke Lu:

Patch Set 3:

(2 comments)

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

@gopherbot
Copy link
Contributor

Message from Zeke Lu:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Dan Kortschak:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Zeke Lu:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Dan Kortschak:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Zeke Lu:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Alessandro Arzilli:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Alessandro Arzilli:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Alessandro Arzilli:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Alessandro Arzilli:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Zeke Lu:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Zeke Lu:

Patch Set 4:

(2 comments)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 6:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 6:

(1 comment)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/go/+/429601 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 Zeke Lu:

Patch Set 7:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 7: Run-TryBot+1

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 7:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 7: TryBot-Result+1

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

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

(1 comment)

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

@ZekeLu
debug/elf: avoid using saferio to read data from SHT_NOBITS sections
24f6460 
A SHT_NOBITS section may have a nonzero size, but it occupies no space
in the file. The reader to read from this section is zeroReader, which
never reaches EOF. In this case, if the section size is incorrect and
holds a huge number, the saferio.ReadData does not help. What's worse,
it will eat up the memory a small block by a small block, and finally
results in an OOM.

This change calls "make([]byte, size)" to return the section data
directly. For an incorrect huge size, it panics with "runtime error:
makeslice: len out of range" fast before memory is allocated.

I think a further fix is to set a limit for the size of the SHT_NOBITS
section and refuse to read from this section when the size is larger
than the limit. But I can not find a documented limit for this section.
@ZekeLu
add regression test
f09d09c
@ZekeLu
Revert "add regression test"
dc6dea4 
This reverts commit bdbe75f.
@ZekeLu
debug/elf: return error on reading from SHT_NOBITS sections
d4592cb 
An SHT_NOBITS section contains no bytes and occupies no space in the file.
This change makes it return an error on reading from this section so that
it will force the caller to check for an SHT_NNOBITS section.

We have considered another option to return "nil, nil" for the Data method.
It's abandoned because it might lead a program to simply do the wrong thing,
thinking that the section is empty.

Please note that it breaks programs which expect a byte slice with the
length described by the sh_size field. There are two reasons to introduce
this breaking change: 1. SHT_NOBITS means no data and it's unnecessary to
allocate memory for it; 2. it could reslut in an OOM if the file is
corrupted and has a huge sh_size.
@ZekeLu
adjust test
fb1fd51
@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/go/+/429601 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 Zeke Lu:

Patch Set 8:

(1 comment)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/go/+/429601 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 Zeke Lu:

Patch Set 9:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 9: Run-TryBot+1

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 9:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 9: TryBot-Result+1

(1 comment)

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

gopherbot pushed a commit that referenced this pull request Sep 27, 2022
@ZekeLu @gopherbot
debug/elf: return error on reading from SHT_NOBITS sections
fa13731 
An SHT_NOBITS section contains no bytes and occupies no space in the
file. This change makes it return an error on reading from this section
so that it will force the caller to check for an SHT_NNOBITS section.

We have considered another option to return "nil, nil" for the Data
method. It's abandoned because it might lead a program to simply do
the wrong thing, thinking that the section is empty.

Please note that it breaks programs which expect a byte slice with the
length described by the sh_size field. There are two reasons to
introduce this breaking change: 1. SHT_NOBITS means no data and it's
unnecessary to allocate memory for it; 2. it could result in an OOM if
the file is corrupted and has a huge sh_size.

Fixes #54967.

Change-Id: I0c3ed4e097214fe88413d726a89122105ad45d4f
GitHub-Last-Rev: 994c12d
GitHub-Pull-Request: #54994
Reviewed-on: https://go-review.googlesource.com/c/go/+/429601
Run-TryBot: Ian Lance Taylor <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
@gopherbot
Copy link
Contributor

Message from Dmitri Shuralyov:

Patch Set 9: Code-Review+1

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

@gopherbot
Copy link
Contributor

Message from Dmitri Shuralyov:

Patch Set 9: Auto-Submit+1

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

@gopherbot
Copy link
Contributor

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

@gopherbot gopherbot closed this Sep 27, 2022
@ZekeLu ZekeLu deleted the elf-bss-section branch October 5, 2022 03:18
@kakkoyun kakkoyun mentioned this pull request Feb 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

debug/elf: timeout/oom in NewFile
2 participants
@ZekeLu @gopherbot