Closed Bug 1456147 Opened 3 years ago Closed 3 years ago
Assertion failure: !Done(), at mozilla/Buffer
List .h:191 when IPC fuzzing
Bug 1456147 - do not fail on an assertion error when calling Pickle::ExtractBuffers on an empty iterator;
59 bytes, text/x-review-board-request
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+
Pushed by firstname.lastname@example.org: 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
You need to log in before you can comment on or make changes to this bug.