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

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1456147 - do not fail on an assertion error when calling Pickle::ExtractBuffers on an empty iterator;

Review commit: https://reviewboard.mozilla.org/r/239048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239048/

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

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 3

[Jed Davis [:jld] (on leave until 08-31)]

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
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

Comment 5

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f9fd4d658111