[MD settings] i18n source stream filtering

content/browser/webui/i18n_source_stream.cc

+// 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 "content/browser/webui/i18n_source_stream.h"
+
+#include <algorithm>
+#include <utility>
+
+#include "base/logging.h"
+#include "net/base/io_buffer.h"
+
+namespace content {
+
+I18nSourceStream::~I18nSourceStream() {}
+
+std::unique_ptr<I18nSourceStream> I18nSourceStream::Create(
+    std::unique_ptr<SourceStream> upstream,
+    SourceStream::SourceType type,
+    const ui::TemplateReplacements* replacements) {
+  DCHECK(replacements);
+  std::unique_ptr<I18nSourceStream> source(
+      new I18nSourceStream(std::move(upstream), type, replacements));
+  return source;
+}
+
+I18nSourceStream::I18nSourceStream(std::unique_ptr<SourceStream> upstream,
+                                   SourceStream::SourceType type,
+                                   const ui::TemplateReplacements* replacements)
+    : FilterSourceStream(type, std::move(upstream)),
+      replacements_(replacements) {}
+
+std::string I18nSourceStream::GetTypeAsString() const {
+  return "i18n";
+}
+
+int I18nSourceStream::FilterData(net::IOBuffer* output_buffer,
+                                 int output_buffer_size,
+                                 net::IOBuffer* input_buffer,
+                                 int input_buffer_size,
+                                 int* consumed_bytes,
+                                 bool upstream_end_reached) {
+  *consumed_bytes = 0;
+  int bytes_out = 0;
+  // TODO(dschuyler): Perform replacements without accumulation.
+  if (!upstream_end_reached) {
+    // Accumulate.
+    input_.append(input_buffer->data(), input_buffer_size);

[mmenke, 2016/12/09 18:40:17]

I assume there's no way for non-chrome files to reach this point (Extension
requests, etc).

[dschuyler, 2016/12/09 22:31:17]

Same here, but:

Is there something that should change if non-chrome files could be run through
this filter? IIUC, if something rouge did run data through this filter, what
they could do is get the i18n values by passing a buffer with the $i18n{} tags
with the keys. Which would be ineffective as an exploit afaik, since the i18n
values are just translated strings with no PII.

[mmenke, 2016/12/09 22:54:08]

You're buffering the entire response in a single string, potentially OOMing the
browser if this were applied to an attacker-provided resource.  The other
filters all stream data as they get it.

[mmenke, 2016/12/09 22:55:21]

Also, just from a more general memory standpoint, even if we only use it for
chrome resources, this could be a problem on mobile, where memory is more
limited.

[dschuyler, 2016/12/10 00:05:55]

Acknowledged.

[dschuyler, 2016/12/10 00:05:55]

I'd like to make this more streaming, that's what the todo on line 45 is about.
This CL is intended to get a baseline i18n working.

+    *consumed_bytes = input_buffer_size;

[mmenke, 2016/12/09 18:40:17]

I think both of these lines can be done unconditionally.  Currently, if you
always consume all the data, you'll be given a final call with an
input_buffer_size of 0, but I don't think that's part of the API contract. 
Maybe add a test case for this?

[dschuyler, 2016/12/09 22:31:17]

I agree that they could be moved outside of the if(), but the condition would
then move to line 50 (like "if (upstream_end_reached && !input_.empty() &&
output_.empty())"). That would mean that the condition is still checked and
append() is called during the processing and the draining. I agree that the
append() would effectively be a no-op, but it may still be a cache miss to go
over to that code (to do nothing and come back, err, unless it's inlined). I
also think it's clearer to see the three stages with three blocks.

I did move the "*consumed_bytes = input_buffer_size;" line.

Please let me know if you're unconvinced and still want this changed ("please
change it" would be clear enough).

[mmenke, 2016/12/09 22:54:08]

I'm not sure we're on the same page here.  The API contract does *not* guarantee
that if |upstream_end_reached| is true, then input_buffer will be empty.  The
fact that is currently always the case is just the behavior of the current
implementation.  So the current code does not adhere to the API that
FilterSourceStream requires.

[dschuyler, 2016/12/10 00:05:55]

I didn't know that. Thanks, that helps me understand the issue better (I feel
like I learned something; that makes me feel better about the change than just
changing it without knowing why).

+  } else if (!input_.empty() && output_.empty()) {
+    // Process.
+    output_ = ui::ReplaceTemplateExpressions(base::StringPiece(input_),

[mmenke, 2016/12/09 18:40:17]

base::StringPiece not needed - it should be implicitly converted.

[mmenke, 2016/12/09 18:40:17]

Include string_piece.h (Even without the explicit conversion, need the header
for the implicit one)

[dschuyler, 2016/12/09 22:31:16]

Done.

[dschuyler, 2016/12/09 22:31:17]

Done.

+                                             *replacements_);
+    input_.clear();

[mmenke, 2016/12/09 18:40:17]

Shouldn't modify input_ here.  I don't think it breaks anything, it's not
expected.

[dschuyler, 2016/12/09 22:31:16]

done.

+  }
+
+  if (!output_.empty()) {
+    // Drain.
+    bytes_out = std::min(static_cast<int>(output_.size()), output_buffer_size);

[mmenke, 2016/12/09 18:40:17]

Doesn't really matter, but from a "this works even for insanely large strings
that it shouldn't be fed anyways", standpoint, this should be:

bytes_out = std::min(output_.size(), static_cast<size_t>(output_buffer_size));

(i.e., this has no issues with overflow, though it does do two casts instead of
one)

[dschuyler, 2016/12/09 22:31:16]

I can see that being good practice in general (convert to the larger type).
Since this won't have an affect unless the type for output_buffer_size changes;
and that change will generate a type mismatch message from std::min, it seems
good to skip the extra conversions and cast to int here. i.e. if the type of
output_buffer_size could change without a warning, then I'd want to make this a
size_t cast now to avoid errors (I would have done it that way twenty years ago,
for example).

Please let me know if you're unconvinced and still want this changed ("please
change it" would be clear enough).

[mmenke, 2016/12/09 22:54:08]

There's nothing preventing output_buffer_size_ from being over 2 GB, so I'm not
sure what you mean by this changing?  Admittedly, it doesn't really matter here,
as, barring a bug, it shouldn't be that big.  Just thought I'm mention it.

[dschuyler, 2016/12/10 00:05:55]

I wasn't seeing the signed vs unsigned issue. Thanks!

+    output_.copy(output_buffer->data(), bytes_out);
+    output_.erase(0, bytes_out);

[mmenke, 2016/12/09 18:40:17]

Is it worth the erase?  The memory allocated by the std::string may well not be
reduced by this call, so this could just be a wasted copy.  Could instead
remember the offset of the last byte that was written.

[dschuyler, 2016/12/09 22:31:16]

Depending on how erase() is implemented, this may be about the same either way.
I agree though, that it's nice to make it explicit - thanks for suggesting it.

Done.

+  }
+
+  return bytes_out;
+}
+
+}  // namespace content