Page MenuHomePhabricator

Fix DWARFDataExtractor::getRelocatedValue near EOF
ClosedPublic

Authored by labath on Apr 14 2020, 8:23 AM.

Details

Summary

If we have an (invalid) relocation which relocates bytes which partially
lie outside the range of the relocated section, the getRelocatedValue
would return confusing results. It would first read zero (because that's
what the underlying DataExtractor api does for out-of-bounds reads, and
then relocate that zero anyway.

A more appropriate behavior is to return zero straight away. This is
what this patch does.

Diff Detail

Event Timeline

labath created this revision.Apr 14 2020, 8:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2020, 8:23 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
labath updated this revision to Diff 257377.Apr 14 2020, 9:19 AM
  • add const & to Twine
dblaikie accepted this revision.Apr 14 2020, 9:35 AM

Sounds OK (some slight concern about whether we should have any defined behavior for return values when an Error is produced, but I can see the value in it for dumping, etc)

This revision is now accepted and ready to land.Apr 14 2020, 9:35 AM
jhenderson accepted this revision.Apr 15 2020, 12:17 AM

LGTM, although there's a typo in your summary (a missing closing ')') which should be fixed if you are using it as the commit message.

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp
65

Don't think it needs fixing in this commit, since it's not really related to the change, but an offset of 0x4 here is confusing, given that the end of data is at 0x6.

labath marked an inline comment as done.Apr 15 2020, 3:48 AM
labath added inline comments.
llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp
65

Maybe it's because I'm aware of the implementation, but I don't find that confusing -- we were at offset 4, tried to read 4 bytes, and got an EOF. The fact that the read would have succeeded if we tried to read 2 bytes instead does not give me pause.

This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Apr 15 2020, 9:45 AM
llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp
65

Yeah, I'd go with @jhenderson here that it's somewhat unintuitive - could potentially be useful to show all three values: read offset, length of read, offset that's the end of the data, or something like that. (maybe start offset, end offset would be better), eg: unexpected end of data at offset 0x6 while reading [0x4, 0x8)

dblaikie added inline comments.Apr 15 2020, 9:47 AM
llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp
65

(but I'm not sure how much value the [0x4, 0x8) is - given it's just a fact about the field size we happen to be reading - even though at a broader scope we might be trying to read many more bytes, so is knowing the length of this particular field especially valuable/important/actionable? (fixing the data to have length 0x8 isn't necessarily going to fix this error, for instance - because it'll just try to read the next field and fail there)

Might help diagnose run-away things, where, say, a length-prefixed string read where it would print [0xN, 0xHUGE) and you'd be like "well that's not reasonable"... )

labath marked an inline comment as done.Apr 21 2020, 5:51 AM
labath added inline comments.
llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp
65

Fair enough, I've created D78558 to implement something like that. That version print both the EOF offset as well as the currently attempted read range.

Assuming the user does the usual (recommended) thing of reading a bunch of data in sequence (with the same offset+error / cursor objects) and then checking for errors in bulk, we could theoretically try to be fancy, detect this, and extend the range of the error to cover the data that would be read this way. But that could be too fancy...

jhenderson added inline comments.Apr 21 2020, 6:10 AM
llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp
65

But that could be too fancy...

Yeah, I think that might be going too far.