Add brotli content-encoding filter.

net/filter/brotli_filter.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 "net/filter/brotli_filter.h"
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "third_party/brotli/dec/decode.h"
+
+namespace net {
+
+class BrotliFilter : public Filter {
+ public:
+  BrotliFilter(FilterType type)
+      : Filter(type), decoding_status_(DECODING_UNINITIALIZED) {}
+
+  ~BrotliFilter() override {
+    if (decoding_status_ != DECODING_UNINITIALIZED)
+      BrotliStateCleanup(brotli_state_.get());
+  }
+
+  // Initializes filter decoding mode and internal control blocks.
+  // The function returns true if success and false otherwise.
+  // The filter can only be initialized once.
+  bool InitDecoding() {
+    if (decoding_status_ != DECODING_UNINITIALIZED)
+      return false;
+
+    brotli_state_.reset(new BrotliState);

[Ryan Sleevi, 2015/12/28 19:30:53]

Does this actually need to be a pointer? It seems like you could make
BrotliState a member variable of this class, and just leave InitDecoding /
Cleanup to do the BrotliStateInit/BrotliStateCleanup

[eustas, 2015/12/29 13:15:39]

No need. It just moved from previous patches. Done.

+    BrotliStateInit(brotli_state_.get());
+
+    decoding_status_ = DECODING_IN_PROGRESS;
+    return true;
+  }
+
+  // Decodes the pre-filter data and writes the output into the |dest_buffer|
+  // passed in.
+  // The function returns FilterStatus. See filter.h for its description.
+  //
+  // Upon entry, |*dest_len| is the total size (in number of chars) of the
+  // destination buffer. Upon exit, |*dest_len| is the actual number of chars
+  // written into the destination buffer.
+  //
+  // This function will fail if there is no pre-filter data in the
+  // |stream_buffer_|. On the other hand, |*dest_len| can be 0 upon successful
+  // return. For example, decompressor may process some pre-filter data
+  // but not produce output yet.
+  FilterStatus ReadFilteredData(char* dest_buffer, int* dest_len) override {
+    if (!dest_buffer || !dest_len || *dest_len <= 0)
+      return Filter::FILTER_ERROR;
+
+    if (decoding_status_ == DECODING_DONE) {
+      *dest_len = 0;
+      return Filter::FILTER_DONE;
+    }
+
+    if (decoding_status_ != DECODING_IN_PROGRESS)
+      return Filter::FILTER_ERROR;
+
+    size_t available_in = stream_data_len_;
+    const uint8_t* next_in = bit_cast<uint8_t*>(next_stream_data_);
+    size_t available_out = *dest_len;
+    uint8_t* next_out = bit_cast<uint8_t*>(dest_buffer);
+    size_t total_out = 0;
+    BrotliResult result =
+        BrotliDecompressStream(&available_in, &next_in, &available_out,
+                               &next_out, &total_out, brotli_state_.get());
+    int bytes_written = *dest_len - available_out;

[Ryan Sleevi, 2015/12/28 19:30:53]

DANGER: You're mixing signed and unsigned types in math, and using a third-party
library. While it may be safe, it can often cause subtle bugs if the third-party
library ends up changing API behaviour or contract.

It may be wise to use the base/safe_numerics we have and use the appropriate
checked types, as an defense in depth when mixing these. For example, I think we
want to avoid 'decompression bomb' type attacks where the attacker is able to
return a >int sized bit of data. It also gives you strong guards against bad
calls to ReadFilteredData, where dest_len might be a negative value (which would
be developer error, but hey, mistakes happen)

[eustas, 2015/12/29 13:15:39]

Good idea. Done.

+
+    Filter::FilterStatus status;
+    switch (result) {
+      case BROTLI_RESULT_ERROR: {
+        status = Filter::FILTER_ERROR;
+        break;
+      }
+      // Fall through.

[Ryan Sleevi, 2015/12/28 19:30:53]

This comment doesn't seem correct; you break on line 75. Should you move line 73
to 94, and then actually let it fall through to the default case?

[eustas, 2015/12/29 13:15:39]

I meant that "case BROTLI_RESULT_NEEDS_MORE_OUTPUT:" falls through to "case
BROTLI_RESULT_SUCCESS: {"

But combining error & default also looks good.

+      case BROTLI_RESULT_NEEDS_MORE_OUTPUT:
+      case BROTLI_RESULT_SUCCESS: {
+        status = Filter::FILTER_OK;
+        *dest_len = bytes_written;
+        stream_data_len_ = available_in;
+        next_stream_data_ = bit_cast<char*>(next_in);
+        if (result == BROTLI_RESULT_SUCCESS)
+          status = Filter::FILTER_DONE;
+        break;
+      }
+      case BROTLI_RESULT_NEEDS_MORE_INPUT: {
+        status = Filter::FILTER_NEED_MORE_DATA;
+        *dest_len = bytes_written;
+        stream_data_len_ = 0;
+        next_stream_data_ = nullptr;
+        break;
+      }
+      default: {
+        status = Filter::FILTER_ERROR;
+        break;
+      }
+    }
+
+    if (status == Filter::FILTER_DONE) {
+      decoding_status_ = DECODING_DONE;
+    } else if (status == Filter::FILTER_ERROR) {
+      decoding_status_ = DECODING_ERROR;
+    }

[Ryan Sleevi, 2015/12/28 19:30:53]

Should this be a switch, rather than a conditional?

For example, when reading this, it's ambiguous whether or not there are more
statuses (there are; FILTER_NEED_MORE_DATA and FILTER_OK) or what the 'expected'
value of decoding_status_ should be.

[eustas, 2015/12/29 13:15:39]

Backed this conditional to previous switch; now it is easier to track state
transitions.

+
+    return status;
+  }
+
+ private:
+  enum DecodingStatus {
+    DECODING_UNINITIALIZED,
+    DECODING_IN_PROGRESS,
+    DECODING_DONE,
+    DECODING_ERROR
+  };
+
+  // Tracks the status of decoding.
+  // This variable is initialized by InitDecoding and updated only by
+  // ReadFilteredData.
+  DecodingStatus decoding_status_;
+
+  scoped_ptr<BrotliStateStruct> brotli_state_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrotliFilter);
+};
+
+Filter* CreateBrotliFilter(Filter::FilterType type_id) {
+  scoped_ptr<BrotliFilter> brotli_filter(new BrotliFilter(type_id));
+  return brotli_filter->InitDecoding() ? brotli_filter.release() : nullptr;
+}
+
+}  // namespace net