Blimp: add packet-level DEFLATE compression using zlib.

blimp/net/compressed_packet_reader.cc

+// Copyright 2015 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 "blimp/net/compressed_packet_reader.h"
+
+#include <iostream>
+
+#include "base/callback_helpers.h"
+#include "base/logging.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/sys_byteorder.h"
+#include "blimp/net/common.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/socket/stream_socket.h"
+
+namespace blimp {
+
+CompressedPacketReader::CompressedPacketReader(scoped_ptr<PacketReader> source)
+    : source_(std::move(source)),
+      compressed_buf_(new net::GrowableIOBuffer),
+      weak_factory_(this) {
+  DCHECK(source_);
+
+  memset(&zlib_stream_, 0, sizeof(z_stream));
+
+  // MAX_WBITS means we are using the maximal window size for decompression;
+  // a negative value means that we are ignoring headers and CRC checks.
+  int err = inflateInit2(&zlib_stream_, -MAX_WBITS);

[Wez, 2016/03/29 00:45:39]

nit: |init_result|?

[Kevin M, 2016/03/29 01:25:55]

Done.

+  DCHECK_EQ(err, Z_OK);

[Wez, 2016/03/29 00:45:39]

nit: Suggest DCHECK_EQ(Z_OK, err)

[Kevin M, 2016/03/29 01:25:55]

Done.

+}
+
+CompressedPacketReader::~CompressedPacketReader() {
+  inflateEnd(&zlib_stream_);
+}
+
+void CompressedPacketReader::ReadPacket(
+    const scoped_refptr<net::GrowableIOBuffer>& decompressed_buf,
+    const net::CompletionCallback& cb) {

[Wez, 2016/03/29 00:45:39]

nit: |callback|, here and below.

[Kevin M, 2016/03/29 01:25:55]

Done.

+  DCHECK(decompressed_buf);
+  DCHECK(!cb.is_null());
+  source_->ReadPacket(
+      compressed_buf_,
+      base::Bind(&CompressedPacketReader::ReadCompressedPacketDone,
+                 weak_factory_.GetWeakPtr(), decompressed_buf, cb));
+}
+
+void CompressedPacketReader::ReadCompressedPacketDone(
+    const scoped_refptr<net::GrowableIOBuffer> decompressed_buf,
+    const net::CompletionCallback& cb,
+    int result) {
+  if (result <= 0) {
+    cb.Run(result);
+    return;
+  }
+
+  cb.Run(DecompressPacket(compressed_buf_->data(), result, decompressed_buf));
+}
+
+int CompressedPacketReader::DecompressPacket(
+    char* compressed,
+    int compressed_size,
+    const scoped_refptr<net::GrowableIOBuffer>& decompressed) {
+  // Read the size of the uncompressed data from the block heading.
+  size_t decompressed_size =
+      base::NetToHost32(*reinterpret_cast<uint32_t*>(compressed));

[Wez, 2016/03/29 00:45:39]

Don't you need to check that |compressed_size| is >= sizeof(uint32_t) here?

I'd also prefer that, given a compressed buffer + size, we have simple static
helper methods that pull out e.g. the decompressed size, the compressed-data
pointer, and the actual compressed-data size, to make the rest of this more
readable.  Or, at least define a kHeaderSize = sizeof(uint32_t) that can be used
below.

[Kevin M, 2016/03/29 01:25:55]

Done.

+  if (decompressed_size == 0) {
+    // Empty payload - nothing to do.

[Wez, 2016/03/29 00:45:39]

Why would we allow an empty but compressed-looking payload, rather than a
zero-length message at the underlying layer?

[Kevin M, 2016/03/29 01:25:55]

Good question. Done.

+    return 0;
+  }
+  if (decompressed_size > kMaxPacketPayloadSizeBytes) {
+    // Payload is too large.
+    return net::ERR_FILE_TOO_BIG;
+  }
+  if (static_cast<uint32_t>(decompressed->capacity()) < decompressed_size) {

[Wez, 2016/03/29 00:45:39]

nit: Don't abutt this buffer-resize logic into the early-exit sanity checks; I'd
suggest:

FetchSize

IsItZero?
IsItInsane?

SetCapacityToSize

as the logical blocks here, each w/ a one-line comment introducing them, e.g.
"Check that the payload size is within bounds."

[Kevin M, 2016/03/29 01:25:55]

Done.

+    decompressed->SetCapacity(decompressed_size);
+  }
+
+  zlib_stream_.next_in =
+      reinterpret_cast<uint8_t*>(compressed + sizeof(uint32_t));
+  zlib_stream_.avail_in =
+      static_cast<unsigned>(compressed_size - sizeof(uint32_t));

[Wez, 2016/03/29 00:45:39]

unsigned what?

What about using a safe type-conversion to get compressed_size from |int| to
|size_t| and then doing the -= on that?

[Kevin M, 2016/03/29 01:25:55]

Done.

+  zlib_stream_.next_out = reinterpret_cast<uint8_t*>(decompressed->data());
+  zlib_stream_.avail_out = decompressed_size;
+  int err = inflate(&zlib_stream_, Z_SYNC_FLUSH);

[Wez, 2016/03/29 00:45:39]

nit: |inflate_result|

[Kevin M, 2016/03/29 01:25:55]

Done.

+  if (err != Z_OK) {
+    DLOG(ERROR) << "inflate() returned unexpected error code: " << err;
+    return net::ERR_UNEXPECTED;
+  }
+
+  // Verify that the decompressed block size is a perfect fit for the output
+  // buffer.
+  if (zlib_stream_.avail_in > 0) {

[Wez, 2016/03/29 00:45:40]

This provides us a mechanism for telling when our buffer was too small; we could
e.g. double the decompressed-data buffer in such situations, allowing us to try
decompressing into the buffer and resize it if there wasn't enough space,
removing the need for the explicit decompressed-size header. WDYT?

[Kevin M, 2016/03/29 01:25:55]

I was on the fence about this. I favored the simplicity of this approach
initially, but now that I have a stable solution and better understand how zlib
works, I can go with the resizing route.

Added a TODO that I'll address in an upcoming patch.

[Kevin M, 2016/03/29 20:36:23]

Done. Removed the payload header size. Instead of dynamically resizing the
buffer, I am simply sizing the buffer up to the payload size limit.

+    DLOG(ERROR) << "Decompressed payload is larger than expected size ("
+                << decompressed_size << " bytes.)";
+    return net::ERR_FILE_TOO_BIG;
+  }
+  if (zlib_stream_.avail_out > 0) {
+    DLOG(ERROR) << "Unexpected end of compressed block, num of extra bytes: "
+                << zlib_stream_.avail_out;
+    return net::ERR_UNEXPECTED;
+  }
+
+  return decompressed_size;
+}
+
+}  // namespace blimp