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

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect, P1)

Description

[Andrea Marchesini [:baku]]

Attachment: part 2 - WebSocket test

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)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

1mb isn't really big ;)

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 0 - NS_ReadInputStreamToString and async inputStreams

Make NS_ReadInputStreamToString able to work with nsIAsyncInputStream blocking the current thread.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - gtest

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 3 - WebSocketChannel should not use ::Available()

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 4 - DataChannel

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 5 - nsFrameMessageManager

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 6 - ImageEncoder

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 7 - HTMLCanvasElement

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 8 - DataTransferItem

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Is this a regression? If so, what regressed this?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Since if this is a regression, do we need the fix in FF57?
1MB blob is tiny.

Comment 12

[Andrea Marchesini [:baku]]

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.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 14

[Andrea Marchesini [:baku]]

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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

If this is the cause for https://bugzilla.mozilla.org/show_bug.cgi?id=1394564, this is rather bad.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

And the patches are scary, yikes.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8926039 [details] [diff] [review]
part 5 - nsFrameMessageManager

Doesn't compiler complain about passing uint64_t to ScriptLoader::ConvertToUTF16?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Smaug pinged me about this one. I'd like to track this as a blocker until we understand the impact, severity better.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

baku, can you clarify the security aspect of this bug. What kinds of crashes may happen when things go badly?

Comment 25

[Andrea Marchesini [:baku]]

> 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.

Comment 26

[Andrea Marchesini [:baku]]

> >+  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.

Comment 27

[Andrea Marchesini [:baku]]

Attachment: part 5 - nsFrameMessageManager

No, it doesn't not complain. But let's put back the std::min.

Comment 28

[Andrea Marchesini [:baku]]

Attachment: part 7 - HTMLCanvasElement

The cast was actually wrong. CreateMemoryFile wants a uint64_t

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Reasking:

baku, can you clarify the security aspect of this bug. What kinds of crashes may happen when things go badly?

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

(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 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8925828 [details] [diff] [review]
part 2 - WebSocket test

Hey, I think we really should test this on workers too.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8926163 [details] [diff] [review]
part 7 - HTMLCanvasElement

I assume this needs a new review. The change makes sense.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

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+

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

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 36

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 37

[Andrea Marchesini [:baku]]

Attachment: part 2 - WebSocket test

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 39

[Andrea Marchesini [:baku]]

> 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.

Comment 40

[Andrea Marchesini [:baku]]

Attachment: part 0 - NS_ReadInputStreamToString and async inputStreams

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

(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?

Comment 42

[Andrea Marchesini [:baku]]

Attachment: part 0 - NS_ReadInputStreamToString and async inputStreams

Comment 43

[Andrea Marchesini [:baku]]

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 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8926382 [details] [diff] [review]
part 2 - WebSocket test

A worker test would be still really nice.

Comment 45

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 46

[Andrea Marchesini [:baku]]

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?)

Comment 47

[Pulsebot]

Pushed by [e-mail]:
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

Comment 49

[u279076]

Wont fix for Firefox 56 and 57 based on comment 43. Keeping the tracking flag based on comment 45.

Comment 50

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/057c4132d248
https://hg.mozilla.org/mozilla-central/rev/7cf6867bed42
https://hg.mozilla.org/mozilla-central/rev/90939e355e8d
https://hg.mozilla.org/mozilla-central/rev/42a4f661f2d3
https://hg.mozilla.org/mozilla-central/rev/872fbd197346
https://hg.mozilla.org/mozilla-central/rev/b9d2a4a48001
https://hg.mozilla.org/mozilla-central/rev/f4bd76733880
https://hg.mozilla.org/mozilla-central/rev/161a4957d9b5
https://hg.mozilla.org/mozilla-central/rev/79b9b464a9eb