Support replacement of IP address resolutions via command line flag

net/base/ip_pattern.cc

+// Copyright (c) 2014 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/base/ip_pattern.h"
+
+#include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_tokenizer.h"
+
+namespace net {
+
+class IPPattern::ComponentPattern {
+ public:
+  explicit ComponentPattern(ComponentPattern* next);
+  void AppendRange(uint32 min, uint32 max);
+  bool Match(uint32 value) const;
+  ComponentPattern* next() const { return next_; }
+  ComponentPattern* Reverse(ComponentPattern* previous);

[wtc, 2014/02/11 23:52:30]

At least Reverse should be documented. (The other methods are somewhat
self-explanatory.)

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+ private:
+  struct Range {
+   public:
+    Range(uint32 min, uint32 max) : minimum(min), maximum(max) {}
+    uint32 minimum;
+    uint32 maximum;
+  };
+  typedef std::vector<Range> RangeVector;
+  RangeVector ranges_;
+  ComponentPattern* next_;
+  DISALLOW_COPY_AND_ASSIGN(ComponentPattern);

[wtc, 2014/02/11 23:52:30]

Nit: you should add more blank lines. For example, a blank line before the
beginning of "private:", and a blank line to separate the typedef from data
members.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+};
+
+

[wtc, 2014/02/11 23:52:30]

Nit: delete one blank line.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+IPPattern::ComponentPattern::ComponentPattern(ComponentPattern* next)
+    : next_(next) {
+}
+
+void IPPattern::ComponentPattern::AppendRange(uint32 min, uint32 max) {
+  ranges_.push_back(Range(min, max));
+}
+
+bool IPPattern::ComponentPattern::Match(uint32 value) const {
+  // Simple linear search should be fine, as we usually only have very few
+  // distinct ranges to test.
+  for (RangeVector::const_iterator range_it = ranges_.begin();
+       range_it != ranges_.end(); ++range_it) {
+    if (range_it->maximum >= value && range_it->minimum <= value) {
+      return true;
+    }

[wtc, 2014/02/11 23:52:30]

Nit: we usually omit curly braces for one-line if statement bodies, except for
QUIC code.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  }
+  return false;
+}
+
+IPPattern::ComponentPattern* IPPattern::ComponentPattern::Reverse(
+  ComponentPattern* previous) {

[wtc, 2014/02/11 23:52:30]

Nit: the function parameter should be indented by four.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  if (!next_) {
+    next_ = previous;
+    return this;
+  }
+  ComponentPattern* start = next_->Reverse(this);
+  next_ = previous;
+  return start;
+}
+
+IPPattern::IPPattern() : is_ipv4_(true), first_component_pattern_(NULL) {}
+
+IPPattern::~IPPattern() {
+  while (first_component_pattern_ != NULL) {
+    ComponentPattern* component_pattern = first_component_pattern_;
+    first_component_pattern_ = first_component_pattern_->next();
+    delete component_pattern;
+  }
+}
+
+bool IPPattern::Match(const IPAddressNumber& address) const {
+  if (ip_mask_.empty()) {

[wtc, 2014/02/11 23:52:30]

It seems that ip_mask_ should not be empty. ip_mask_ should always be of size 4
or 8.

[jar (doing other things), 2014/02/15 21:14:56]

Rather than DCHECKing, it seemed nice to insist that until a pattern is loaded
(parsed into place), the instance "does not match" anything.

If you'd really rather, I could change this, and change the associated test
case.

+    return false;
+  }
+  bool address_is_ipv4 = address.size() == kIPv4AddressSize;
+  if (address_is_ipv4 != is_ipv4_) {
+    return false;
+  }
+
+  ComponentPattern* next_pattern = first_component_pattern_;

[wtc, 2014/02/11 23:52:30]

Nit: name this variable "pattern".

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  int fixed_value_index = 0;
+  // IPv6 |address| vectors have 16 pieces, while our  |ip_mask_| has only
+  // 8, so it is easier to count separately.
+  int address_index = 0;
+  for (size_t i = 0; i < ip_mask_.size(); ++i) {
+    uint32 value_to_test = address[address_index++];
+    if (!is_ipv4_) {
+      value_to_test = (value_to_test << 8) + address[address_index++];
+    }
+    if (ip_mask_[i]) {
+      if (component_values_[fixed_value_index++] != value_to_test) {
+        return false;
+      }
+      continue;
+    }
+    if (!next_pattern->Match(value_to_test)) {
+      return false;
+    }
+    next_pattern = next_pattern->next();
+  }
+  return true;
+}
+
+bool IPPattern::ParsePattern(const std::string& ip_pattern) {
+  DCHECK(ip_mask_.empty());
+  if (ip_pattern.find(':') != std::string::npos) {
+    is_ipv4_ = false;
+  }
+  Strings components;
+  base::SplitString(ip_pattern, is_ipv4_ ? '.' : ':', &components);
+  if (components.size() != (is_ipv4_ ? 4u : 8u)) {
+    DVLOG(1) << "Invalid component count: " << ip_pattern;
+    return false;
+  }
+  for (Strings::iterator component_it = components.begin();
+       component_it != components.end(); ++component_it) {
+    if (component_it->empty()) {
+      DVLOG(1) << "Empty component: " << ip_pattern;
+      return false;
+    }
+    if (*component_it == "*") {
+      // Let standard code handle this below.
+      *component_it = is_ipv4_ ? "[0-255]" : "[0-FFFF]";
+    } else if ((*component_it)[0] != '[') {
+      // This value will just have a specific integer to match.
+      uint32 value;
+      if (!ValueTextToInt(*component_it, &value)) {
+        return false;
+      }
+      ip_mask_.push_back(true);
+      component_values_.push_back(value);
+      continue;
+    }
+    if ((*component_it)[component_it->size() - 1] != ']') {

[wtc, 2014/02/11 23:52:30]

This can be component_it->back().

Similarly, on line 129, *component_it)[0] can be component_it->front(), but I
think back() is more useful.

[jar (doing other things), 2014/02/15 21:14:56]

Much nicer! thanks.

Done.

...but then... the try bot for patch set 12 blew up with:
../../net/base/ip_pattern.cc: In member function 'bool
net::IPPattern::ParsePattern(const string&)':
../../net/base/ip_pattern.cc:108:30: error: 'struct std::basic_string<char>' has
no member named 'front'
../../net/base/ip_pattern.cc:117:23: error: 'struct std::basic_string<char>' has
no member named 'back'

and I had to go back to the original form.

[wtc, 2014/02/19 00:24:11]

On 2014/02/15 21:14:56, jar wrote:
[...]
Sorry. front() and back() are only available in C++11.

+      DVLOG(1) << "Missing close bracket: " << ip_pattern;
+      return false;
+    }
+    // Now we know the size() is at least 2.
+    if (component_it->size() == 2) {
+      DVLOG(1) << "Empty bracket: " << ip_pattern;
+      return false;
+    }
+    //  We'll need a pattern to match this bracketed component.

[wtc, 2014/02/11 23:52:30]

Nit: two spaces before "We'll".

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+    // Anticipate that it will probably link to (be pushed in front of) the
+    // existing first_component_pattern_, if we parse the pattern.
+    scoped_ptr<ComponentPattern> component_pattern(
+        new ComponentPattern(first_component_pattern_));
+    // Trim leading and trailing bracket before calling for parsing.
+    if (!ParseComponentPattern(component_it->substr(

[wtc, 2014/02/11 23:52:30]

If you change ParseComponentPattern to take a base::StringPiece, then you don't
need to modify the std::string.

[jar (doing other things), 2014/02/15 21:14:56]

Done.... but we end up converting it std::string in order to do a SplitString in
the parse method.

+        1, component_it->size() - 2), component_pattern.get())) {
+      return false;
+    }
+    ip_mask_.push_back(false);
+    // Push onto list.
+    DCHECK_EQ(first_component_pattern_, component_pattern->next());
+    first_component_pattern_ = component_pattern.release();
+  }
+  // The list starting with first_component_pattern_ is now backwards from what
+  // is needed to process a match, so we need to reverse it.

[wtc, 2014/02/11 23:52:30]

If you maintain a last_component_pattern pointer as you add component patterns
to the list, then you don't need to reverse the list.

[jar (doing other things), 2014/02/15 21:14:56]

Transitioned to std::vector<pointers>

+  if (first_component_pattern_) {
+    first_component_pattern_ = first_component_pattern_->Reverse(NULL);
+  }
+  return true;
+}
+
+bool IPPattern::ParseComponentPattern(const std::string& text,
+                                      ComponentPattern* pattern) const {
+  // We're given a comma separated set of ranges, some of which may be simple
+  // constants.
+  Strings ranges;
+  base::SplitString(text, ',', &ranges);
+  for (Strings::iterator range_it = ranges.begin();
+        range_it != ranges.end(); ++range_it) {
+    base::StringTokenizer range_pair(*range_it, "-");
+    uint32 min = 0;
+    range_pair.GetNext();
+    if (!ValueTextToInt(range_pair.token(), &min)) {
+      return false;
+    }
+    uint32 max = min;  // Sometimes we have no distinct max.
+    if (range_pair.GetNext()) {
+      if (!ValueTextToInt(range_pair.token(), &max)) {
+        return false;
+      }
+    }
+    if (range_pair.GetNext()) {
+      // Too many "-" in this range specifier.
+      DVLOG(1) << "Too many hyphens in range: ";
+      return false;
+    }
+    pattern->AppendRange(min, max);

[wtc, 2014/02/11 23:52:30]

Is it OK to append some ranges, and then fail and return false?

[jar (doing other things), 2014/02/15 21:14:56]

The caller discards the partially constructed pattern, but the pattern (in the
incomplete form) is fully functional.  Clearing it out would leave a similarly
odd instance, which would also "function" to some extent (never matching
anything).  The current returned state seems reasonable.

+  }
+  return true;
+}
+
+bool IPPattern::ValueTextToInt(const base::StringPiece& input,
+                               uint32* output) const {
+  bool ok = is_ipv4_ ? base::StringToUint(input, output)
+                     : base::HexStringToUInt(input, output);

[wtc, 2014/02/11 23:52:30]

Nit: we usually put an operator (such as ":") at the end of the previous line. I
am not sure if this is recommended by the Style Guide though.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+  if (is_ipv4_ && *output > 255u) {
+    DVLOG(1) << "IPv4 component greater than 255";

[wtc, 2014/02/11 23:52:30]

For IPv6, you should also verify that *output <= 0xFFFF.

[jar (doing other things), 2014/02/15 21:14:56]

Done.

+    return false;
+  }
+  if (!ok) {
+    DVLOG(1) << "Could not convert value to number: " << input;
+  }

[wtc, 2014/02/11 23:52:30]

I think we should first check !ok, then check the is_ipv4_ && *output > 255u
case. If base::StringToUint returns false, we should not assume we can use the
output.

[jar (doing other things), 2014/02/15 21:14:56]

Good catch!

Done.

+  return ok;
+}
+
+}  // namespace net