Bug 1531344 - Be strict about incorrect chunked encoding. r=mayhemer
☠☠ backed out by 036e5224f2a2 ☠ ☠
authorDragana Damjanovic <dd.mozilla@gmail.com>
Tue, 07 May 2019 17:57:33 +0000
changeset 534837 723587a2ae497817823c4b42f339e91d640a6208
parent 534836 17b693747dbd8b6020787a0111ed4883b517acff
child 534838 c3ea428d6b2a4ab026fea0e1dc5c0776720d1b2d
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs1531344
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1531344 - Be strict about incorrect chunked encoding. r=mayhemer Differential Revision: https://phabricator.services.mozilla.com/D28811
modules/libpref/init/all.js
netwerk/protocol/http/nsHttpChunkedDecoder.h
netwerk/protocol/http/nsHttpHandler.cpp
netwerk/protocol/http/nsHttpHandler.h
netwerk/protocol/http/nsHttpTransaction.cpp
--- a/modules/libpref/init/all.js
+++ b/modules/libpref/init/all.js
@@ -1875,16 +1875,17 @@ pref("network.http.tcp_keepalive.short_l
 // Idle time of TCP connection until first keepalive probe sent.
 pref("network.http.tcp_keepalive.short_lived_idle_time", 10);
 
 pref("network.http.tcp_keepalive.long_lived_connections", true);
 pref("network.http.tcp_keepalive.long_lived_idle_time", 600);
 
 pref("network.http.enforce-framing.http1", false); // should be named "strict"
 pref("network.http.enforce-framing.soft", true);
+pref("network.http.enforce-framing.strict_chunked_encoding", true);
 
 // Max size, in bytes, for received HTTP response header.
 pref("network.http.max_response_header_size", 393216);
 
 // If we should attempt to race the cache and network
 pref("network.http.rcwn.enabled", true);
 pref("network.http.rcwn.cache_queue_normal_threshold", 8);
 pref("network.http.rcwn.cache_queue_priority_threshold", 2);
--- a/netwerk/protocol/http/nsHttpChunkedDecoder.h
+++ b/netwerk/protocol/http/nsHttpChunkedDecoder.h
@@ -29,16 +29,17 @@ class nsHttpChunkedDecoder {
   MOZ_MUST_USE nsresult HandleChunkedContent(char* buf, uint32_t count,
                                              uint32_t* contentRead,
                                              uint32_t* contentRemaining);
 
   nsHttpHeaderArray* Trailers() { return mTrailers.get(); }
 
   nsHttpHeaderArray* TakeTrailers() { return mTrailers.forget(); }
 
+  // How mush data is still missing(needs to arrive) from the current chunk.
   uint32_t GetChunkRemaining() { return mChunkRemaining; }
 
  private:
   MOZ_MUST_USE nsresult ParseChunkRemaining(char* buf, uint32_t count,
                                             uint32_t* countRead);
 
  private:
   nsAutoPtr<nsHttpHeaderArray> mTrailers;
--- a/netwerk/protocol/http/nsHttpHandler.cpp
+++ b/netwerk/protocol/http/nsHttpHandler.cpp
@@ -1821,26 +1821,33 @@ void nsHttpHandler::PrefsChanged(const c
   if (PREF_CHANGED(HTTP_PREF("tcp_keepalive.long_lived_idle_time"))) {
     rv = Preferences::GetInt(HTTP_PREF("tcp_keepalive.long_lived_idle_time"),
                              &val);
     if (NS_SUCCEEDED(rv) && val > 0)
       mTCPKeepaliveLongLivedIdleTimeS = clamped(val, 1, kMaxTCPKeepIdle);
   }
 
   if (PREF_CHANGED(HTTP_PREF("enforce-framing.http1")) ||
-      PREF_CHANGED(HTTP_PREF("enforce-framing.soft"))) {
+      PREF_CHANGED(HTTP_PREF("enforce-framing.soft")) ||
+      PREF_CHANGED(HTTP_PREF("enforce-framing.strict_chunked_encoding"))) {
     rv = Preferences::GetBool(HTTP_PREF("enforce-framing.http1"), &cVar);
     if (NS_SUCCEEDED(rv) && cVar) {
       mEnforceH1Framing = FRAMECHECK_STRICT;
     } else {
-      rv = Preferences::GetBool(HTTP_PREF("enforce-framing.soft"), &cVar);
+      rv = Preferences::GetBool(
+          HTTP_PREF("enforce-framing.strict_chunked_encoding"), &cVar);
       if (NS_SUCCEEDED(rv) && cVar) {
-        mEnforceH1Framing = FRAMECHECK_BARELY;
+        mEnforceH1Framing = FRAMECHECK_STRICT_CHUNKED;
       } else {
-        mEnforceH1Framing = FRAMECHECK_LAX;
+        rv = Preferences::GetBool(HTTP_PREF("enforce-framing.soft"), &cVar);
+        if (NS_SUCCEEDED(rv) && cVar) {
+          mEnforceH1Framing = FRAMECHECK_BARELY;
+        } else {
+          mEnforceH1Framing = FRAMECHECK_LAX;
+        }
       }
     }
   }
 
   if (PREF_CHANGED(TCP_FAST_OPEN_ENABLE)) {
     rv = Preferences::GetBool(TCP_FAST_OPEN_ENABLE, &cVar);
     if (NS_SUCCEEDED(rv)) {
       mUseFastOpen = cVar;
--- a/netwerk/protocol/http/nsHttpHandler.h
+++ b/netwerk/protocol/http/nsHttpHandler.h
@@ -42,17 +42,46 @@ bool OnSocketThread();
 class ATokenBucketEvent;
 class EventTokenBucket;
 class Tickler;
 class nsHttpConnection;
 class nsHttpConnectionInfo;
 class nsHttpTransaction;
 class AltSvcMapping;
 
-enum FrameCheckLevel { FRAMECHECK_LAX, FRAMECHECK_BARELY, FRAMECHECK_STRICT };
+/*
+ * FRAMECHECK_LAX - no check
+ * FRAMECHECK_BARELY - allows:
+ *                     1) that chunk-encoding does not have the last 0-size
+ *                     chunk. So, if a chunked-encoded transfer ends on exactly
+ *                     a chunk boundary we consider that fine. This will allows
+ *                     us to accept buggy servers that do not send the last
+ *                     chunk. It will make us not detect a certain amount of
+ *                     cut-offs.
+ *                     2) When receiving a gzipped response, we consider a
+ *                     gzip stream that doesn't end fine according to the gzip
+ *                     decompressing state machine to be a partial transfer.
+ *                     If a gzipped transfer ends fine according to the
+ *                     decompressor, we do not check for size unalignments.
+ *                     This allows to allow HTTP gzipped responses where the
+ *                     Content-Length is not the same as the actual contents.
+ *                     3) When receiving HTTP that isn't
+ *                     content-encoded/compressed (like in case 2) and not
+ *                     chunked (like in case 1), perform the size comparison
+ *                     between Content-Length: and the actual size received
+ *                     and consider a mismatch to mean a
+ *                     NS_ERROR_NET_PARTIAL_TRANSFER error.
+ * FRAMECHECK_STRICT_CHUNKED - This is the same as FRAMECHECK_BARELY only we
+ *                             enforce that the last 0-size chunk is received
+ *                             in case 1).
+ * FRAMECHECK_STRICT - we also do not allow case 2) and 3) from
+ *                     FRAMECHECK_BARELY.
+ */
+enum FrameCheckLevel { FRAMECHECK_LAX, FRAMECHECK_BARELY,
+                       FRAMECHECK_STRICT_CHUNKED, FRAMECHECK_STRICT };
 
 //-----------------------------------------------------------------------------
 // nsHttpHandler - protocol handler for HTTP and HTTPS
 //-----------------------------------------------------------------------------
 
 class nsHttpHandler final : public nsIHttpProtocolHandler,
                             public nsIObserver,
                             public nsSupportsWeakReference,
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -1124,18 +1124,33 @@ void nsHttpTransaction::Close(nsresult r
 
   if ((mChunkedDecoder || (mContentLength >= int64_t(0))) &&
       (NS_SUCCEEDED(reason) && !mResponseIsComplete)) {
     NS_WARNING("Partial transfer, incomplete HTTP response received");
 
     if ((mHttpResponseCode / 100 == 2) && (mHttpVersion >= HttpVersion::v1_1)) {
       FrameCheckLevel clevel = gHttpHandler->GetEnforceH1Framing();
       if (clevel >= FRAMECHECK_BARELY) {
+        // If clevel == FRAMECHECK_STRICT mark any incomplete response as
+        // partial.
+        // if clevel == FRAMECHECK_BARELY: 1) mark a chunked-encoded response
+        // that do not ends on exactly a chunk boundary as partial; We are not
+        // strict about the last 0-size chunk and do not mark as parial
+        // responses that do not have the last 0-size chunk but do end on a
+        // chunk boundary. (check mChunkedDecoder->GetChunkRemaining() != 0)
+        // 2) mark a transfer that is partial and it is not chunk-encoded or
+        // gzip-encoded or other content-encoding as partial. (check
+        // !mChunkedDecoder && !mContentDecoding && mContentDecodingCheck))
+        // if clevel == FRAMECHECK_STRICT_CHUNKED mark a chunked-encoded
+        // response that ends on exactly a chunk boundary also as partial.
+        // Here a response must have the last 0-size chunk.
         if ((clevel == FRAMECHECK_STRICT) ||
-            (mChunkedDecoder && mChunkedDecoder->GetChunkRemaining()) ||
+            (mChunkedDecoder &&
+             (mChunkedDecoder->GetChunkRemaining() ||
+              (clevel == FRAMECHECK_STRICT_CHUNKED))) ||
             (!mChunkedDecoder && !mContentDecoding && mContentDecodingCheck)) {
           reason = NS_ERROR_NET_PARTIAL_TRANSFER;
           LOG(("Partial transfer, incomplete HTTP response received: %s",
                mChunkedDecoder ? "broken chunk" : "c-l underrun"));
         }
       }
     }