1027364 - Sending multiple headers crashes FFx

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Ben Brittain (:bbrittain)]

Attachment: stacktrace

Comment 1

[u408661]

This was triggered by calling "response.writeHead" twice in a row from a node-http2 server talking to firefox (thus sending 2 sets of headers, each with a different status code in this case). The stack trace appears to be worthless, because there's likely memory corruption involved.

Comment 2

[Patrick McManus [:mcmanus]]

nick - does that mean you have a repro?

given the pref'd off status of http2 this probably doesn't need to be security-sensitive once we are sure its http2 limited.

Comment 3

[Benjamin Smedberg]

That's a crash stack? That looks like the main thread simply waiting for something to do.

Perhaps the crash was on a different thread?

Do you have a server (node) testcase you can attach?

Comment 4

[Ben Brittain (:bbrittain)]

The crash probably occurs on a different thread, but FFx tells me to attach GDB to that thread.

I've attached a server that will crash it.

Comment 5

[Ben Brittain (:bbrittain)]

Attachment: crash.js

Comment 6

[Patrick McManus [:mcmanus]]

(In reply to Ben Brittain (:bbrittain) from comment #4)
> The crash probably occurs on a different thread, but FFx tells me to attach
> GDB to that thread.
> 
> I've attached a server that will crash it.

:) thanks for the testcase - I haven't tried it yet but will if nick doesn't beat me to it.

fwiw - you generally need to go to info threads and find the right thread to switch to in gdb.. given the nature of the crash it will probably be labeled "socket thread" or "necko thread".

Comment 7

[u408661]

I'm making a local draft12 build right now, and have a test server just waiting to crash my build. Will report back with (hopefully) a better stack once that's done.

Comment 8

[u408661]

Attachment: stack.txt

OK, here's a better stack trace. This definitely appears to be HTTP/2 specific - we're hitting a MOZ_ASSERT in Http2Session.cpp.

I need to dig in more, but I suspect the double-headers is confusing our state machine, leading to the assert being hit. Probably the best course of action is to detect such a condition and send a GOAWAY/PROTOCOL_ERROR because, honestly, WTF? :)

Comment 9

[Patrick McManus [:mcmanus]]

no doubt an nspr log is necssary.

iirc off the top of my head multiple headers for the same stream has some kind of meaning wherein we can ignore the subsequent ones - but it isn't a sending error. basically the http/2 equivalent of trailers or per chunk headers.

I'd have to double check that.

Comment 10

[u408661]

Attachment: nspr.log

And here's an nspr log. I haven't dug into it yet, but everyone loves posterity, right?

Comment 11

[Martin Thomson [:mt:]]

(In reply to Patrick McManus [:mcmanus] from comment #9)
> iirc off the top of my head multiple headers for the same stream has some
> kind of meaning wherein we can ignore the subsequent ones - but it isn't a
> sending error. basically the http/2 equivalent of trailers or per chunk
> headers.

Don't bother; it's permitted and the headers don't carry semantics.  So ignore them.

Now, that's a part of the spec I don't like, but that's how it is.

Comment 12

[Patrick McManus [:mcmanus]]

section 8.1.. basically trailers for http. in non http scenarios they are allowed pretty much anywhere.

The code already ignores them at the stream level, but it forgets to reset the parsing state in the session to expect the next frame.

does this totally untested and uncompiled patch help?

diff --git a/netwerk/protocol/http/Http2Session.cpp b/netwerk/protocol/http/Http2Session.cpp
index a98c86f..720a14c 100644
--- a/netwerk/protocol/http/Http2Session.cpp
+++ b/netwerk/protocol/http/Http2Session.cpp
@@ -1151,22 +1151,25 @@ Http2Session::RecvHeaders(Http2Session *self)
 }
 
 // ResponseHeadersComplete() returns NS_ERROR_ILLEGAL_VALUE when the stream
 // should be reset with a PROTOCOL_ERROR, NS_OK when the response headers were
 // fine, and any other error is fatal to the session.
 nsresult
 Http2Session::ResponseHeadersComplete()
 {
-  LOG3(("Http2Session::ResponseHeadersComplete %p for 0x%X fin=%d",
-        this, mInputFrameDataStream->StreamID(), mInputFrameFinal));
+  LOG3(("Http2Session::ResponseHeadersComplete %p for 0x%X fin=%d ahr=%d",
+        this, mInputFrameDataStream->StreamID(),
+        mInputFrameFinal, mInputFrameDataStream->AllHeadersReceived()));
 
   // only do this once, afterwards ignore trailers
-  if (mInputFrameDataStream->AllHeadersReceived())
+  if (mInputFrameDataStream->AllHeadersReceived()) {
+    ResetDownstreamState();
     return NS_OK;
+  }
   mInputFrameDataStream->SetAllHeadersReceived();
 
   // The stream needs to see flattened http headers
   // Uncompressed http/2 format headers currently live in
   // Http2Stream::mDecompressBuffer - convert that to HTTP format in
   // mFlatHTTPResponseHeaders via ConvertHeaders()
 
   nsresult rv;

Comment 13

[Patrick McManus [:mcmanus]]

its a good testcase.

Comment 14

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #12)
> does this totally untested and uncompiled patch help?

Why yes, yes it does.

Comment 15

[Patrick McManus [:mcmanus]]

Attachment: multiple http/2 header frames on same stream

Comment 16

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=d8060afe9510

Comment 17

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2e0996e76d5

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d2e0996e76d5