[webkit-reviews] review granted: [Bug 210844] IPC::decodeSharedBuffer() should check the return value of SharedMemory::map() : [Attachment 397167] Patch v1

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 210844: IPC::decodeSharedBuffer() should check the return value of
SharedMemory::map()
https://bugs.webkit.org/show_bug.cgi?id=210844

Attachment 397167: Patch v1

https://bugs.webkit.org/attachment.cgi?id=397167&action=review




--- Comment #4 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 397167
  --> https://bugs.webkit.org/attachment.cgi?id=397167
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=397167&action=review

r=me

>>> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:179
>>> +	     return false;
>> 
>> When does this situation arise? Is "return false" sufficient here? Should we
be crashing instead? Or making the process that passed us this handle crash?
> 
> There are numerous reasons that SharedMemory::map() can fail:
> 
>
<https://trac.webkit.org/browser/trunk/Source/WebKit/Platform/cocoa/SharedMemor
yCocoa.cpp#L212>
> 
> I think `return false` is sufficient here because callers of
IPC::decodeSharedBuffer() always check their return values via
WARN_UNUSED_RETURN:
> 
> static WARN_UNUSED_RETURN bool decodeSharedBuffer(Decoder& decoder,
RefPtr<SharedBuffer>& buffer)
> 
> In general, decoding is a task where failures are (must be) handled since
we're dealing with untrusted input.

I think Darin is agreeing that decoding should handle failure, and then asking
whether our failure handling should include killing the sender (to avoid giving
a malicious sender multiple attempts at the same exploit).

I guess my take is that decodeSharedBuffer is used by lots of different
clients, so it may not be appropriate for decodeSharedBuffer to kill the sender
in all cases. But maybe we should audit callers of decodeSharedBuffer to
consider whether they should kill the sender on failure. Here's an example I
found that might want to MESSAGE_CHECK a PasteboardWebContent, which contains a
SharedBuffer:

    void WebPasteboardProxy::writeWebContentToPasteboard(IPC::Connection&
connection, const WebCore::PasteboardWebContent& content, const String&
pasteboardName)