This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Verification of the validity of the hash data offset and hash data DIEs in the the .apple_names section.
ClosedPublic

Authored by sgravani on Jun 19 2017, 11:43 AM.

Details

Summary

This patch adds checks for the validity of the hash data offset in the .apple_names accelerator table.
It also adds checks for the validity of the DIEs in the actual hash data.

Diff Detail

Event Timeline

sgravani created this revision.Jun 19 2017, 11:43 AM
aprantl added inline comments.Jun 19 2017, 3:29 PM
lib/DebugInfo/DWARF/DWARFVerifier.cpp
313

. at the end

318

should we just say 64 here?

test/tools/llvm-dwarfdump/X86/apple_names_verify_hashdata_offset.s
3 ↗(On Diff #103085)

Since the verifier continues on error, could this test be merged with any of the existing .apple_names tests?

sgravani updated this revision to Diff 103614.Jun 22 2017, 11:55 AM
sgravani retitled this revision from [DWARF] Verification of the validity of the hash data offsets in the .apple_names section. to [DWARF] Verification of the validity of the hash data offset and hash data DIEs in the the .apple_names section..
sgravani edited the summary of this revision. (Show Details)
sgravani added a reviewer: friss.

Added more checks for the validity of the .apple_names section.

friss edited edge metadata.Jun 23 2017, 1:46 PM

Here's a first round of comments.

include/llvm/BinaryFormat/Dwarf.h
65–66

Why is this not a con unit32_t? I feel like the comment should explain what it is too.

include/llvm/DebugInfo/DWARF/DWARFContext.h
279–280 ↗(On Diff #103614)

This needs a comment, and I'm not fond of the name. It's not really descriptive.

include/llvm/DebugInfo/DWARF/DWARFFormValue.h
96–98 ↗(On Diff #103614)

I can see the usefulness of the getAsReference helper, but it should really return an Optional<uint64_t>. I think you should just use getAsUnsignedConstant() instead of adding an unchecked version.

include/llvm/DebugInfo/DWARF/DWARFUnit.h
301–310 ↗(On Diff #103614)

DIEs are sorted by offset, so this should use some kind of binary search. Also, I think the API is overly constrained. What about "DWARFDie getDIEAtOffset(nuint32_t offset)" ? It would return an invalid DwarfDIE if that offset is not valid.

lib/DebugInfo/DWARF/DWARFFormValue.cpp
359–360 ↗(On Diff #103614)

All those added cases don't change the behavior, just leave this file as it is.

lib/Support/DataExtractor.cpp
131–137 ↗(On Diff #103614)

This could be implemented in terms of getCStr()

sgravani updated this revision to Diff 104024.Jun 26 2017, 2:39 PM

Addressed reviewer's comments.

sgravani marked 2 inline comments as done.Jun 26 2017, 2:40 PM
sgravani marked 5 inline comments as done.
friss added a comment.Jun 26 2017, 3:43 PM

This looks much better!

include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
58–60

Can this be changed into just one
ArrayRef<std::pair<AtomType, Form>> getAtomsDesc()
?

I find it cleaner that the piecewise interface.

61

It's not obvious what the function does, what the argument is and what it returns. Please add a doxygen comment.

lib/DebugInfo/DWARF/DWARFVerifier.cpp
331–337

The error doesn't seem to reflect the test. Also, unless I'm missing something, you should be able to do this test once outside of the loop.

346–349

I would return here. I think this is a fatal error. There's no point in continuing to try to decode the bitstream when you failed to read a piece of it.

test/tools/llvm-dwarfdump/X86/apple_names_verify_buckets_hashdataoffset.s
107 ↗(On Diff #104024)

Do you need all the other sections? Wouldn't it work just with the __apple_names section?

sgravani updated this revision to Diff 104213.Jun 27 2017, 10:48 AM
sgravani marked 5 inline comments as done.

Added isSupportedForm() function in DWARFFormValue class that will help us get more detailed error messages
when reading the Atoms in apple_names (and in future any accelerator table) during verification.

Addressed reviewer's comments.

friss added a comment.Jun 27 2017, 1:24 PM

Some more comments:

include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
58

you don't really need this, you can do getAtomsDesc().size();

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
75–76
for (auto Atom : getAtomDesc()) {
    AtomType Type = Atom.first;
    Form Form = Atom.second;
...
82

Either you add a check here that the Optional is populated, or you check before iterating that the description of the die_offset has a form that can be decoded by getAsUnsignedConstant(). As written this can crash.

sgravani updated this revision to Diff 104287.Jun 27 2017, 3:40 PM
sgravani marked 3 inline comments as done.

Added test on an object file without errors in the .apple_names section.
Addressed comments.

include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
58

...good point

friss accepted this revision.Jun 27 2017, 5:48 PM

This LGTM. Few nitpicks you might wanna address before committing:

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
84

I think you can do that test only once for the whole table instead of at each iteration.

test/tools/llvm-dwarfdump/X86/apple_names.test
1–5

No big deal, but I would just add these checks to the test/DebugInfo/dwarfdump-accel.test test.

This revision is now accepted and ready to land.Jun 27 2017, 5:48 PM
sgravani closed this revision.Jun 29 2017, 1:31 PM
sgravani marked 2 inline comments as done.
test/tools/llvm-dwarfdump/X86/apple_names_verify_atoms.s