[PATCH] D35166: [DWARF] Introduce verification for the unit header chain in .debug_info section to llvm-dwarfdump.

[David Blaikie via llvm-commits]

On Mon, Jul 10, 2017 at 5:21 PM Spyridoula Gravani via Phabricator <
reviews at reviews.llvm.org> wrote:

> sgravani added inline comments.
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:250
> +  static uint32_t computeDWARF5HeaderSize(uint8_t UnitType) {
> +    uint32_t HeaderSize = 12;
> +    switch (UnitType) {
> ----------------
> dblaikie wrote:
> > Might be nice for consistency to have the default in the switch as well.
> >
> > Also might be easier to return directly rather than having an extra
> variable:
> >
> >   switch (UnitType) {
> >   case skeleton:
> >   case split_compile:
> >     return 20;
> >   case type:
> >   case split-type:
> >     return 24;
> >   case compile:
> >   case partial:
> >   default:
> >     return 12;
> >   }
> >
> > That said, should this switch have a default? Or is it reasonable to say
> "this must be called with a valid unit type, of which there are only these
> 6"?
> I think it makes more sense not to have the default. I modified this to
> reflect that the only valid arguments are these 6 types for dwarf5.
> Thanks!
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:423-425
> +    OS << "Verification of .debug_info unit header chain failed; no need
> to "
> +          "verify .debug_info contents\n";
> +    Success = false;
> ----------------
> dblaikie wrote:
> > Arguably, each unit with a valid header (that doesn't go off the end,
> etc) could be validated - just because one header is mangled doesn't mean
> the rest are (granted if it's mangled in a way that means the length is
> wrong and the rest of the headers from then on are probably off - then
> there's not much you can do with everything after the broken one, of
> course). Not necessary, but not impossible either. If it's going to stay
> this way maybe the error could be rephrased "unit contents will not be
> verified" (not that tehy don't need to be, or that they wouldn't benefit
> from being verified, etc)
> I agree that there's still value in verifying the contents of .debug_info,
> even when the  unit header  chain check fails. I'm changing this to have
> verification of the debug info section in any case. Thanks.
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:50
> +    OffsetStart = Offset;
> +    ValidOffset = DebugInfoData.isValidOffset(Offset);
> +    Length = DebugInfoData.getU32(&Offset);
> ----------------
> dblaikie wrote:
> > Looks like this could never return false - since if the
> Offset+HeaderSize is a valid offset, then 'Offset' must be too (since it's
> a zero based array index - so X + N (where X and N are positive) can't be
> valid while X is invalid)
> You're right. I fixed this part.
>
>
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:75
> +      ++NumDebugInfoUnitHeaderChainErrors;
> +      OS << format("Unit Start Offset: 0x%08x \n", OffsetStart);
> +      if (ValidOffset) {
> ----------------
> dblaikie wrote:
> > When does this happen? Seems like it could only happen if the debug_info
> section was empty, maybe? (Otherwise the previous unit would've already
> failed because it was too long) Perhaps it'd be better to have a special
> case for empty? (or maybe it's not an error, really?)
> You're right. The only case would be if .debug_info section was empty.
> I'm not sure if we should handle this as an error.
> I think I would like to go with printing a warning message for this case
> (since we're verifying the .debug_info section and .debug_info is empty).
>

So long as this doesn't trigger wehn there's no debug_info section,
perhaps? (someone might just be trying to verify that any debug info that
is present, is valid - but may also be running on objects that have no
debug info whatsoever)


>
>
> https://reviews.llvm.org/D35166
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170711/8470c8e6/attachment.html>