When I start downloading a certain, non-publicly-accessible file, WebKit crashes. In Debug builds, it asserts first. The assertion is this: ASSERT(getClass() == MessageKindTraits<EnumType>::messageClass); getClass() is 18, which is MessageClassDownloadProxy, while MessageKindTraits<EnumType>::messageClass is MessageClassWebPageProxy. Here's the backtrace of the assertion: > WebKit.dll!CoreIPC::MessageID::get<enum Messages::WebPageProxy::Kind>() Line 136 + 0x31 bytes C++ WebKit.dll!WebKit::WebPageProxy::didReceiveSyncWebPageProxyMessage(CoreIPC::Connection * __formal=0x0a5b3380, CoreIPC::MessageID messageID={...}, CoreIPC::ArgumentDecoder * arguments=0x0f897c48, WTF::OwnPtr<CoreIPC::ArgumentEncoder> & reply={...}) Line 434 + 0x8 bytes C++ WebKit.dll!WebKit::WebPageProxy::didReceiveSyncMessage(CoreIPC::Connection * connection=0x0a5b3380, CoreIPC::MessageID messageID={...}, CoreIPC::ArgumentDecoder * arguments=0x0f897c48, WTF::OwnPtr<CoreIPC::ArgumentEncoder> & reply={...}) Line 1389 C++ WebKit.dll!WebKit::WebProcessProxy::didReceiveSyncMessage(CoreIPC::Connection * connection=0x0a5b3380, CoreIPC::MessageID messageID={...}, CoreIPC::ArgumentDecoder * arguments=0x0f897c48, WTF::OwnPtr<CoreIPC::ArgumentEncoder> & reply={...}) Line 284 C++ WebKit.dll!CoreIPC::Connection::dispatchSyncMessage(CoreIPC::MessageID messageID={...}, CoreIPC::ArgumentDecoder * arguments=0x0f897c48) Line 632 + 0x25 bytes C++ WebKit.dll!CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::ArgumentDecoder> & message={...}) Line 676 C++ WebKit.dll!CoreIPC::Connection::SyncMessageState::dispatchMessages() Line 171 C++ WebKit.dll!CoreIPC::Connection::SyncMessageState::dispatchMessageAndResetDidScheduleDispatchMessagesWork() Line 183 C++ WebKit.dll!MemberFunctionWorkItem0<CoreIPC::Connection::SyncMessageState>::execute() Line 76 + 0x10 bytes C++ WebKit.dll!RunLoop::performWork() Line 63 + 0x1a bytes C++ WebKit.dll!RunLoop::wndProc(HWND__ * hWnd=0x00080502, unsigned int message=1025, unsigned int wParam=154326080, long lParam=0) Line 62 C++ WebKit.dll!RunLoop::RunLoopWndProc(HWND__ * hWnd=0x00080502, unsigned int message=1025, unsigned int wParam=154326080, long lParam=0) Line 44 + 0x18 bytes C++ user32.dll!_InternalCallWinProc@20() + 0x28 bytes And here's the backtrace of the crash: > JavaScriptCore.dll!WTF::fastMalloc(unsigned int size=4284360724) Line 3811 + 0x153 bytes C++ WebKit.dll!WTF::StringImpl::createUninitialized(unsigned int length=0, wchar_t * & data=) Line 87 + 0xa bytes C++ WebKit.dll!CoreIPC::ArgumentCoder<WTF::String>::decode(CoreIPC::ArgumentDecoder * decoder=0x7faea578, WTF::String & result={}) Line 274 C++ WebKit.dll!CoreIPC::Arguments2<unsigned __int64,WTF::String>::decode(CoreIPC::ArgumentDecoder * decoder=0x00000000, CoreIPC::Arguments2<unsigned __int64,WTF::String> & result={...}) Line 118 + 0xa bytes C++ WebKit.dll!CoreIPC::handleMessage<Messages::WebPageProxy::RunJavaScriptPrompt,WebKit::WebPageProxy,void (__thiscall WebKit::WebPageProxy::*)(unsigned __int64,WTF::String const &,WTF::String const &,WTF::String &)>(CoreIPC::ArgumentDecoder * argumentDecoder=0x00000000, CoreIPC::ArgumentEncoder * replyEncoder=, WebKit::WebPageProxy * object=, void (unsigned __int64, const WTF::String &, const WTF::String &, WTF::String &)* function=) Line 284 + 0x1b bytes C++ WebKit.dll!WebKit::WebPageProxy::didReceiveSyncWebPageProxyMessage(CoreIPC::Connection * __formal=0x039c8267, CoreIPC::MessageID messageID={...}, CoreIPC::ArgumentDecoder * arguments=0x00000000, WTF::OwnPtr<CoreIPC::ArgumentEncoder> & reply={...}) Line 552 C++ WebKit.dll!WebKit::WebPageProxy::didReceiveSyncMessage(CoreIPC::Connection * connection=0x039d27ed, CoreIPC::MessageID messageID={...}, CoreIPC::ArgumentDecoder * arguments=0x00000000, WTF::OwnPtr<CoreIPC::ArgumentEncoder> & reply={...}) Line 1389 C++ WebKit.dll!WebKit::WebProcessProxy::didReceiveSyncMessage(CoreIPC::Connection * connection=0x7fc93200, CoreIPC::MessageID messageID={...}, CoreIPC::ArgumentDecoder * arguments=0x7faea578, WTF::OwnPtr<CoreIPC::ArgumentEncoder> & reply={...}) Line 284 C++ WebKit.dll!CoreIPC::Connection::dispatchSyncMessage(CoreIPC::MessageID messageID={...}, CoreIPC::ArgumentDecoder * arguments=0x7faea578) Line 637 C++ WebKit.dll!CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::ArgumentDecoder> & message={...}) Line 676 C++ WebKit.dll!CoreIPC::Connection::SyncMessageState::dispatchMessages() Line 170 + 0xb bytes C++ WebKit.dll!CoreIPC::Connection::SyncMessageState::dispatchMessageAndResetDidScheduleDispatchMessagesWork() Line 183 C++ WebKit.dll!RunLoop::performWork() Line 64 C++ WebKit.dll!RunLoop::wndProc(HWND__ * hWnd=, unsigned int message=0, unsigned int wParam=0, long lParam=) Line 65 C++ WebKit.dll!RunLoop::RunLoopWndProc(HWND__ * hWnd=0x000904e0, unsigned int message=1025, unsigned int wParam=2146018816, long lParam=0) Line 55 C++ user32.dll!_InternalCallWinProc@20() + 0x28 bytes user32.dll!_UserCallWinProcCheckWow@32() + 0xb7 bytes user32.dll!_DispatchMessageWorker@8() + 0xdc bytes user32.dll!_DispatchMessageW@4() + 0xf bytes You can see that we're trying to malloc 4.3GB of memory.
There are two bugs here: 1) WebProcessProxy::didReceiveSyncMessage is missing an early return. It was removed in r86812. 2) ArgumentDecoder::alignBufferPosition and ArgumentDecoder::bufferIsLargeEnoughToContain are vulnerable to underflow. I'll fix them both!
A little more detail: The web process is sending the UI process a Messages::DownloadProxy::DecideDestinationWithSuggestedFilename message. The UI process is handling this message, but then also passing the message along to a WebPageProxy. This is when the assertion fires in Debug builds. In Release builds, WebPageProxy thinks the message is a Messages::WebPageProxy::RunJavaScriptPrompt. When it starts decoding the arguments for that message (a uint64_t and two Strings), we've already reached the end of the ArgumentDecoder's buffer (thanks to decoding the DecideDestinationWithSuggestedFilename earlier). In the crashing case, ArgumentDecoder::m_bufferPos and m_bufferEnd are equal when ArgumentDecoder::decodeUInt64 is called. decodeUInt64 calls through to ArgumentDecoder::alignBufferPosition, which contains this code: uint8_t* buffer = roundUpToAlignment(m_bufferPos, alignment); if (static_cast<size_t>(m_bufferEnd - buffer) < size) { // We've walked off the end of this buffer. markInvalid(); return false; } Since m_bufferPos and m_bufferEnd are equal, buffer will be greater than m_bufferEnd, resulting in underflow. We won't detect that we've walked off the buffer, and will continue as if nothing were wrong. We're now reading off the end of the buffer, which is what eventually gets us that 4.3GB size for one of the String arguments.
<rdar://problem/9471680>
(In reply to comment #2) > uint8_t* buffer = roundUpToAlignment(m_bufferPos, alignment); > if (static_cast<size_t>(m_bufferEnd - buffer) < size) { > // We've walked off the end of this buffer. > markInvalid(); > return false; > } This was fixed elsewhere by rounding the buffer end up to the alignment as well.
(In reply to comment #4) > This was fixed elsewhere by rounding the buffer end up to the alignment as well. That exact solution won’t work here, but it should be easy to fix with something along those lines here.
Created attachment 94121 [details] Don't try to process DownloadProxy messages twice (and robustify code that runs if we do)
Comment on attachment 94121 [details] Don't try to process DownloadProxy messages twice (and robustify code that runs if we do) View in context: https://bugs.webkit.org/attachment.cgi?id=94121&action=review > Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp:92 > uint8_t* buffer = roundUpToAlignment(m_bufferPos, alignment); > - if (static_cast<size_t>(m_bufferEnd - buffer) < size) { > + if (!alignedBufferIsLargeEnoughToContain(buffer, m_bufferEnd, size)) { Seems to me a simpler way to write this is this: if (!(alignedPosition >= m_bufferEnd || static_cast<size_t>(m_bufferEnd - alignedPosition) < size)) I am baffled by the name “buffer” for the local variable for the aligned position. > Source/WebKit2/UIProcess/WebProcessProxy.cpp:274 > if (messageID.is<CoreIPC::MessageClassWebContext>() || messageID.is<CoreIPC::MessageClassWebContextLegacy>() > || messageID.is<CoreIPC::MessageClassDownloadProxy>() || messageID.is<CoreIPC::MessageClassWebIconDatabase>()) { > m_context->didReceiveSyncMessage(connection, messageID, arguments, reply); > + return; > } Oof! Looks like I just broke this! Thanks for the fix!!!
Comment on attachment 94121 [details] Don't try to process DownloadProxy messages twice (and robustify code that runs if we do) View in context: https://bugs.webkit.org/attachment.cgi?id=94121&action=review >> Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp:92 >> + if (!alignedBufferIsLargeEnoughToContain(buffer, m_bufferEnd, size)) { > > Seems to me a simpler way to write this is this: > > if (!(alignedPosition >= m_bufferEnd || static_cast<size_t>(m_bufferEnd - alignedPosition) < size)) > > I am baffled by the name “buffer” for the local variable for the aligned position. I changed alignedBufferIsLargeEnoughToContain to this: return bufferEnd >= alignedPosition && static_cast<size_t>(bufferEnd - alignedPosition) >= size; I also renamed "buffer" to "alignedPosition". >> Source/WebKit2/UIProcess/WebProcessProxy.cpp:274 >> } > > Oof! Looks like I just broke this! Thanks for the fix!!! No problem. :-) Thanks for reviewing!
Committed r86945: <http://trac.webkit.org/changeset/86945>