Skip to content

[llvm][ELF]Add Shdr check for getBuildID #126537

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

Merged
merged 19 commits into from
Oct 3, 2025
Merged

[llvm][ELF]Add Shdr check for getBuildID #126537

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a19e1a0
[llvm-objdump][ELF]Add Shdr buildID check(#126418)
cabbaken Feb 10, 2025
20f7214
Merge branch 'main' into 126418
cabbaken Feb 11, 2025
169f9db
[LLVM][ELF]Add Align check for getDesc()
cabbaken Feb 11, 2025
0c6d355
Optimize getBuildID() code style
cabbaken Feb 12, 2025
7105db4
Merge branch 'main' into 126418
cabbaken Feb 12, 2025
40ff1ff
Fix alignment judge of getDesc()
cabbaken Feb 13, 2025
0a502cd
Add AddressAlign to symbolize-build-id.test to
cabbaken Feb 13, 2025
7193339
Optimize code style.
cabbaken Feb 13, 2025
c388efa
remove unneeded empty check
cabbaken Feb 13, 2025
9393e97
Add invalid filesz of program header(PT_NOTE)
cabbaken Feb 14, 2025
e07a49b
add unit test of getBuildId()
cabbaken Feb 21, 2025
4e83f8a
add test cases for BuildIDTest.
cabbaken Feb 25, 2025
2e8c6ba
add InvalidSectionHeaderTest, modify name of test in BuildIDTest.cpp
Mar 6, 2025
6fa8919
Merge branch 'main' into 126418
cabbaken Sep 18, 2025
b64f287
Fix uint64_t overflow detect.
cabbaken Sep 22, 2025
fece101
Update llvm/unittests/Object/BuildIDTest.cpp
cabbaken Sep 22, 2025
a1971c9
Fix boundary value and comment style
cabbaken Sep 30, 2025
62ee98b
Merge branch 'main' into 126418
cabbaken Sep 30, 2025
47b96a1
Merge branch 'main' into 126418
cabbaken Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions llvm/include/llvm/Object/ELF.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,8 @@ class ELFFile {
Elf_Note_Iterator notes_begin(const Elf_Phdr &Phdr, Error &Err) const {
assert(Phdr.p_type == ELF::PT_NOTE && "Phdr is not of type PT_NOTE");
ErrorAsOutParameter ErrAsOutParam(Err);
if (Phdr.p_offset + Phdr.p_filesz > getBufSize()) {
if (Phdr.p_offset + Phdr.p_filesz > getBufSize() ||
Phdr.p_offset + Phdr.p_filesz < Phdr.p_offset) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these are independent of your wider change, they should be their own PR, with appropriate unit testing, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This serves as an overflow check, and the unit test would fail without it. Do you think I should create a separate PR to merge this check first?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, and I see you've put a PR up for it.

Err =
createError("invalid offset (0x" + Twine::utohexstr(Phdr.p_offset) +
") or size (0x" + Twine::utohexstr(Phdr.p_filesz) + ")");
Expand Down Expand Up @@ -435,7 +436,8 @@ class ELFFile {
Elf_Note_Iterator notes_begin(const Elf_Shdr &Shdr, Error &Err) const {
assert(Shdr.sh_type == ELF::SHT_NOTE && "Shdr is not of type SHT_NOTE");
ErrorAsOutParameter ErrAsOutParam(Err);
if (Shdr.sh_offset + Shdr.sh_size > getBufSize()) {
if (Shdr.sh_offset + Shdr.sh_size > getBufSize() ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

Shdr.sh_offset + Shdr.sh_size < Shdr.sh_offset) {
Err =
createError("invalid offset (0x" + Twine::utohexstr(Shdr.sh_offset) +
") or size (0x" + Twine::utohexstr(Shdr.sh_size) + ")");
Expand Down
9 changes: 6 additions & 3 deletions llvm/unittests/Object/BuildIDTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ static StringRef getInvalidNoteELF(bool WithShdr) {
Machine: EM_X86_64
ProgramHeaders:
- Type: PT_NOTE
FileSize: 0xffffffffffffffff
FileSize: 0xffffffffffffff88
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I follow why this is 0xffffffffffffff88 and not 0xffffffffffffffff like it was before (or indeed some other value that better tests the boundary limits).

Copy link
Contributor Author

@cabbaken cabbaken Sep 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In ELF.h, we calculate Phdr.p_offset + Phdr.p_filesz. With p_offset = 0x78, adding it to 0xffffffffffffff88 causes an overflow (wrapping around to 0).
In this case, it effectively works as a boundary value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not what I had in mind, since I was referring to the original non-overflow checks (i.e. Phdr.p_offset + Phdr.p_filesz > getBufSize() and its section header equivalent). In other words, show that if Phdr.p_offset + Phdr.p_filesz == getBufSize() + 1, we get the error case (arguably there should be a separate test case where Phdr.p_offset + Phdr.p_filesz == getBufSize()). The same applies for the section header case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I should add that the current case is a good way of testing the overflow boundary case in the other PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I should add that the current case is a good way of testing the overflow boundary case in the other PR.

Yes, the boundary value is intended for the overflow check.
In this PR, I’ll remove the unrelated parts so it focuses on its main subject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I add the boundary check in another PR, should I remove the boundary value here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please refer back to this comment above. I think that should mostly answer your question. It's enough to only check one of the paths there in this PR, because what you're after is test the behaviour when an error is encountered in the underlying code.

FirstSec: .note.gnu.build-id
LastSec: .note.gnu.build-id
Sections:
Expand All @@ -61,6 +61,7 @@ static StringRef getInvalidNoteELF(bool WithShdr) {
return WithoutSection;
}

// The BuildID can be looked up from a section header, if there is no program header.
TEST(BuildIDTest, InvalidPhdrFileSizeWithShdrs) {
SmallString<0> Storage;
Expected<ELFObjectFile<ELF64LE>> ElfOrErr =
Expand All @@ -72,6 +73,7 @@ TEST(BuildIDTest, InvalidPhdrFileSizeWithShdrs) {
"\xAB\xB5\x0D\x82\xB6\xBD\xC8\x61");
}

// The code handles a malformed program header that points at data outside the file.
TEST(BuildIDTest, InvalidPhdrFileSizeNoShdrs) {
SmallString<0> Storage;
Expected<ELFObjectFile<ELF64LE>> ElfOrErr =
Expand All @@ -80,9 +82,10 @@ TEST(BuildIDTest, InvalidPhdrFileSizeNoShdrs) {
BuildIDRef BuildID = getBuildID(&ElfOrErr.get());
EXPECT_EQ(
StringRef(reinterpret_cast<const char *>(BuildID.data()), BuildID.size()),
"\xAB\xB5\x0D\x82\xB6\xBD\xC8\x61");
"");
}

// the code handles a malformed section header that points at data outside the file.
TEST(BuildIDTest, InvalidSectionHeader) {
SmallString<0> Storage;
Expected<ELFObjectFile<ELF64LE>> ElfOrErr = toBinary<ELF64LE>(Storage, R"(
Expand All @@ -100,7 +103,7 @@ TEST(BuildIDTest, InvalidSectionHeader) {
- Name: .note.gnu.build-id
Type: SHT_NOTE
AddressAlign: 0x04
ShOffset: 0x8000
ShOffset: 0xffffffffffffff88
Notes:
- Name: "GNU"
Desc: "abb50d82b6bdc861"
Expand Down
Loading