Page MenuHomePhabricator

[DWARFDebugLine] Check for errors when parsing v2 file/dir lists

Authored by labath on Apr 6 2020, 8:01 AM.



Without this we could silently accept an invalid prologue because the
default DataExtractor behavior is to return an empty string when
reaching the end of file. And empty string is also used to terminate
these lists.

This makes the parsing code slightly more complicated, but this
complexity will go away once the parser starts working with truncating
data extractors. The reason I am doing it this way is because without
this, the truncation would regress the quality of error messages (right
now, we produce bad error messages only near EOF, but truncation would
make everything behave as if it was near EOF).

Diff Detail

Event Timeline

labath created this revision.Apr 6 2020, 8:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 8:01 AM
dblaikie accepted this revision.Apr 6 2020, 11:11 AM

Looks good - some of the suggestions I made don't hold or hold less with one of your other changes applied (truncated data extractors) - but might still be worth considering even combined with that.


I realize this is the way the existing code is - so feel free to ignore this, do it as a follow-up or precommit, or roll it is as you see fit: The "Terminated" flag here seems to complicate this code compared to doing the error handling and returning immediately in the one way out of the loop above where Terminated would be false (this would also allow you to narrow the scope of the the "Error" variable to inside the loop)


If you changed this code to be:

if (!Err && Name.empty()) {
  Terminated = true;

Would that let the later error handling catch handle both errors - then you could do the same as above, and roll the error handling in tighter without needing the "Terminated" flag, etc?


Rather than doing this, could we return early in these cases (eg: have ReportInvalidDirFileTable return Error::Success and have the two calls be "return ReportInvalid..." perhaps? or just two separate statements, (ReportInvalid + return Success)) so the follow-on check is never run, rather than having to create a fake state to avoid it reporting?


Does this only catch cases where the offset is < the end prologue offset now? If so, it might be nice to assert that it's not > and write the test as < and rephrase the error message to be more specific about the mismatch being too short, etc.

This revision is now accepted and ready to land.Apr 6 2020, 11:11 AM
jhenderson added inline comments.Apr 7 2020, 12:24 AM

It was a little bit of time ago since I added the Terminated flag, but IIRC the reason it needed to exist was because a missing null terminator versus running off the section end would be undetectable without it. I think it becomes redundant once the other planned changes are made, since we no longer need to check the offset when reading, and can keep going until the EOF error or a null terminator is hit.


Are there separate test cases for each of these being truncated?


Why did this change?

labath updated this revision to Diff 257673.Apr 15 2020, 4:35 AM
labath marked 7 inline comments as done.
  • add tests
  • remove the "Terminated" flag
labath marked an inline comment as done.Apr 15 2020, 4:36 AM
labath added inline comments.

We can't get rid of checking for termination in a fairly convoluted way (but we will after the other patches), but we can avoid materializing the check into a Terminated variable, which is what I believe David was proposing.


There are now.


I've done that, but slightly differently -- I reorganized the code to remove the lambda and added a return success() after the error is reported.


That isn't fully the case yet -- there are still some situations in which the v5 parser can escape the prologue. However, it will be the case after this starts using truncated data extractors. I'll update this in the other patch.


This is due to the additional error checking in the parser (line 204) -- it now checks for errors before appending an entry to the list and if it detects an error (or that the offset is beyond the prologue bound) it rejects that entries.

jhenderson accepted this revision.Apr 15 2020, 7:48 AM

LGTM, I think. Thanks.

This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.