This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Refine the condition to detect CIEs.
ClosedPublic

Authored by ikudrin on Feb 3 2020, 6:08 AM.

Details

Summary

The condition was not accurate enough and could interpret some FDEs in .eh_frame or 64-bit DWARF .debug_frame sections as CIEs. Even though such FDEs are unlikely in a normal situation, the wrong interpretation could hide an issue in a buggy generator.

Diff Detail

Event Timeline

ikudrin created this revision.Feb 3 2020, 6:08 AM
aprantl added inline comments.Feb 3 2020, 9:43 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
401

Is there any way to break this up into subexpressions and helper variables that would make this condition more easy to follow?

ikudrin marked an inline comment as done.Feb 3 2020, 5:54 PM
ikudrin added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
401

What about this?

bool IsCIE = (!IsEH && !IsDWARF64 && Id == DW_CIE_ID) ||
             (!IsEH && IsDWARF64 && Id == DW64_CIE_ID) ||
             (IsEH && !Id);

Or we may define a helper method, CIE::getCIEId(), like in D73887.

ikudrin updated this revision to Diff 244657.Feb 14 2020, 7:12 AM
  • Simplified the condition by using a function that returns an appropriate CIE id.
probinson added a comment.EditedFeb 25 2020, 12:12 PM

The two tests look like they have an FDE with no preceding CIE? Isn't that invalid?

Well, in one test that's on purpose, but the other one looks like it's being handled as if the input were actually valid.

The two tests look like they have an FDE with no preceding CIE? Isn't that invalid?

The inputs are for sure invalid. The idea was to check that our consumer does not misinterpret them as valid CIEs as it did before the patch.

Well, in one test that's on purpose, but the other one looks like it's being handled as if the input were actually valid.

We allow FDEs without CIEs in .debug_frame, for good or bad. However, that is out of the scope of this patch.

Hmmm. I guess the next question is, do we have to bail out when something doesn't look right? Or can we keep going?
If we see an FDE with no preceding CIE, yes that's something we should report, but the not --crash tells me we are abandoning attempts to continue dumping the section. Does one of your other patches address this?

Hmmm. I guess the next question is, do we have to bail out when something doesn't look right? Or can we keep going?
If we see an FDE with no preceding CIE, yes that's something we should report, but the not --crash tells me we are abandoning attempts to continue dumping the section. Does one of your other patches address this?

DWARFDebugFrame::parse() tends to call report_fatal_error() in case of any trouble; that was initially added in https://reviews.llvm.org/rG705085da37eed9557202ff1e86f9462842ef7af9 and than extended in D15535 (https://reviews.llvm.org/rG03a670c0ec1d38de8c206de1426340041f10f8ff).

While I agree that the parser should not crash the program on invalid input, resolving that lays a bit far away from my current intentions, which are just to add support for 64-bit values here.

ikudrin updated this revision to Diff 246886.Feb 27 2020, 12:58 AM
  • Fixed section flags in the test.

Ping.

@probinson, do you suggest to change the intended behavior to crash on incorrect input before fixing this issue?

probinson accepted this revision.Mar 4 2020, 7:55 AM

Ping.

@probinson, do you suggest to change the intended behavior to crash on incorrect input before fixing this issue?

There is a longer-term effort to make parsing more tolerant, but I see that this is existing behavior in handling .debug_frame and you should not be required to fix it before committing this patch. However, I would appreciate it if you would file a bug about this, mentioning the eh-frame-cie-id.s test, so that the people trying to clean up the parsing behavior can be aware of it.

File that bug and LGTM.

This revision is now accepted and ready to land.Mar 4 2020, 7:55 AM

Thanks!

Here is the bug report.

This revision was automatically updated to reflect the committed changes.