Page MenuHomePhabricator

[libObject] Fix getDesc for Elf_Note_Impl
ClosedPublic

Authored by jakehehrlich on Nov 12 2018, 3:26 PM.

Details

Summary

This change fixes a bug in Elf_Note_Impl in which Elf_Word was used where uint8_t should have been used.

Diff Detail

Event Timeline

jakehehrlich created this revision.Nov 12 2018, 3:26 PM

Overall this looks fine, but can you add a test of something that this fixes? I'm rather surprised that no tests fail now.

Overall this looks fine, but can you add a test of something that this fixes? I'm rather surprised that no tests fail now.

Yeah, I think I spotted bug that could have been spotted in an integration test.

As far as I am aware there are no unit tests for libObject. The reason no tests fail makes sense as well. llvm-readobj was the only user of this and everywhere it used it it did something like the following

std::string(reinterpret_cast<const char *>(Words.data()),
                  Words.size())};

Notice that Words.size() is still 16 so this conversion actually works and fixes the faulty bug.

I think the one spot where an issue could have been noticed is in getGNUAbiTag where despite the size being say 1-3 Elf_Word's the size in bytes would have been between 4-12 all of which would not have triggered the error. I think checking for that error in an integration test would suffice but the code could change in a way that made that test stale.

  • Added test that would have caused undefined behavior before but should be solid now.

Ah, the bugs canceled each other out. Adding a unit test is fine. Just add a new file in llvm/unittests/Object/.

This revision is now accepted and ready to land.Nov 12 2018, 4:19 PM
jakehehrlich closed this revision.Nov 12 2018, 7:17 PM