From eb406bc935efb97c20d51f1650c888d415016417 Mon Sep 17 00:00:00 2001 From: Honza Bambas Date: Sat, 24 Aug 2019 13:21:37 +0000 Subject: [PATCH] Bug 1562276 - Proper transaction restart on HTTP_1_1_REQUIRED and h2 proxy, r=dragana. 1. Unsticky connection from a transaction we disable SPDY on to restart 2. Call OnReadSegment from TLSFilterTransaction;FilterOutput to push written data out (renegotiation) 3. Break indefinite loop that may occur during renegotiation by propagating WOULD_BLOCK via mFilterReadCode Differential Revision; https;\\phabricator.services.mozilla.com\D40409 Differential Revision: https://phabricator.services.mozilla.com/D42310 --HG-- extra : moz-landing-system : lando --- netwerk/protocol/http/Http2Session.cpp | 2 + netwerk/protocol/http/TunnelUtils.cpp | 47 +++++++++++++++++++++- netwerk/protocol/http/TunnelUtils.h | 4 ++ netwerk/protocol/http/nsAHttpTransaction.h | 1 + netwerk/protocol/http/nsHttpTransaction.h | 1 + 5 files changed, 54 insertions(+), 1 deletion(-) diff --git a/netwerk/protocol/http/Http2Session.cpp b/netwerk/protocol/http/Http2Session.cpp index 1681a5092015..c73ceb2f70b4 100644 --- a/netwerk/protocol/http/Http2Session.cpp +++ b/netwerk/protocol/http/Http2Session.cpp @@ -3382,6 +3382,8 @@ nsresult Http2Session::WriteSegmentsAgain(nsAHttpSegmentWriter* writer, streamCleanupCode = NS_ERROR_NET_RESET; mInputFrameDataStream->Transaction()->ReuseConnectionOnRestartOK(true); mInputFrameDataStream->Transaction()->DisableSpdy(); + mInputFrameDataStream->Transaction() + ->MakeNonSticky(); // actully allow restart by unsticking } else { streamCleanupCode = mInputFrameDataStream->RecvdData() ? NS_ERROR_NET_PARTIAL_TRANSFER diff --git a/netwerk/protocol/http/TunnelUtils.cpp b/netwerk/protocol/http/TunnelUtils.cpp index e8490207c5f1..0f4c0e79fe8f 100644 --- a/netwerk/protocol/http/TunnelUtils.cpp +++ b/netwerk/protocol/http/TunnelUtils.cpp @@ -26,6 +26,7 @@ #include "nsComponentManagerUtils.h" #include "nsSocketTransport2.h" #include "nsSocketTransportService2.h" +#include "mozilla/AutoRestore.h" #include "mozilla/Mutex.h" namespace mozilla { @@ -46,6 +47,8 @@ TLSFilterTransaction::TLSFilterTransaction(nsAHttpTransaction* aWrapped, mSegmentReader(aReader), mSegmentWriter(aWriter), mFilterReadCode(NS_ERROR_NOT_INITIALIZED), + mFilterReadAmount(0), + mInOnReadSegment(false), mForce(false), mReadSegmentReturnValue(NS_OK), mCloseReason(NS_ERROR_UNEXPECTED), @@ -100,6 +103,11 @@ TLSFilterTransaction::TLSFilterTransaction(nsAHttpTransaction* aWrapped, TLSFilterTransaction::~TLSFilterTransaction() { LOG(("TLSFilterTransaction dtor %p\n", this)); + + // Prevent call to OnReadSegment from FilterOutput, our mSegmentReader is now + // an invalid pointer. + mInOnReadSegment = true; + Cleanup(); } @@ -199,6 +207,11 @@ nsresult TLSFilterTransaction::OnReadSegment(const char* aData, uint32_t aCount, EnsureBuffer(mEncryptedText, aCount + 4096, 0, mEncryptedTextSize); + // Prevents call to OnReadSegment from inside FilterOutput, as we handle it + // here. + AutoRestore inOnReadSegment(mInOnReadSegment); + mInOnReadSegment = true; + while (aCount > 0) { int32_t written = PR_Write(mFD, aData, aCount); LOG(("TLSFilterTransaction %p OnReadSegment PRWrite(%d) = %d %d\n", this, @@ -240,7 +253,8 @@ nsresult TLSFilterTransaction::OnReadSegment(const char* aData, uint32_t aCount, // return OK because all the data was consumed and stored in this buffer Connection()->TransactionHasDataToWrite(this); return NS_OK; - } else if (NS_FAILED(rv)) { + } + if (NS_FAILED(rv)) { return rv; } } @@ -262,6 +276,19 @@ int32_t TLSFilterTransaction::FilterOutput(const char* aBuf, int32_t aAmount) { mEncryptedTextSize); memcpy(&mEncryptedText[mEncryptedTextUsed], aBuf, aAmount); mEncryptedTextUsed += aAmount; + + LOG(("TLSFilterTransaction::FilterOutput %p %d buffered=%u mSegmentReader=%p", + this, aAmount, mEncryptedTextUsed, mSegmentReader)); + + if (!mInOnReadSegment) { + // When called externally, we must make sure any newly written data is + // actually sent to the higher level connection. + // This also covers the case when PR_Read() wrote a re-negotioation + // response. + uint32_t notUsed; + Unused << OnReadSegment("", 0, ¬Used); + } + return aAmount; } @@ -289,10 +316,22 @@ nsresult TLSFilterTransaction::OnWriteSegment(char* aData, uint32_t aCount, // this will call through to FilterInput to get data from the higher // level connection before removing the local TLS layer mFilterReadCode = NS_OK; + mFilterReadAmount = 0; int32_t bytesRead = PR_Read(mFD, aData, aCount); if (bytesRead == -1) { PRErrorCode code = PR_GetError(); if (code == PR_WOULD_BLOCK_ERROR) { + LOG( + ("TLSFilterTransaction::OnWriteSegment %p PR_Read would block, " + "actual read: %d\n", + this, mFilterReadAmount)); + + if (mFilterReadAmount == 0 && NS_SUCCEEDED(mFilterReadCode)) { + // No reading happened, but also no error occured, hence there is no + // condition to break the `again` loop, propagate WOULD_BLOCK through + // mFilterReadCode to break it and poll the socket again for reading. + mFilterReadCode = NS_BASE_STREAM_WOULD_BLOCK; + } return NS_BASE_STREAM_WOULD_BLOCK; } // If reading from the socket succeeded (NS_SUCCEEDED(mFilterReadCode)), @@ -336,6 +375,8 @@ int32_t TLSFilterTransaction::FilterInput(char* aBuf, int32_t aAmount) { if (mReadSegmentReturnValue == NS_BASE_STREAM_WOULD_BLOCK) { mNudgeCounter = 0; } + + mFilterReadAmount += outCountRead; } if (mFilterReadCode == NS_BASE_STREAM_WOULD_BLOCK) { PR_SetError(PR_WOULD_BLOCK_ERROR, 0); @@ -357,6 +398,10 @@ nsresult TLSFilterTransaction::ReadSegments(nsAHttpSegmentReader* aReader, mReadSegmentReturnValue = NS_OK; mSegmentReader = aReader; nsresult rv = mTransaction->ReadSegments(this, aCount, outCountRead); + + // mSegmentReader is left assigned (not nullified) because we want to be able + // to call OnReadSegment directly, it expects mSegmentReader be non-null. + LOG(("TLSFilterTransaction %p called trans->ReadSegments rv=%" PRIx32 " %d\n", this, static_cast(rv), *outCountRead)); if (NS_SUCCEEDED(rv) && diff --git a/netwerk/protocol/http/TunnelUtils.h b/netwerk/protocol/http/TunnelUtils.h index 2b16e088f745..3b166c7fabe0 100644 --- a/netwerk/protocol/http/TunnelUtils.h +++ b/netwerk/protocol/http/TunnelUtils.h @@ -186,6 +186,10 @@ class TLSFilterTransaction final : public nsAHttpTransaction, nsAHttpSegmentWriter* mSegmentWriter; nsresult mFilterReadCode; + int32_t mFilterReadAmount; + // Set only when we are calling PR_Write from inside OnReadSegment. Prevents + // calling back to OnReadSegment from inside FilterOutput. + bool mInOnReadSegment; bool mForce; nsresult mReadSegmentReturnValue; // Before Close() is called this is NS_ERROR_UNEXPECTED, in Close() we either diff --git a/netwerk/protocol/http/nsAHttpTransaction.h b/netwerk/protocol/http/nsAHttpTransaction.h index 26bd7ba7e77b..d3ed279e6db8 100644 --- a/netwerk/protocol/http/nsAHttpTransaction.h +++ b/netwerk/protocol/http/nsAHttpTransaction.h @@ -180,6 +180,7 @@ class nsAHttpTransaction : public nsSupportsWeakReference { } virtual void DisableSpdy() {} + virtual void MakeNonSticky() {} virtual void ReuseConnectionOnRestartOK(bool) {} // Returns true if early-data or fast open is possible. diff --git a/netwerk/protocol/http/nsHttpTransaction.h b/netwerk/protocol/http/nsHttpTransaction.h index 328db5ed7f54..09fd53f424ed 100644 --- a/netwerk/protocol/http/nsHttpTransaction.h +++ b/netwerk/protocol/http/nsHttpTransaction.h @@ -119,6 +119,7 @@ class nsHttpTransaction final : public nsAHttpTransaction, void EnableKeepAlive() { mCaps |= NS_HTTP_ALLOW_KEEPALIVE; } void MakeSticky() { mCaps |= NS_HTTP_STICKY_CONNECTION; } + void MakeNonSticky() override { mCaps &= ~NS_HTTP_STICKY_CONNECTION; } // SetPriority() may only be used by the connection manager. void SetPriority(int32_t priority) { mPriority = priority; }