Page MenuHomePhabricator

[DWARF] Prevent crash when .debug_line line_range is zero
Changes PlannedPublic

Authored by jhenderson on Feb 19 2018, 8:28 AM.

Details

Summary

This change adds a check for a .debug_line line_range value of 0 to prevent the program crashing when a special opcode or DW_LNS_const_add_pc is detected. The diagnostic is only issued once per table, and for this case the opcodes do not adjust the address or line.

Diff Detail

Event Timeline

jhenderson created this revision.Feb 19 2018, 8:28 AM
aprantl added inline comments.Feb 19 2018, 8:32 AM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
309 ↗(On Diff #134927)

Can you also make it so that llvm-dwarfdump --verify reports this as an error?

aprantl accepted this revision.Feb 19 2018, 8:32 AM
aprantl added inline comments.
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
309 ↗(On Diff #134927)

Nevermind. That's what you did!

This revision is now accepted and ready to land.Feb 19 2018, 8:32 AM
aprantl added inline comments.Feb 19 2018, 8:34 AM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
302 ↗(On Diff #134927)

Bonus points for fixing this one, too :-)

309 ↗(On Diff #134927)

Can you use DWARFVerifier::warn() here?

jhenderson added inline comments.Feb 19 2018, 9:31 AM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
290 ↗(On Diff #134927)

And presumably this one too @aprantl?

309 ↗(On Diff #134927)

I'll take a look for this and the other usages here.

llvm-dwarfdump --verify reports an error automatically if false is returned here, so that's how we get an error, as demonstrated in the test.

aprantl added inline comments.Feb 19 2018, 9:33 AM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
290 ↗(On Diff #134927)

That would be great. Can be a separate commit, and you don't need to open a review for that one.

jhenderson added inline comments.Feb 19 2018, 9:36 AM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
309 ↗(On Diff #134927)

Can you use DWARFVerifier::warn() here?

Just quickly looking at this before I leave the office. This function is currently private, and relies on an instance of DWARFVerifier, which I don't think is available. Do you mean to make it do the same thing as what DWARFVerifier::warn() does?

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

jhenderson planned changes to this revision.Feb 20 2018, 3:21 AM

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

Having thought about this a bit and played around, I'm inclined to agree that this is the correct approach. It'll be a bigger change, but it will make the code more amenable to being used as a library, where the consumers can decide what to do with the errors/warnings on a case-by-case basis. I'll experiment with it a bit more, and then upload an updated patch.

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

Having thought about this a bit and played around, I'm inclined to agree that this is the correct approach. It'll be a bigger change, but it will make the code more amenable to being used as a library, where the consumers can decide what to do with the errors/warnings on a case-by-case basis. I'll experiment with it a bit more, and then upload an updated patch.

Great, thanks!

jhenderson edited the summary of this revision. (Show Details)
jhenderson added reviewers: ikudrin, MaskRay.
jhenderson changed the repository for this revision from rL LLVM to rG LLVM Github Monorepo.
jhenderson added a subscriber: chrisjackson.

Resurrecting this change, by rewriting the fix on the latest HEAD. In this version, the check is done when a special opcode or DW_LNS_const_add_pc is encountered. An Error is reported if the line_range is 0 for the first such instance of these. Testing is now done via unit tests.

This revision is now accepted and ready to land.Fri, Feb 14, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Feb 14, 7:54 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jhenderson requested review of this revision.Fri, Feb 14, 7:55 AM

Please could this be re-reviewed, given the significant changes in the area since the last review.

jhenderson planned changes to this revision.Wed, Feb 19, 1:14 AM

I missed a case that needs checking. Will revisit.