[PATCH] D82886: [DebugInfo] Fix a possible crash when reading a malformed .debug_*lists section.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.

In D82886#2147198 <https://reviews.llvm.org/D82886#2147198>, @ikudrin wrote:

> In D82886#2145774 <https://reviews.llvm.org/D82886#2145774>, @dblaikie wrote:
>
> > In D82886#2140817 <https://reviews.llvm.org/D82886#2140817>, @ikudrin wrote:
> >
> > > In D82886#2139657 <https://reviews.llvm.org/D82886#2139657>, @dblaikie wrote:
> > >
> > > > I'm not suggesting it needs to be fixed - but that that codepath (the one that returns zero) is untested - so when it was committed, it was committed without test coverage. It'd be good to add test coverage where it is missing like this.
> > >
> > >
> > > Isn't adding that test coverage orthogonal to this particular patch?
> >
> >
> > Oh yeah, for sure! Didn't mean to suggest it should go here!
>
>
> Well, I've created D83673 <https://reviews.llvm.org/D83673> to add some tests. Please, take a look.


Looks good! (though perhaps a full functionality test would be good - but I guess if we end up changing this API contract at some point, the test'll fail & hopefully someone'll then look at the end-to-end situation)

>>>> Right - but what I mean is if there's only 10 bytes, as in your example - it reads the 4 bytes of DWARF64 mark, then 6 bytes out of the desired 8 - if the length was then reported as 10 (with an error saying the length was garbled/the contents terminated earlier than expected), would that be adequate to no longer need the zero length special case?
>>> 
>>> I am OK with the current convention that if it is not possible to read the length field the code returns zero as the total length. It might be better to make the result `Optional` and return `None` in that case, but I really doubt it is worth investing time in that. Reporting something that was not read from the section (10 in your example) smells not good for me,
>> 
>> Not sure I follow - 10 bytes of the length (as many bytes as were available to read) were read from the section, right? Ah, you mean the length bytes did not describe a length of 10, but 10 bytes were read. *nod* I can see how that's a bit ambiguous, indeed.
> 
> In fact, these 10 bytes even were never read. Only the first 4 bytes were actually read, after which the check for insufficient space triggered an error. Also note, that `getInitialLength()` preserves the value of `Offset` in case of an error, not sets it to the position of the error.

*nod* sure enough - got some mildly mixed feelings still, but seems OK. Thanks for walking me through it!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82886/new/

https://reviews.llvm.org/D82886