Closed
Bug 1322373
Opened 7 years ago
Closed 7 years ago
TLS 1.3 early-data for http/2
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: u408661, Assigned: mcmanus)
References
Details
(Whiteboard: [necko-active])
Attachments
(1 file, 1 obsolete file)
42.11 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
We need to support early-data when talking http/2 to servers, not just http/1.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
Can we send the early data for more than one transaction? It should be possible. Can we also rewind h2 transaction similar as we do with h1, in that case we do not need to keep the buffer and we can call RealignOutputQueue. (this is not that important, it will work without it as well).
(In reply to Dragana Damjanovic [:dragana] from comment #2) > Can we send the early data for more than one transaction? It should be > possible. The chances of this even being possible on a new connection with h2 are pretty slim - most of the time, this will be the root pageload, so we won't have other resources to fetch yet. > Can we also rewind h2 transaction similar as we do with h1, in that case we > do not need to keep the buffer and we can call RealignOutputQueue. (this is > not that important, it will work without it as well). Sure, we could, but why bother? Not that the optmization of keeping the buffer matters all that much, but there's no particular reason to *not* keep the data around (and it makes the "early data rejected but still h2" case much easier).
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #3) > (In reply to Dragana Damjanovic [:dragana] from comment #2) > > Can we send the early data for more than one transaction? It should be > > possible. > > The chances of this even being possible on a new connection with h2 are > pretty slim - most of the time, this will be the root pageload, so we won't > have other resources to fetch yet. not necessarily true - esp where conn is to a cdn carrying the static resources in a different origin. this is an impt feature. maybe it can be added on.
(In reply to Patrick McManus [:mcmanus] from comment #4) > not necessarily true - esp where conn is to a cdn carrying the static > resources in a different origin. this is an impt feature. maybe it can be > added on. Good point. Thinking about it, it should be pretty easy to add - just need to keep an array of pointers instead of the single pointer. Then just return the first one in the array to the connection, since that should be the first one that got dispatched to us (or near enough) and NET_RESET the rest.
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8836178 [details] Bug 1322373 - TLS 1.3 early-data for HTTP/2. https://reviewboard.mozilla.org/r/111614/#review113862 I think we need some changes, but they aren't big changes. I also would like dragana to take a look at nsHttpConnection around the existing early data bits. but I'm comfortable with doing the h2 parts. ::: netwerk/protocol/http/ASpdySession.h:32 (Diff revision 1) > virtual bool RoomForMoreStreams() = 0; > virtual PRIntervalTime IdleTime() = 0; > virtual uint32_t ReadTimeoutTick(PRIntervalTime now) = 0; > virtual void DontReuse() = 0; > + virtual uint32_t SpdyVersion() = 0; > + virtual nsAHttpTransaction *Take0RTTTransaction() = 0; TakeFOO normally means that the callee is giving up its reference so the caller can have it. It looks like this is just an accessor so it should either be called GetFOO or just FOO.. (or maybe it should be returning some already_addreffed var and dropping its own ref?) ::: netwerk/protocol/http/Http2Session.h:503 (Diff revision 1) > bool mGoAwayOnPush; > > bool mUseH2Deps; > > + bool mAttemptingEarlyData; > + // The stream that we are getting 0RTT data from. is there a reason to be sure this won't be a uaf pointer? its presence in the transactionHash is what keeps it alive. I think you need to store it by ID and look it up in mStreamIDHash at use time (perhaps with a verifystream() thrown in). I wouldn't try and keep it alive with a reference. perhaps a cancel from gecko, or a rst from the peer could make this happen. ::: netwerk/protocol/http/Http2Session.cpp:546 (Diff revision 1) > > // Dont worry about errors on write, we will pick this up as a read error too > if (NS_FAILED(rv)) > return; > > - if (countRead == avail) { > + if (countRead == avail && !mAttemptingEarlyData) { we can remove a footgun here by moving up the sent += countRead line to be right after the NS_FAILED check and then just do if (attemptingEarlyData) {return;} ::: netwerk/protocol/http/Http2Session.cpp:2378 (Diff revision 1) > this)); > FlushOutputQueue(); > SetWriteCallbacks(); > + if (mAttemptingEarlyData) { > + // We can still try to send our preamble as early-data > + *countRead = mOutputQueueUsed - mOutputQueueSent; If you're going to return a countRead, I think you should return OK. ::: netwerk/protocol/http/Http2Session.cpp:2395 (Diff revision 1) > + // We can still send our preamble > + *countRead = mOutputQueueUsed - mOutputQueueSent; > + return NS_OK; > + } > + > + m0RTTStream = stream; what if m0RTTStream is not null? this function is called in a loop looking for things on the writeQ.. that's how you would get multiple transactons when we're ready to deal with that.. ::: netwerk/protocol/http/Http2Session.cpp:2971 (Diff revision 1) > > nsresult > +Http2Session::Finish0RTT(bool aRestart, bool aAlpnChanged) > +{ > + MOZ_ASSERT(mAttemptingEarlyData); > + LOG Finish0Rtt(arestart, aalpnchanged) ::: netwerk/protocol/http/Http2Session.cpp:2972 (Diff revision 1) > nsresult > +Http2Session::Finish0RTT(bool aRestart, bool aAlpnChanged) > +{ > + MOZ_ASSERT(mAttemptingEarlyData); > + > + if (m0RTTStream) { see comment in .h file about referencing this by id instead of raw pointer ::: netwerk/protocol/http/Http2Session.cpp:2978 (Diff revision 1) > + // We use aAlpnChanged here, because if the ALPN selection hasn't changed, > + // then we have data we can just use and all we need to do is rewind the > + // session's internal buffer. If the ALPN selection changed, though, then > + // we need to do a bit more work to get all our transactions back into a > + // place where they can do http/1 > + m0RTTStream->Finish0RTT(aAlpnChanged, aAlpnChanged); I think you mean (aRestart, aAlpnChanged) ? ::: netwerk/protocol/http/Http2Session.cpp:2987 (Diff revision 1) > + // 0RTT failed > + if (aAlpnChanged) { > + // This is a slightly more involved case - we need to get all our streams/ > + // transactions back in the queue so they can restart as http/1 > + > + // In order for the connection to be reused, we need to reset the this path is pretty scary and I think we can simplify it if we don't care about keeping the connection open - and I don't as the change from h2 to h1 should not be a normal thing, as long as we recover with a new conn its ok. If we are ok with that - won't close()->shutdown() just do the right thing? this is pretty much going to be required when there is more than 1 transaction in early data anyhow. otoh, you might be able to save the optimization by just monkeying with the reuse flag (setting it back to true) on the conn after Close() has returned below.. and I think you could set the connection in the trans to null (but I'm not 100% sure) and the cmgr will just find one according to its normal precedence rules. its the cleanup up of streams here that makes me really nervous. that's been a source of bugs before and its best to leave it in the existing subroutines where possible. ::: netwerk/protocol/http/Http2Session.cpp:2995 (Diff revision 1) > + // the TCP handshake all over again. Later on, the connection will reclaim > + // this transaction from us and start it appropriately. > + if (m0RTTStream) { > + m0RTTTransaction = m0RTTStream->Transaction(); > + m0RTTTransaction->SetConnection(mConnection); > + mStreamTransactionHash.Remove(m0RTTTransaction); so this leaves a bunch of dangling uaf raw pointers around.. in mStreamIDHash, and the various nsDeque's (like readyForWrite) ::: netwerk/protocol/http/Http2Session.cpp:3004 (Diff revision 1) > + mGoAwayID = 0; > + mCleanShutdown = true; > + > + // Close takes care of the rest of our work for us. The reason code here > + // doesn't matter, as we aren't actually going to send a GOAWAY frame. > + Close(NS_OK); make the reason NET_ERROR_RESET just for the log. its closer to the truth. ::: netwerk/protocol/http/Http2Session.cpp:3012 (Diff revision 1) > + // we just need to rewind to the beginning of the preamble and try again. > + mOutputQueueSent = 0; > + } > + } else { > + // 0RTT succeeded > + if (mOutputQueueSent == mOutputQueueUsed) { maybe just RealignOutputQueue() after attemptingEarlyData is set to false? ::: netwerk/protocol/http/nsHttpConnection.h:380 (Diff revision 1) > // are waiting > // for the end of > // the handsake. > int64_t mContentBytesWritten0RTT; > bool mEarlyDataNegotiated; //Only used for telemetry > + nsAutoCString mEarlyNegotiatedALPN; auto generally only goes on the stack.. so nsCString ::: netwerk/protocol/http/nsHttpConnection.cpp:508 (Diff revision 1) > LOG(("nsHttpConnection::EnsureNPNComplete [this=%p] - %d bytes " > "has been sent during 0RTT.", this, mContentBytesWritten0RTT)); > mContentBytesWritten = mContentBytesWritten0RTT; > + if (mSpdySession) { > + // We had already started 0RTT-spdy, now we need to fully set up > + // spdy, since we know we're sticking with it. LOG(())
Attachment #8836178 -
Flags: review?(mcmanus) → review-
Assignee | ||
Comment 7•7 years ago
|
||
Comment on attachment 8836178 [details] Bug 1322373 - TLS 1.3 early-data for HTTP/2. dragana, would you mind reviewing nsHttpConnection? thanks nick!
Attachment #8836178 -
Flags: review?(dd.mozilla)
Most of these are easy/reasonable enough (especially if we come from the perspective of not caring about reusing the connection - maybe I shouldn't have spent as much time on it as I did!). I'll rework this and should have a new iteration sometime Wednesday. But first, a few responses inline... (In reply to Patrick McManus [:mcmanus] from comment #6) > Comment on attachment 8836178 [details] > Bug 1322373 - TLS 1.3 early-data for HTTP/2. > > https://reviewboard.mozilla.org/r/111614/#review113862 > > I think we need some changes, but they aren't big changes. I also would like > dragana to take a look at nsHttpConnection around the existing early data > bits. but I'm comfortable with doing the h2 parts. > > ::: netwerk/protocol/http/ASpdySession.h:32 > (Diff revision 1) > > virtual bool RoomForMoreStreams() = 0; > > virtual PRIntervalTime IdleTime() = 0; > > virtual uint32_t ReadTimeoutTick(PRIntervalTime now) = 0; > > virtual void DontReuse() = 0; > > + virtual uint32_t SpdyVersion() = 0; > > + virtual nsAHttpTransaction *Take0RTTTransaction() = 0; > > TakeFOO normally means that the callee is giving up its reference so the > caller can have it. It looks like this is just an accessor so it should > either be called GetFOO or just FOO.. (or maybe it should be returning some > already_addreffed var and dropping its own ref?) Indeed - I know I had it returning already_addrefed at one point, not sure when that disappeared. But, if we get rid of the connection reuse logic, this goes away anyway (along with all the other dangling pointers, with a few other minor modifications). > ::: netwerk/protocol/http/Http2Session.cpp:2978 > (Diff revision 1) > > + // We use aAlpnChanged here, because if the ALPN selection hasn't changed, > > + // then we have data we can just use and all we need to do is rewind the > > + // session's internal buffer. If the ALPN selection changed, though, then > > + // we need to do a bit more work to get all our transactions back into a > > + // place where they can do http/1 > > + m0RTTStream->Finish0RTT(aAlpnChanged, aAlpnChanged); > > I think you mean (aRestart, aAlpnChanged) ? That all depends on if we want to re-use what we have in the session buffer or not. If we do, then using aAlpnChanged is the right thing to do - we don't want the transaction to rewind itself when we're going to be using a copy of that anyway (so far as the transaction knows, it's already sent its data, it's just buffered somewhere along the network). This is what the comment above was meant to explain, obviously I did a poor job writing it. I'll re-write for clarity. Or, we can just rewind everything and empty out the session buffer no matter what (except for the connection preamble). > ::: netwerk/protocol/http/Http2Session.cpp:2987 > (Diff revision 1) > > + // 0RTT failed > > + if (aAlpnChanged) { > > + // This is a slightly more involved case - we need to get all our streams/ > > + // transactions back in the queue so they can restart as http/1 > > + > > + // In order for the connection to be reused, we need to reset the > > this path is pretty scary and I think we can simplify it if we don't care > about keeping the connection open - and I don't as the change from h2 to h1 > should not be a normal thing, as long as we recover with a new conn its ok. > > If we are ok with that - won't close()->shutdown() just do the right thing? Absolutely. That's what happened before I got the "reuse the connection" case working. > this is pretty much going to be required when there is more than 1 > transaction in early data anyhow. I think this would be doable by letting all but the first 0rtt transaction rewind and go through the usual close/shutdown path. But that would likely make this code path even scarier than it already is :) > otoh, you might be able to save the optimization by just monkeying with the > reuse flag (setting it back to true) on the conn after Close() has returned > below.. and I think you could set the connection in the trans to null (but > I'm not 100% sure) and the cmgr will just find one according to its normal > precedence rules. I may have missed the proper ordering of things or something, but try as I wanted to, I couldn't get this to work with the (mostly) regular close/shutdown case. So I'm not confident in that. > its the cleanup up of streams here that makes me really nervous. that's been > a source of bugs before and its best to leave it in the existing subroutines > where possible. Sounds fine. Let's just dial back, it's a mostly-silly optimization, anyway. > ::: netwerk/protocol/http/Http2Session.cpp:3012 > (Diff revision 1) > > + // we just need to rewind to the beginning of the preamble and try again. > > + mOutputQueueSent = 0; > > + } > > + } else { > > + // 0RTT succeeded > > + if (mOutputQueueSent == mOutputQueueUsed) { > > maybe just RealignOutputQueue() after attemptingEarlyData is set to false? That could work, but I wanted to avoid a memmove if possible. Maybe realignoutputqueue is smart enough for that, though, and I misread.
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8836178 [details] Bug 1322373 - TLS 1.3 early-data for HTTP/2. https://reviewboard.mozilla.org/r/111614/#review114568 Have you tested chane from early_alpn=h1 to alpn=h2? It should just work because it is an easy case.
Attachment #8836178 -
Flags: review?(dd.mozilla) → review+
Reporter | ||
Comment 11•7 years ago
|
||
(In reply to Dragana Damjanovic [:dragana] from comment #10) > Comment on attachment 8836178 [details] > Bug 1322373 - TLS 1.3 early-data for HTTP/2. > > https://reviewboard.mozilla.org/r/111614/#review114568 > > Have you tested chane from early_alpn=h1 to alpn=h2? It should just work > because it is an easy case. I kind of assumed it worked, since that should've already been handled by the h1 early-data case :) I'll double-check, just to make sure I didn't break anything in the process.
Comment 12•7 years ago
|
||
(In reply to Dragana Damjanovic [:dragana] from comment #10) > Comment on attachment 8836178 [details] > Bug 1322373 - TLS 1.3 early-data for HTTP/2. > > https://reviewboard.mozilla.org/r/111614/#review114568 > > Have you tested chane from early_alpn=h1 to alpn=h2? It should just work > because it is an easy case. I have only reviewed nsHttpConnection part. Thanks for checking h1.
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8836178 [details] Bug 1322373 - TLS 1.3 early-data for HTTP/2. https://reviewboard.mozilla.org/r/111614/#review114938 looks really good to me - thanks. I'm especially pleased it was that easy to accommodate N transactions. can you make sure to test the N 0rtt transactions? It would probably be pretty easy to do so by using the 0rtt origin just for subresources of a sample page (say 3 js files).
Attachment #8836178 -
Flags: review?(mcmanus) → review+
Reporter | ||
Comment 14•7 years ago
|
||
(In reply to Patrick McManus [:mcmanus] from comment #13) > Comment on attachment 8836178 [details] > Bug 1322373 - TLS 1.3 early-data for HTTP/2. > > https://reviewboard.mozilla.org/r/111614/#review114938 > > looks really good to me - thanks. I'm especially pleased it was that easy to > accommodate N transactions. > > can you make sure to test the N 0rtt transactions? It would probably be > pretty easy to do so by using the 0rtt origin just for subresources of a > sample page (say 3 js files). I'd love to... but mint appears to only be able to serve one file. I can't imagine we would send 3 requests for the same js (or image, or ...) file, even if it's referenced 3 times on the page... right? And I think the CF test server doesn't speak h2? So I'm at a loss for how to test this in a way that's useful.
Reporter | ||
Comment 15•7 years ago
|
||
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #14) > (In reply to Patrick McManus [:mcmanus] from comment #13) > > Comment on attachment 8836178 [details] > > Bug 1322373 - TLS 1.3 early-data for HTTP/2. > > > > https://reviewboard.mozilla.org/r/111614/#review114938 > > > > looks really good to me - thanks. I'm especially pleased it was that easy to > > accommodate N transactions. > > > > can you make sure to test the N 0rtt transactions? It would probably be > > pretty easy to do so by using the 0rtt origin just for subresources of a > > sample page (say 3 js files). > > I'd love to... but mint appears to only be able to serve one file. I can't > imagine we would send 3 requests for the same js (or image, or ...) file, > even if it's referenced 3 times on the page... right? And I think the CF > test server doesn't speak h2? So I'm at a loss for how to test this in a way > that's useful. (Though I suppose I could attempt to learn enough to to make mint serve more than one file - perhaps an entire directory!)
Assignee | ||
Comment 16•7 years ago
|
||
that's awkward. I believe you could do 3 XHR gets in parallel for the same URI... we changed things a while back so those weren't serialized on the cache. (put no-cache in the request headers just for emphasis).. I would try it out in a non-early data scenario just to see if it was doing the necessary thing first :) maybe barnes can help with a better strategy. I'll email
Reporter | ||
Comment 17•7 years ago
|
||
Actually, looking at mint again, I think I misinterpreted. There is only one set of bytes it will respond with, but it will respond to *any* URL with that set of bytes. So, I can send as many requests as I want, so long as I don't care that the responses are all the same (I don't).
Reporter | ||
Comment 18•7 years ago
|
||
(In reply to Patrick McManus [:mcmanus] from comment #13) > can you make sure to test the N 0rtt transactions? It would probably be > pretty easy to do so by using the 0rtt origin just for subresources of a > sample page (say 3 js files). And this works! 3 images - lots more early data sent and accepted. Calling this one ready to land.
Comment 19•7 years ago
|
||
Pushed by hurley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/40d227b74dd1 TLS 1.3 early-data for HTTP/2. r=dragana,mcmanus
Comment 20•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/ce178550cf63 for https://treeherder.mozilla.org/logviewer.html#?job_id=78522313&repo=autoland on Android.
Assignee | ||
Comment 21•7 years ago
|
||
nick is on pto this week and this just looks a like a complaint about a compiler and logging string formatter. I'll try and get it relanded.
Comment hidden (obsolete) |
Assignee | ||
Comment 23•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b117a1df6835
Assignee | ||
Comment 24•7 years ago
|
||
I have smoketested this against https://h2o.examp1e.net/ as well
Assignee | ||
Comment 25•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee: hurley → mcmanus
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Attachment #8836178 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8839700 -
Flags: review+
Assignee | ||
Comment 26•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/001bc0207dbcf4b008d9a23030e9b3adc5bdd7ab Bug 1322373 - TLS 1.3 early-data for HTTP/2. r=dragana,mcmanus
Assignee | ||
Comment 27•7 years ago
|
||
nick, I forgot to reset the author of the patch back to you before relanding. sorry!
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/001bc0207dbc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Reporter | ||
Comment 29•7 years ago
|
||
(In reply to Patrick McManus [:mcmanus] from comment #27) > nick, I forgot to reset the author of the patch back to you before > relanding. sorry! No worries. If android's compiler weren't The Worst(tm) you wouldn't have had to fix my mess up :) Thanks for doing that!
You need to log in
before you can comment on or make changes to this bug.
Description
•