Add net::GzipSourceStream

net/filter/gzip_source_stream.cc

Index: net/filter/gzip_source_stream.cc
diff --git a/net/filter/gzip_source_stream.cc b/net/filter/gzip_source_stream.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7bf55d8f4db4fa895ccf638c6e331d2432f0bab7
--- /dev/null
+++ b/net/filter/gzip_source_stream.cc
@@ -0,0 +1,223 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/filter/gzip_source_stream.h"
+
+#include "base/bind.h"
+#include "base/bit_cast.h"
+#include "base/logging.h"
+#include "net/base/io_buffer.h"
+#include "third_party/zlib/zlib.h"
+
+namespace net {
+
+namespace {
+
+const char kDeflate[] = "DEFLATE";
+const char kGzip[] = "GZIP";
+const char kGzipFallback[] = "GZIP_FALLBACK";
+
+}  // namespace
+
+GzipSourceStream::~GzipSourceStream() {
+  if (zlib_stream_)
+    inflateEnd(zlib_stream_.get());
+}
+
+std::unique_ptr<GzipSourceStream> GzipSourceStream::Create(
+    std::unique_ptr<SourceStream> upstream,
+    SourceStream::SourceType type) {
+  std::unique_ptr<GzipSourceStream> source(
+      new GzipSourceStream(std::move(upstream), type));
+
+  if (!source->Init())
+    return nullptr;
+  return source;
+}
+
+GzipSourceStream::GzipSourceStream(std::unique_ptr<SourceStream> upstream,
+                                   SourceStream::SourceType type)
+    : FilterSourceStream(type, std::move(upstream)),
+      zlib_header_added_(false),
+      gzip_footer_bytes_left_(0),
+      input_state_(STATE_START) {}
+
+bool GzipSourceStream::Init() {
+  zlib_stream_.reset(new z_stream);
+  if (!zlib_stream_)
+    return false;
+  memset(zlib_stream_.get(), 0, sizeof(z_stream));
+
+  int ret;
+  if (type() == TYPE_GZIP || type() == TYPE_GZIP_FALLBACK) {
+    ret = inflateInit2(zlib_stream_.get(), -MAX_WBITS);
+  } else {
+    ret = inflateInit(zlib_stream_.get());
+  }
+  DCHECK_NE(Z_VERSION_ERROR, ret);
+  return ret == Z_OK;
+}
+
+std::string GzipSourceStream::GetTypeAsString() const {
+  switch (type()) {
+    case TYPE_GZIP:
+      return kGzip;
+    case TYPE_GZIP_FALLBACK:
+      return kGzipFallback;
+    case TYPE_DEFLATE:
+      return kDeflate;
+    default:
+      NOTREACHED();
+      return "";
+  }
+}
+
+int GzipSourceStream::FilterData(IOBuffer* output_buffer,
+                                 int output_buffer_size,
+                                 IOBuffer* input_buffer,
+                                 int input_buffer_size,
+                                 int* consumed_bytes,
+                                 bool /*upstream_end_reached*/) {
+  if (input_buffer_size == 0)
+    return 0;
+  *consumed_bytes = 0;
+  char* input_data = input_buffer->data();
+  int input_data_size = input_buffer_size;
+  int bytes_out = 0;
+  while (true) {
+    InputState state = input_state_;
+    switch (state) {
+      case STATE_START: {
+        if (type() == TYPE_DEFLATE) {
+          input_state_ = STATE_COMPRESSED_BODY;
+          break;
+        }
+        // If this stream is not really gzipped as detected by
+        // ShouldFallbackToPlain, pretend that the zlib stream has ended.
+        if (ShouldFallbackToPlain(input_data[0])) {

[Randy Smith (Not in Mondays), 2016/09/23 20:53:47]

Suggestion: I'll call out that this is assuming that input data is non-zero,
which will always be true because of the combination of a) the conditional about
input_buffer_size == 0 at the top of the function, and b) the fact that no other
state enters STATE_START.  I'm finding myself wondering if that's worth either a
comment or a DCHECK, but I'm not sure.  Up to you. 

[xunjieli, 2016/09/26 15:29:43]

Done. I added a DCHECK.

+          input_state_ = STATE_UNCOMPRESSED_BODY;
+        } else {
+          input_state_ = STATE_GZIP_HEADER;
+        }
+        break;
+      }
+      case STATE_GZIP_HEADER: {
+        const size_t kGzipFooterBytes = 8;
+        const char* end = nullptr;
+        GZipHeader::Status status =
+            gzip_header_.ReadMore(input_data, input_data_size, &end);
+        if (status == GZipHeader::INCOMPLETE_HEADER) {
+          input_data += input_data_size;
+          input_data_size = 0;
+        } else if (status == GZipHeader::COMPLETE_HEADER) {
+          // If there is a valid header, there should also be a valid footer.
+          gzip_footer_bytes_left_ = kGzipFooterBytes;
+          int bytes_consumed = end - input_data;
+          input_data += bytes_consumed;
+          input_data_size -= bytes_consumed;

[Randy Smith (Not in Mondays), 2016/09/23 20:53:47]

If this drops input_data_size to zero, we want to return, correct?  I think the
best way to handle that is by pulling the "if (input_buffer_size == 0)"
conditional inside the while loop and making it on input_data_size.

Actually, isn't that always true for INCOMPLETE_HEADER, above?  Does that
suggest a hole in testing (i.e. that there's no test that triggers the
INCOMPLETE_HEADER return from GzipHeader::ReadMore())?

[xunjieli, 2016/09/26 15:29:43]

Done. This is a bug! Thanks for catching it. There's a test in
url_request_job_unittest.cc (SlowFilterRead), but not here. I have added a test.

+          input_state_ = STATE_COMPRESSED_BODY;
+        } else if (status == GZipHeader::INVALID_HEADER) {
+          return ERR_CONTENT_DECODING_FAILED;
+        }
+        break;
+      }
+      case STATE_COMPRESSED_BODY: {
+        DCHECK_LE(0, input_data_size);
+        zlib_stream_.get()->next_in = bit_cast<Bytef*>(input_data);
+        zlib_stream_.get()->avail_in = input_data_size;
+        zlib_stream_.get()->next_out = bit_cast<Bytef*>(output_buffer->data());
+        zlib_stream_.get()->avail_out = output_buffer_size;

[Randy Smith (Not in Mondays), 2016/09/23 20:53:47]

Suggestion: The fact that {next,avail}_out are being set directly from the
output buffer passed into the function is assuming that the
STATE_COMPRESSED_BODY element in the switch statement will never be visited more
than once for a single invocation of FilterData().  I believe that that
assumption is both based on the fact that we never leave this block with a
simple "break;"; we either return or set a new state and break.  But I find
myself a bit uncomfortable with that chain of assumptions (even though I think
it's a valid chain).  How would you feel about having a DCHECK at the top of
this clause asserting that this is the only time for this function invocation
the clause has been entered?  (I'd imagine this would involve declaring a
boolean variable in function scope, asserting on it here, and then changing its
state.)

[xunjieli, 2016/09/26 15:29:43]

Done.

+
+        int ret = inflate(zlib_stream_.get(), Z_NO_FLUSH);
+
+        // Sometimes misconfigured servers omit the zlib header, relying on
+        // clients to splice it back in.
+        if (ret < 0 && !zlib_header_added_) {
+          zlib_header_added_ = true;
+          if (!InsertZlibHeader())
+            return ERR_CONTENT_DECODING_FAILED;
+
+          zlib_stream_.get()->next_in = bit_cast<Bytef*>(input_data);
+          zlib_stream_.get()->avail_in = input_data_size;
+          zlib_stream_.get()->next_out =
+              bit_cast<Bytef*>(output_buffer->data());
+          zlib_stream_.get()->avail_out = output_buffer_size;
+
+          ret = inflate(zlib_stream_.get(), Z_NO_FLUSH);
+          // TODO(xunjieli): add a histogram to see how often this happens. The
+          // original bug for this behavior was ancient and maybe it doesn't
+          // happen in the wild any more? crbug.com/649339
+        }
+
+        int bytes_used = input_data_size - zlib_stream_.get()->avail_in;
+        bytes_out = output_buffer_size - zlib_stream_.get()->avail_out;
+
+        input_data_size -= bytes_used;
+        input_data += bytes_used;
+
+        if (ret != Z_STREAM_END && ret != Z_OK)
+          return ERR_CONTENT_DECODING_FAILED;

[Randy Smith (Not in Mondays), 2016/09/23 20:53:47]

nit, suggestion: Is there a reason this isn't up above before all the arithmetic
around how much has been consumed and produced, which I sorta presume isn't
valid if inflate() returned an error?

[xunjieli, 2016/09/26 15:29:43]

Done.

+        if (ret == Z_STREAM_END) {
+          input_state_ = STATE_GZIP_FOOTER;
+          break;
+        }
+        *consumed_bytes = input_buffer_size - input_data_size;
+        return bytes_out;
+      }
+      case STATE_GZIP_FOOTER: {
+        size_t footer_bytes_skipped = NumGzipFooterBytesToSkip(input_data_size);
+        input_data_size -= footer_bytes_skipped;
+        input_data += footer_bytes_skipped;
+        input_state_ = STATE_UNCOMPRESSED_BODY;

[Randy Smith (Not in Mondays), 2016/09/23 20:53:47]

NumGzipFooterBytesToSkip() handles being called repeatedly, but this code
unilaterally transitioning to STATE_UNCOMPRESSED_BODY seems to assume that the
footer will always fit into a single input buffer.  Shouldn't this transition
only occur if gzip_footer_bytes_left_ == 0?  

(Anticipated future comment: I'd rather not read&set gzip_footer_bytes_left_ in
NumGzip_FooterBytesToSkip and also read it here; that feels like a (minor)
layering violation.  So if you solve this the obvious way and test
gzip_footer_bytes_left_ in evaluating the state transition, could you also
inline NumGzipFooterBytesToSkip()?  Or alternatively simply transition and break
if NumGzipFooterBytesToSkip() returns 0?)

[xunjieli, 2016/09/26 15:29:43]

Done. Good catch. This is a bug. The newly added SmallOutputBuffer test will
catch this. 

+        break;
+      }
+      case STATE_UNCOMPRESSED_BODY: {
+        int to_copy = std::min(input_data_size, output_buffer_size - bytes_out);

[Randy Smith (Not in Mondays), 2016/09/23 20:53:47]

Suggestion: I don't know if it's worth it, but if you wanted to, you could do
the same thing with the output buffer/size you've done with the input buffer &
size and have variables that track the current place to write to and the amount
of space that's available to write.  (It would be a different way of addressing
my assertion concern above about only going through STATE_COMPRESSED_BODY once
per call, because you'd be updating the output pointers.)

[xunjieli, 2016/09/26 15:29:43]

Acknowledged. The new code is less clustered. |output_buffer_size| is only used
in three places. Since we already have a |bytes_out| so I didn't add the
variables.  

+        memcpy(output_buffer->data() + bytes_out, input_data, to_copy);
+        input_data_size -= to_copy;
+        input_data += to_copy;
+        *consumed_bytes = input_buffer_size - input_data_size;
+        return bytes_out + to_copy;
+      }
+    }
+  }
+  NOTREACHED();
+  return ERR_UNEXPECTED;
+}
+
+bool GzipSourceStream::InsertZlibHeader() {
+  char dummy_header[] = {0x78, 0x01};
+  char dummy_output[4];
+
+  inflateReset(zlib_stream_.get());
+  zlib_stream_.get()->next_in = bit_cast<Bytef*>(&dummy_header[0]);
+  zlib_stream_.get()->avail_in = sizeof(dummy_header);
+  zlib_stream_.get()->next_out = bit_cast<Bytef*>(&dummy_output[0]);
+  zlib_stream_.get()->avail_out = sizeof(dummy_output);
+
+  int ret = inflate(zlib_stream_.get(), Z_NO_FLUSH);
+  return ret == Z_OK;
+}
+
+// Dumb heuristic. Gzip files always start with a two-byte magic value per RFC
+// 1952 2.3.1, so if the first byte isn't the first byte of the gzip magic, and
+// this filter is checking whether it should fallback, then fallback.
+bool GzipSourceStream::ShouldFallbackToPlain(char first_byte) {
+  if (type() != TYPE_GZIP_FALLBACK)
+    return false;
+  static const char kGzipFirstByte = 0x1f;
+  return first_byte != kGzipFirstByte;
+}
+
+size_t GzipSourceStream::NumGzipFooterBytesToSkip(int input_buffer_size) {
+  if (gzip_footer_bytes_left_ == 0)
+    return 0;
+  size_t to_read = gzip_footer_bytes_left_;
+  if (to_read > base::checked_cast<size_t>(input_buffer_size))
+    to_read = input_buffer_size;
+  gzip_footer_bytes_left_ -= to_read;
+  return to_read;
+}
+
+}  // namespace net