Avoid heap allocations in IPAddress

net/base/ip_address.cc

Index: net/base/ip_address.cc
diff --git a/net/base/ip_address.cc b/net/base/ip_address.cc
index cf18650fd7c51480455dabc0e8aa3b1f0e51b735..20974bdebff6c1df26bfec507e6b961b26d5d3e7 100644
--- a/net/base/ip_address.cc
+++ b/net/base/ip_address.cc
@@ -22,7 +22,7 @@ const uint8_t kIPv4MappedPrefix[] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xFF, 0xFF};
 // Note that this function assumes:
 // * |ip_address| is at least |prefix_length_in_bits| (bits) long;
 // * |ip_prefix| is at least |prefix_length_in_bits| (bits) long.
-bool IPAddressPrefixCheck(const std::vector<uint8_t>& ip_address,
+bool IPAddressPrefixCheck(const net::IPAddress::IPAddressBytes& ip_address,
                           const uint8_t* ip_prefix,
                           size_t prefix_length_in_bits) {
   // Compare all the bytes that fall entirely within the prefix.
@@ -51,7 +51,7 @@ bool IPAddressPrefixCheck(const std::vector<uint8_t>& ip_address,
 // www.iana.org/assignments/ipv4-address-space/ipv4-address-space.xhtml
 // www.iana.org/assignments/iana-ipv4-special-registry/
 // iana-ipv4-special-registry.xhtml
-bool IsReservedIPv4(const std::vector<uint8_t>& ip_address) {
+bool IsReservedIPv4(const net::IPAddress::IPAddressBytes& ip_address) {
   // Different IP versions have different range reservations.
   DCHECK_EQ(net::IPAddress::kIPv4AddressSize, ip_address.size());
   struct {
@@ -79,7 +79,7 @@ bool IsReservedIPv4(const std::vector<uint8_t>& ip_address) {
 // addresses outside these ranges are reserved.
 // Sources for info:
 // www.iana.org/assignments/ipv6-address-space/ipv6-address-space.xhtml
-bool IsReservedIPv6(const std::vector<uint8_t>& ip_address) {
+bool IsReservedIPv6(const net::IPAddress::IPAddressBytes& ip_address) {
   // Different IP versions have different range reservations.
   DCHECK_EQ(net::IPAddress::kIPv6AddressSize, ip_address.size());
   struct {
@@ -103,7 +103,7 @@ bool IsReservedIPv6(const std::vector<uint8_t>& ip_address) {
 }
 
 bool ParseIPLiteralToBytes(const base::StringPiece& ip_literal,
-                           std::vector<uint8_t>* bytes) {
+                           net::IPAddress::IPAddressBytes* bytes) {
   // |ip_literal| could be either an IPv4 or an IPv6 literal. If it contains
   // a colon however, it must be an IPv6 address.
   if (ip_literal.find(':') != base::StringPiece::npos) {
@@ -132,18 +132,65 @@ bool ParseIPLiteralToBytes(const base::StringPiece& ip_literal,
 
 namespace net {
 
+IPAddress::IPAddressBytes::IPAddressBytes() : size_(0) {}
+
+net::IPAddress::IPAddressBytes::~IPAddressBytes() {}
+net::IPAddress::IPAddressBytes::IPAddressBytes(
+    net::IPAddress::IPAddressBytes const& other) = default;
+
+bool operator<(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs) {

[eroman, 2017/05/12 23:25:04]

std::array<> already defines these operators (as lexicographic compares, which
is what we want), so you can just forward to them. As in:

  return lhs < rhs;
  return lhs > rhs;
  return lhs == rhs;

[Ryan Hamilton, 2017/05/13 13:20:46]

Ah! So I implemented this initially, and while it compiles, it's subtly wrong.
(There's even a test which fails). The std::array operators look at all the
elements in the array, but IPAddressByte operators only look at the elements in
[0, size_). 

[eroman, 2017/05/15 22:15:23]

Glad that there is test coverage to catch this!

+  if (lhs.size_ < rhs.size_)
+    return true;
+  if (lhs.size_ > rhs.size_)
+    return false;
+  for (size_t i = 0; i < lhs.size_; ++i) {
+    if (lhs.bytes_[i] < rhs.bytes_[i])
+      return true;
+  }
+  return false;
+}
+
+bool operator>(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs) {
+  if (lhs.size_ > rhs.size_)
+    return true;
+  if (lhs.size_ < rhs.size_)
+    return false;
+  for (size_t i = 0; i < lhs.size_; ++i) {
+    if (lhs.bytes_[i] > rhs.bytes_[i])
+      return true;
+  }
+  return false;
+}
+
+bool operator==(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs) {
+  if (lhs.size_ != rhs.size_)
+    return false;
+  for (size_t i = 0; i < lhs.size_; ++i) {
+    if (lhs.bytes_[i] != rhs.bytes_[i])
+      return false;
+  }
+  return true;
+}
+
+bool operator!=(IPAddress::IPAddressBytes lhs, IPAddress::IPAddressBytes rhs) {
+  return !(lhs == rhs);
+}
+
 IPAddress::IPAddress() {}
 
-IPAddress::IPAddress(const std::vector<uint8_t>& address)
-    : ip_address_(address) {}
+IPAddress::IPAddress(const std::vector<uint8_t>& address) {

[eroman, 2017/05/12 23:25:05]

I am a bit worried that we have codepaths that may pass variable sized
addresses, that will now result in memory errors.

Am I being overly paranoid? Should we throw in a CHECK() at the entry points
which take variable sized data? (i.e. here and the other ctor).  We can leave
things like push_back() as DCHECK().

[Ryan Hamilton, 2017/05/13 13:20:47]

Fair enough! I added a CHECK() in the IPAddressBytes constructor which both
these call into now.

+  for (uint8_t val : address)
+    ip_address_.push_back(val);

[eroman, 2017/05/12 23:25:05]

may want to do a single memcpy(). Or perhaps code generator is smart enough to
optimize the loop.

[Ryan Hamilton, 2017/05/13 13:20:46]

Done.

+}
 
 IPAddress::IPAddress(const IPAddress& other) = default;
 
-IPAddress::IPAddress(const uint8_t* address, size_t address_len)
-    : ip_address_(address, address + address_len) {}
+IPAddress::IPAddress(const uint8_t* address, size_t address_len) {
+  for (size_t i = 0; i < address_len; ++i)
+    ip_address_.push_back(address[i]);

[eroman, 2017/05/12 23:25:04]

Same comments as above.

[Ryan Hamilton, 2017/05/13 13:20:47]

Done.

+}
 
 IPAddress::IPAddress(uint8_t b0, uint8_t b1, uint8_t b2, uint8_t b3) {
-  ip_address_.reserve(4);
   ip_address_.push_back(b0);
   ip_address_.push_back(b1);
   ip_address_.push_back(b2);
@@ -166,9 +213,22 @@ IPAddress::IPAddress(uint8_t b0,
                      uint8_t b13,
                      uint8_t b14,
                      uint8_t b15) {
-  const uint8_t address[] = {b0, b1, b2,  b3,  b4,  b5,  b6,  b7,
-                             b8, b9, b10, b11, b12, b13, b14, b15};
-  ip_address_ = std::vector<uint8_t>(std::begin(address), std::end(address));
+  ip_address_.push_back(b0);
+  ip_address_.push_back(b1);
+  ip_address_.push_back(b2);
+  ip_address_.push_back(b3);
+  ip_address_.push_back(b4);
+  ip_address_.push_back(b5);
+  ip_address_.push_back(b6);
+  ip_address_.push_back(b7);
+  ip_address_.push_back(b8);
+  ip_address_.push_back(b9);
+  ip_address_.push_back(b10);
+  ip_address_.push_back(b11);
+  ip_address_.push_back(b12);
+  ip_address_.push_back(b13);
+  ip_address_.push_back(b14);
+  ip_address_.push_back(b15);
 }
 
 IPAddress::~IPAddress() {}
@@ -208,7 +268,7 @@ bool IPAddress::IsIPv4MappedIPv6() const {
 }
 
 bool IPAddress::AssignFromIPLiteral(const base::StringPiece& ip_literal) {
-  std::vector<uint8_t> number;
+  IPAddressBytes number;
 
   if (!ParseIPLiteralToBytes(ip_literal, &number))
     return false;
@@ -217,6 +277,10 @@ bool IPAddress::AssignFromIPLiteral(const base::StringPiece& ip_literal) {
   return true;
 }
 
+std::vector<uint8_t> IPAddress::BytesAsVector() const {
+  return std::vector<uint8_t>(ip_address_.begin(), ip_address_.end());
+}
+
 // static
 IPAddress IPAddress::IPv4Localhost() {
   static const uint8_t kLocalhostIPv4[] = {127, 0, 0, 1};