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.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Here's a first round of comments.
| include/llvm/BinaryFormat/Dwarf.h | ||
|---|---|---|
| 65–66 ↗ | (On Diff #103614) | 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() |
Comment Actions
This looks much better!
| include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h | ||
|---|---|---|
| 57–59 ↗ | (On Diff #104024) | Can this be changed into just one I find it cleaner that the piecewise interface. |
| 60 ↗ | (On Diff #104024) | 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? |
Comment Actions
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.
Comment Actions
Some more comments:
| include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h | ||
|---|---|---|
| 58 ↗ | (On Diff #104213) | you don't really need this, you can do getAtomsDesc().size(); |
| lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp | ||
| 75–76 ↗ | (On Diff #104213) | for (auto Atom : getAtomDesc()) {
AtomType Type = Atom.first;
Form Form = Atom.second;
... |
| 82 ↗ | (On Diff #104213) | 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. |
Comment Actions
Added test on an object file without errors in the .apple_names section.
Addressed comments.
| include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h | ||
|---|---|---|
| 58 ↗ | (On Diff #104213) | ...good point |
Comment Actions
This LGTM. Few nitpicks you might wanna address before committing:
| lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp | ||
|---|---|---|
| 84 ↗ | (On Diff #104287) | 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 ↗ | (On Diff #104287) | No big deal, but I would just add these checks to the test/DebugInfo/dwarfdump-accel.test test. |
. at the end