Sending big in-memory blobs to WebSocket makes FF to crash or fail

RESOLVED FIXED in Firefox 58

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 wontfix, firefox57+ wontfix, firefox58 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(9 attachments, 5 obsolete attachments)

5.79 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.06 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.10 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1010 bytes, patch
smaug
: review+
Details | Diff | Splinter Review
1.04 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.62 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.42 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.82 KB, patch
smaug
: review+
Details | Diff | Splinter Review
13.77 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Posted patch part 2 - WebSocket test (obsolete) — Splinter Review
I'm doing audit of the use of NS_ReadInputStreamToString.

We use it in WebSocketChannel.cpp here: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#1058

NS_ReadInputStreamToString fails if the stream works only async as IPCBlobInputStream. Just because this is running on the parent process, the only way to trigger this issue is to send a big blob (>1mb) generated on the content side.

I wrote a test, in attach.

Possible solution:
Get rid of NS_ReadInputStreamToString in WebSocketChannel, block the Socket thread, read the async inputStream on a I/O thread, continue with the current code.

Here the crash log:

GECKO(7803) | [Parent 7803, Socket Thread] ###!!! ASSERTION: Stream length != blob length!: 'bytes == mLength', file /home/baku/Sources/m/bbb/src/netwerk/protocol/websocket/WebSocketChannel.cpp, line 1054
GECKO(7803) | #01: mozilla::net::OutboundMessage::ConvertStreamToString() (/home/baku/Sources/m/bbb/src/netwerk/protocol/websocket/WebSocketChannel.cpp:1054 (discriminator 1))
GECKO(7803) | #02: mozilla::net::WebSocketChannel::PrimeNewOutgoingMessage() (/home/baku/Sources/m/bbb/src/netwerk/protocol/websocket/WebSocketChannel.cpp:2149)
GECKO(7803) | #03: mozilla::net::WebSocketChannel::OnOutputStreamReady(nsIAsyncOutputStream*) (/home/baku/Sources/m/bbb/src/netwerk/protocol/websocket/WebSocketChannel.cpp:4019)
GECKO(7803) | #04: mozilla::net::WebSocketChannel::EnqueueOutgoingMessage(nsDeque&, mozilla::net::OutboundMessage*) (/home/baku/Sources/m/bbb/src/netwerk/protocol/websocket/WebSocketChannel.cpp:2014)
GECKO(7803) | #05: mozilla::net::OutboundEnqueuer::Run() (/home/baku/Sources/m/bbb/src/netwerk/protocol/websocket/WebSocketChannel.cpp:1142)
GECKO(7803) | #06: nsThread::ProcessNextEvent(bool, bool*) (/home/baku/Sources/m/bbb/src/xpcom/threads/nsThread.cpp:1033)
GECKO(7803) | #07: NS_ProcessNextEvent(nsIThread*, bool) (/home/baku/Sources/m/bbb/src/xpcom/threads/nsThreadUtils.cpp:513)
GECKO(7803) | #08: mozilla::net::nsSocketTransportService::Run() (/home/baku/Sources/m/bbb/src/netwerk/base/nsSocketTransportService2.cpp:991)
GECKO(7803) | #09: nsThread::ProcessNextEvent(bool, bool*) (/home/baku/Sources/m/bbb/src/xpcom/threads/nsThread.cpp:1033)
GECKO(7803) | #10: NS_ProcessNextEvent(nsIThread*, bool) (/home/baku/Sources/m/bbb/src/xpcom/threads/nsThreadUtils.cpp:513)
GECKO(7803) | #11: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (/home/baku/Sources/m/bbb/src/ipc/glue/MessagePump.cpp:334)
GECKO(7803) | #12: MessageLoop::RunInternal() (/home/baku/Sources/m/bbb/src/ipc/chromium/src/base/message_loop.cc:327)
GECKO(7803) | #13: MessageLoop::RunHandler() (/home/baku/Sources/m/bbb/src/ipc/chromium/src/base/message_loop.cc:320)
GECKO(7803) | #14: MessageLoop::Run() (/home/baku/Sources/m/bbb/src/ipc/chromium/src/base/message_loop.cc:298)
GECKO(7803) | #15: nsThread::ThreadFunc(void*) (/home/baku/Sources/m/bbb/src/xpcom/threads/nsThread.cpp:427)
GECKO(7803) | #16: _pt_root (/home/baku/Sources/m/bbb/src/nsprpub/pr/src/pthreads/ptthread.c:219)
GECKO(7803) | #17: start_thread (/build/glibc-bfm8X4/glibc-2.23/nptl/pthread_create.c:333)
GECKO(7803) | #18: __clone (/build/glibc-bfm8X4/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:111)
1mb isn't really big ;)
Assignee: nobody → michal.novotny
Priority: -- → P1
See Also: → 1394564
Whiteboard: [necko-triaged]
Make NS_ReadInputStreamToString able to work with nsIAsyncInputStream blocking the current thread.
Assignee: michal.novotny → amarchesini
Attachment #8926033 - Flags: review?(bugs)
Attachment #8926034 - Flags: review?(bugs)
Attachment #8925828 - Attachment description: test → part 2 - WebSocket test
Attachment #8925828 - Flags: review?(bugs)
Attachment #8926036 - Flags: review?(bugs)
Attachment #8926039 - Flags: review?(bugs)
Attachment #8926040 - Flags: review?(bugs)
Posted patch part 7 - HTMLCanvasElement (obsolete) — Splinter Review
Attachment #8926041 - Flags: review?(bugs)
Attachment #8926042 - Flags: review?(bugs)
Is this a regression? If so, what regressed this?
Since if this is a regression, do we need the fix in FF57?
1MB blob is tiny.
smaug, as you probably understood I have written a fix for any use of NS_ReadInputStreamToString and NS_ReadInputStreamToBuffer. I'm OK to file separate bug, but probably it's better to rename this bug and fix all these issues here.
I'll start reviewing only once the regressing bugs are marked in the blocks field and relevant tracking flags sets, so that I know how urgent this is.
Strictly related to WebSocket, this regression is generated by IPCBlob or any previous async approaches such as PMemoryBlobStream and so on. ESR52 doesn't contain any of those patches.

Note that we don't crash in non-debug builds, but simply we don't send data to the WebSocket server. This happens because ::Available() returns 0 and NS_ReadInputStreamToString() returns an empty string.

For any other use of NS_ReadInputStreamTostring() is hard to say if we can end up using them with an async inputStream.
If this is the cause for https://bugzilla.mozilla.org/show_bug.cgi?id=1394564, this is rather bad.
And the patches are scary, yikes.
Attachment #8926042 - Flags: review?(bugs) → review+
Comment on attachment 8926041 [details] [diff] [review]
part 7 - HTMLCanvasElement

You drop the size check and then imgSize is just casted to uint32_t.
Doesn't look quite safe.
Attachment #8926041 - Flags: review?(bugs) → review-
Attachment #8926040 - Flags: review?(bugs) → review+
Comment on attachment 8926039 [details] [diff] [review]
part 5 - nsFrameMessageManager

Doesn't compiler complain about passing uint64_t to ScriptLoader::ConvertToUTF16?
Attachment #8926039 - Flags: review?(bugs) → review-
Attachment #8926036 - Flags: review?(bugs) → review+
Comment on attachment 8926035 [details] [diff] [review]
part 3 - WebSocketChannel should not use ::Available()

Why you remove the assertion? We should be able assert there.
Can't review this.
Attachment #8926035 - Flags: review?(bugs)
Comment on attachment 8925828 [details] [diff] [review]
part 2 - WebSocket test

Could you rather just cache the array you send and compare to that?
That would use less memory when running the test
Attachment #8925828 - Flags: review?(bugs) → review+
Comment on attachment 8926034 [details] [diff] [review]
part 1 - gtest

>+// Here we test the reading an async big stream without passing the size
>+TEST(TestReadStreamToString, AsyncStreamUnknownBigSize) {
>+  nsCString buffer;
>+
>+  buffer.SetLength(4096 * 2);
This is not big. This is very small. Test at least > 1MB streams
Attachment #8926034 - Flags: review?(bugs) → review+
Comment on attachment 8926033 [details] [diff] [review]
part 0 - NS_ReadInputStreamToString and async inputStreams

Hmm, this does what? Blocks the current thread, per your comment 2?
Sounds horrible, but I guess that is what the API is and has been.

But this needs some comment what this is doing. How does BufferWriter work?
Smaug pinged me about this one. I'd like to track this as a blocker until we understand the impact, severity better.
baku, can you clarify the security aspect of this bug. What kinds of crashes may happen when things go badly?
Flags: needinfo?(amarchesini)
Group: core-security → network-core-security
> Why you remove the assertion? We should be able assert there.
> Can't review this.

This is actually what this bug is about. You cannot use ::Available() to know the real size of the stream, right?
The stream is async, and it returns 0.
Flags: needinfo?(amarchesini) → needinfo?(bugs)
> >+  buffer.SetLength(4096 * 2);
> This is not big. This is very small. Test at least > 1MB streams


gtests are not testing what we are sending cross processes. This is why I also added the websocket test.
No, it doesn't not complain. But let's put back the std::min.
Attachment #8926039 - Attachment is obsolete: true
The cast was actually wrong. CreateMemoryFile wants a uint64_t
Attachment #8926041 - Attachment is obsolete: true
(In reply to Andrea Marchesini [:baku] from comment #25)
> > Why you remove the assertion? We should be able assert there.
> > Can't review this.
> 
> This is actually what this bug is about. You cannot use ::Available() to
> know the real size of the stream, right?
> The stream is async, and it returns 0.
Of course.
Flags: needinfo?(bugs)
Reasking:

baku, can you clarify the security aspect of this bug. What kinds of crashes may happen when things go badly?
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #26)
> > >+  buffer.SetLength(4096 * 2);
> > This is not big. This is very small. Test at least > 1MB streams
> 
> 
> gtests are not testing what we are sending cross processes. This is why I
> also added the websocket test.
But 4096 * 2 is no way large, so it isn't clear what the test is testing.
Some larger one might still make sense even without ipc.
Comment on attachment 8925828 [details] [diff] [review]
part 2 - WebSocket test

Hey, I think we really should test this on workers too.
Comment on attachment 8926163 [details] [diff] [review]
part 7 - HTMLCanvasElement

I assume this needs a new review. The change makes sense.
Attachment #8926163 - Flags: review+
Comment on attachment 8926161 [details] [diff] [review]
part 5 - nsFrameMessageManager

ok, the old code was a bit silly, and so is the new one, but end result should be the same. And framescripts shouldn't be 4GB+
Attachment #8926161 - Flags: review+
Attachment #8926035 - Flags: review+
Comment on attachment 8926040 [details] [diff] [review]
part 6 - ImageEncoder

Does this mean CanvasRenderingContextHelper::ToBlob may not work currently if the data size is large, or are we dealing with non-async streams here?
Can you please test?
Comment on attachment 8926033 [details] [diff] [review]
part 0 - NS_ReadInputStreamToString and async inputStreams

>+        nsCOMPtr<nsIEventTarget> target =
>+            do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
>+        if (!target) {
>+            return NS_ERROR_FAILURE;
>+        }
>+
>+        mTaskQueue = new TaskQueue(target.forget());
>+
>+        MonitorAutoLock lock(mMonitor);
>+
>+        nsCOMPtr<nsIRunnable> runnable = this;
>+        mTaskQueue->Dispatch(runnable.forget());
>+
>+        lock.Wait();
Dispatch may fail, especially during shutdown. So I don't think we should wait here in such case.
(Thanks to michal for noticing this)

>+            nsCOMPtr<nsIRunnable> runnable = this;
>+            mTaskQueue->Dispatch(runnable.forget());
Same here, but needs to be handled differently.
Don't you need to notify in such case so that we don't block the calling thread forever?

>+    MaybeExpandBufferSize()
>+    {
>+        MOZ_ASSERT(mCount == -1);
>+
>+        if (mBufferSize >= mWrittenData + BUFFER_SIZE) {
>+            // The buffer is big enough.
>+            return true;
>+        }
>+
>+        // Start at 1 or we'll loop forever.
>+        CheckedUint32 bufferSize =
>+            std::max<uint32_t>(static_cast<uint32_t>(mWrittenData), 1);
>+        while (bufferSize.isValid() &&
>+               bufferSize.value() < mWrittenData + BUFFER_SIZE) {
>+            bufferSize *= 2;
>+        }
Why you start from 1 and not from BUFFER_SIZE?



>+NS_ReadInputStreamToBuffer(nsIInputStream* aInputStream,
>+                           void** aDest,
>+                           int64_t aCount,
>+                           uint64_t* aWritten)
> {
>-    nsresult rv;
>+    MOZ_ASSERT(aInputStream);
>+    MOZ_ASSERT(aCount >= -1);
>+    MOZ_ASSERT_IF(*aDest, aCount > 0);
>+
>+    uint64_t dummyWritten;
>+    if (!aWritten) {
>+        aWritten = &dummyWritten;
>+    }
>+
>+    if (aCount == 0) {
>+        *aDest = nullptr;
Why *aDest is set to null. I don't see this behavior currently


>-NS_ReadInputStreamToString(nsIInputStream *aInputStream,
>-                           nsACString &aDest,
>-                           uint32_t aCount)
>-{
>-    if (!aDest.SetLength(aCount, mozilla::fallible))
>-        return NS_ERROR_OUT_OF_MEMORY;
>-    void* dest = aDest.BeginWriting();
>-    return NS_ReadInputStreamToBuffer(aInputStream, &dest, aCount);
>+
>+    void* dest = nullptr;
>+    nsresult rv = NS_ReadInputStreamToBuffer(aInputStream, &dest, aCount,
>+                                             aWritten);
>+    NS_ENSURE_SUCCESS(rv, rv);
Can we please assert before NS_ENSURE_SUCCESS that if rv is failure value, dest is null.


>+/**
>+ * This function reads an inputStream and stores its content into a buffer. In
>+ * general, you should avoid using this function because, it blocks the current
>+ * thread until the operation is done.
>+ * If the inputStream is async, the reading happens on an I/O thread.
>+ *
>+ * @param aInputStream the inputStream.
>+ * @param aDest the destination buffer. if aDest is null, it will be allocated
>+ *              with the size of the written data. if aDest is not null, aCount
>+ *              must greater than 0.
I guess aDest should never be null. *aDest may be null or not.
Attachment #8926033 - Flags: review?(bugs) → review-
Attachment #8925828 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Comment on attachment 8926382 [details] [diff] [review]
part 2 - WebSocket test

Ok, the previous patch was using some other .py.

Could you call the .py
file_websocket_bigBlob_wsh.py
Attachment #8926382 - Flags: review+
> Dispatch may fail, especially during shutdown. So I don't think we should
> wait here in such case.
> (Thanks to michal for noticing this)

Wrong. TaskQueue::Dispatch returns void.
Attachment #8926033 - Attachment is obsolete: true
Attachment #8926387 - Flags: review?(bugs)
(In reply to Andrea Marchesini [:baku] from comment #39)
> > Dispatch may fail, especially during shutdown. So I don't think we should
> > wait here in such case.
> > (Thanks to michal for noticing this)
> 
> Wrong. TaskQueue::Dispatch returns void.
Uh, TaskQueue::Dispatch is bizarre. If we can't know that it fails, we have a deadlock, no?
Attachment #8926387 - Attachment is obsolete: true
Attachment #8926387 - Flags: review?(bugs)
Attachment #8926401 - Flags: review?(bugs)
Attachment #8926401 - Flags: review?(bugs) → review+
I don't think this is a FF57 blocker: without this set of patches, FF will not crash nor hangs, but it sends a 0-byte blob to the WebSocket server. This is wrong, but definitely it's not enough to be a FF57 blocker.
Comment on attachment 8926382 [details] [diff] [review]
part 2 - WebSocket test

A worker test would be still really nice.
Thanks for your assessment Baku. I will untag this as a 57 blocker. Leaving it as 57 tracked for a bit longer in case this becomes a bigger issue post 57 launch.
This code is ready to land. I don't think this is a security bug. Initially I marked it to be a security bug because I didn't know the security implications of NS_ReadInputStreamToString in our code base when working with nsIAsyncInputStream.
Now, it's clear to me that we don't crash, we don't hang, and this code is safe to land.
I cannot remove the security flag. NI dveditz. Can I directly land this code? Can you please remove the security flag? I'm not part of the networking security team (should I maybe be part of it?)
Flags: needinfo?(dveditz)
Group: network-core-security
Flags: needinfo?(dveditz)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/057c4132d248
part 0 - NS_ReadInputStreamToString must support nsIAsyncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cf6867bed42
part 1 - gTest for NS_ReadInputStreamToString, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/90939e355e8d
part 2 - mochitest for NS_ReadInputStreamToString in WebSocket, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/42a4f661f2d3
part 3 - WebSocketChannel cannot use ::Available() to know the size of a nsIAsyncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/872fbd197346
part 4 - Fix the use of NS_ReadInputStreamToString in DataChannel, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d2a4a48001
part 5 - Fix the use of NS_ReadInputStreamToString in nsFrameMessageManager, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4bd76733880
part 6 - Fix the use of NS_ReadInputStreamToString in ImageEncoder, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/161a4957d9b5
part 7 - Fix the use of NS_ReadInputStreamToString in HTMLCanvasElement, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b9b464a9eb
part 8 - Fix the use of NS_ReadInputStreamToString in DataTransferItem, r=smaug
Duplicate of this bug: 1412114
Wont fix for Firefox 56 and 57 based on comment 43. Keeping the tracking flag based on comment 45.
Depends on: 1417448
Depends on: 1431646
You need to log in before you can comment on or make changes to this bug.