[llvm][ELF]Add Shdr check for getBuildID #126537
Conversation
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-binary-utilities Author: Ruoyu Qiu (cabbaken) ChangesAdd Section Header check for getBuildID, fix crash with invalid Program Header. Full diff: https://github.com/llvm/llvm-project/pull/126537.diff 1 Files Affected:
diff --git a/llvm/lib/Object/BuildID.cpp b/llvm/lib/Object/BuildID.cpp
index 89d6bc3ab550db8..8bb15c89e02e884 100644
--- a/llvm/lib/Object/BuildID.cpp
+++ b/llvm/lib/Object/BuildID.cpp
@@ -24,6 +24,22 @@ using namespace llvm::object;
namespace {
template <typename ELFT> BuildIDRef getBuildID(const ELFFile<ELFT> &Obj) {
+ auto ShdrsOrErr = Obj.sections();
+ if (!ShdrsOrErr) {
+ consumeError(ShdrsOrErr.takeError());
+ return {};
+ }
+ for (const auto &S : *ShdrsOrErr) {
+ if (S.sh_type != ELF::SHT_NOTE)
+ continue;
+ Error Err = Error::success();
+ for (auto N : Obj.notes(S, Err))
+ if (N.getType() == ELF::NT_GNU_BUILD_ID &&
+ N.getName() == ELF::ELF_NOTE_GNU)
+ return N.getDesc(S.sh_addralign);
+ consumeError(std::move(Err));
+ }
+
auto PhdrsOrErr = Obj.program_headers();
if (!PhdrsOrErr) {
consumeError(PhdrsOrErr.takeError());
|
|
I referred related logic of |
cabbaken
changed the title
cabbaken
changed the title
Add Section Header check for getBuildID, fix crash with invalid Program Header. Signed-off-by: Ruoyu Qiu <[email protected]>
cabbaken
changed the title
cabbaken
changed the title
|
This patch modifies a shared cpp file that makes |
Add Aligh check to avoid crashing if Aligh is 0. Signed-off-by: Ruoyu Qiu <[email protected]>
ok, I will modify it.
Yes, I am trying to construct a test, but I need to learn some basic syntax of the test-suite. I will add the test later. |
cabbaken
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Signed-off-by: Ruoyu Qiu <[email protected]>
Signed-off-by: Ruoyu Qiu <[email protected]>
avoid crashing of getDesc(). Signed-off-by: Ruoyu Qiu <[email protected]>
Signed-off-by: Ruoyu Qiu <[email protected]>
I will add the test file in the next commit, I'm working on it now.
Yes, I tested that and it works properly! |
That’s exactly what I planned to do.
Got it! |
That and check that there is boundary condition testing already. |
yaml2obj should determine the program header offset (and other properties) based on the intended values rather than the final `sh_offset` of the section header. `setProgramHeaderLayout` uses section offsets for determining `p_offset`. Move section header overriding after `setProgramHeaderLayout` to prevent `ShOffset` from affecting program header `p_offset`. This change adjusts the timing of when the section header is overridden to ensure that the program headers are set correctly. More details [here](#126537 (comment)). --------- Signed-off-by: Ruoyu Qiu <[email protected]> Signed-off-by: Ruoyu Qiu <[email protected]> Co-authored-by: Ruoyu Qiu <[email protected]>
|
Theoretically, this should pass the check after the merge of 130942. |
…#130942) yaml2obj should determine the program header offset (and other properties) based on the intended values rather than the final `sh_offset` of the section header. `setProgramHeaderLayout` uses section offsets for determining `p_offset`. Move section header overriding after `setProgramHeaderLayout` to prevent `ShOffset` from affecting program header `p_offset`. This change adjusts the timing of when the section header is overridden to ensure that the program headers are set correctly. More details [here](llvm/llvm-project#126537 (comment)). --------- Signed-off-by: Ruoyu Qiu <[email protected]> Signed-off-by: Ruoyu Qiu <[email protected]> Co-authored-by: Ruoyu Qiu <[email protected]>
Yes, I'd do that. I believe the "Update branch" button just does a merge from main. |
It's passed the check now~ |
Did you check to see if there was any boundary condition testing for this case? |
I've added a unit test to ensure the function checks both the program header and the section header to retrieve the build ID. To clarify, are you suggesting that I should also add an additional build ID check to the test file at |
|
A boundary condition is checking that comparison is checking the right value and prevents off-by-one style errors (e.g. it shows that We don't need to add a lit test, as previously discussed in the review. |
|
Thanks for the reminder about why an invalid program header causes llvm-objdump to crash. What would be the best approach for the test? Should I add a new test case to llvm/test/tools/llvm-objdump/ELF/program-headers.test, or is it better to modify an existing one? |
I literally just said don't add a new lit test. You have an existing set of unit tests that you're adding in this patch. You could either add duplicates of them, or probably better, change them to test the edge values, instead of |
|
After reading the unit test I wrote, I noticed something curious. In 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()) {
Err =
createError("invalid offset (0x" + Twine::utohexstr(Phdr.p_offset) +
") or size (0x" + Twine::utohexstr(Phdr.p_filesz) + ")");
return Elf_Note_Iterator(Err);
}The --- !ELF
FileHeader:
Class: ELFCLASS64
Data: ELFDATA2LSB
Type: ET_EXEC
Machine: EM_X86_64
ProgramHeaders:
- Type: PT_NOTE
FileSize: 0xffffffffffffffff
FirstSec: .note.gnu.build-id
LastSec: .note.gnu.build-id
Sections:
- Name: .note.gnu.build-id
Type: SHT_NOBITS
AddressAlign: 0x04Given this, I think we should avoid iterating over I'm sorry I was misunderstanding about this:
|
Modify boundary issue in test. Signed-off-by: Ruoyu Qiu <[email protected]>
llvm/include/llvm/Object/ELF.h
Outdated
There was a problem hiding this comment.
Since these are independent of your wider change, they should be their own PR, with appropriate unit testing, I think.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, and I see you've put a PR up for it.
llvm/include/llvm/Object/ELF.h
Outdated
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, I should add that the current case is a good way of testing the overflow boundary case in the other PR.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Now that I add the boundary check in another PR, should I remove the boundary value here?
There was a problem hiding this comment.
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.
Co-authored-by: James Henderson <[email protected]>
Signed-off-by: Ruoyu Qiu <[email protected]>
|
Yes, please merge it. Thanks! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/16977 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/12892 Here is the relevant piece of the build log for the reference |
Add Section Header check for getBuildID, fix crash with invalid Program Header. Fixes: llvm#126418 --------- Signed-off-by: Ruoyu Qiu <[email protected]> Signed-off-by: Ruoyu Qiu <[email protected]> Co-authored-by: Ruoyu Qiu <[email protected]> Co-authored-by: James Henderson <[email protected]>
5 participants
Add Section Header check for getBuildID, fix crash with invalid Program Header.
Fixes: #126418