Summary: | REGRESSION (r86812): Crash (preceded by assertion) in fastMalloc when downloading a file | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> | ||||
Component: | Platform | Assignee: | Adam Roben (:aroben) <aroben> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | andersca, darin | ||||
Priority: | P2 | Keywords: | InRadar, Regression | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | Windows XP | ||||||
Attachments: |
|
Description
Adam Roben (:aroben)
2011-05-19 14:08:19 PDT
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. (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> |