Closed Bug 1456147 Opened Last year Closed Last year

Assertion failure: !Done(), at mozilla/BufferList.h:191 when IPC fuzzing

Categories

(Core :: IPC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: Alex_Gaynor, Assigned: Alex_Gaynor)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The root cause appears to be that |Data()| on a |PickleIterator| asserts if you're at the end of the buffer, and |ExtractBuffers| doesn't check for that case here: https://searchfox.org/mozilla-central/source/ipc/chromium/src/base/pickle.cc#427

This is reached via https://searchfox.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#857

I'll have a patch up shortly. This is not a sec issue because this is a release assert, just an aggravation to fuzzing efficiency.
I'm not 100% confident this patch is correct; this causes it to not hit the assert, but given the point of BufferList is to have multiple segments, and PickleIterator::Done() only considers the current one, maybe the correct solution is more intelligent.

A critical eye in the review is appreciated.
Comment on attachment 8970250 [details]
Bug 1456147 - do not fail on an assertion error when calling Pickle::ExtractBuffers on an empty iterator;

https://reviewboard.mozilla.org/r/239048/#review246636

I spent some time staring at the code.  The key here is IterImpl's [invariant 2][], which is maintained (and release asserted) by the Advance method: the Done() condition won't be true unless it's the end of the last segment.  So, Done() really does mean it's done.

It seems reasonable for ExtractBuffers to return false instead of crashing in this case — it looks like all the other cases in the Pickle reading methods for unexpected end-of-message do the same thing.  (And ExtractBuffers isn't called with length 0.)

As for the assertion in Data(), because I was also wondering if it really makes sense: in theory it's possible that the caller isn't about to do something bad (it could be checking RemainingInSegment() and carefully avoiding the parts of the C spec where accessing 0 bytes at a bad pointer is undefined behavior), but in practice it probably will, so I agree that the assertion should stay.

[invariant 2]: https://searchfox.org/mozilla-central/rev/8837610b6c999451435695e800f38d4acbc0a644/mfbt/BufferList.h#168
Attachment #8970250 - Flags: review?(jld) → review+
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9fd4d658111
do not fail on an assertion error when calling Pickle::ExtractBuffers on an empty iterator; r=jld
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f9fd4d658111
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.