This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Add checks for v2 directory and file name table terminators
ClosedPublic

Authored by jhenderson on Feb 11 2020, 7:47 AM.

Details

Summary

The DWARFv2-4 specification for the line table header states that the include directories and file name tables both end with a single null byte. Prior to this change, the parser did not detect if this byte was missing, because it also stopped reading the tables once it reached the prologue end, as claimed by the header_length field. This change adds a check that the terminator has been seen at the end of each table.

Diff Detail

Event Timeline

jhenderson created this revision.Feb 11 2020, 7:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2020, 7:47 AM
dblaikie accepted this revision.Feb 11 2020, 9:47 AM
dblaikie added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
165–171

Perhaps this'd be a little more terse (but not necessarily better) written as:

Terminated = S.empty();
if (Terminated)
  break;

or maybe

if ((Terminated = S.empty()))
  break;

Not necessary if you prefer the current version.

I'd probably more strongly suggest making the variable names a bit simpler - probably just using "Terminated" for both.

178

maybe make it a bit more explicit by saying "null terminated"?

This revision is now accepted and ready to land.Feb 11 2020, 9:47 AM
MaskRay added inline comments.Feb 11 2020, 6:30 PM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
165–171

Terminated +1

The variable IncludeDirectories below suggests what Terminated means.

Both the current form and

if ((Terminated = S.empty()))
  break;

look fine to me.

MaskRay accepted this revision.Feb 11 2020, 6:30 PM
jhenderson marked 5 inline comments as done.Feb 12 2020, 4:41 AM
jhenderson added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
165–171

I think I marginally prefer the current form for readability reasons, so I'll leave that as is. The renaming makese sense though. I considered making that change before putting it up for review.

178

That's fine with me, although I should point out that the strings themselves are always null terminated, so even if the null terminator byte is missing, the table will still have a null byte at the end.

jhenderson marked 2 inline comments as done.

Rebase and fixed a unit test.

Remove accidental changes to unit test caused by bad rebase/merge.

This revision was automatically updated to reflect the committed changes.