shill: connection: Fix IP address comparison in FixGatewayReachability
CL:247030 inadvertently broke IP address comparison for
Connection::FixGatewayReachability, since now the |trusted_ip|
contains a prefix while the |peer| and |gateway| still do not.
To solve this, add a new comparison method for IPAddress objects
that does not consider the prefix, and use this for address
comparisons in Connection::FixGatewayReachability.
BUG=chromium:478267
TEST=Unit test + create an ipvanish account and connect
Change-Id: Id30e0cd15bb0171c7f438cf1e7f0087c75d55611
Originally-Reviewed-on: https://chromium-review.googlesource.com/268970
(cherry picked from commit 08e9305b5ffaccd5885f871d76a17b33dbcc474b)
Reviewed-on: https://chromium-review.googlesource.com/269164
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/shill/connection.cc b/shill/connection.cc
index d6529ae..add970e 100644
--- a/shill/connection.cc
+++ b/shill/connection.cc
@@ -32,6 +32,8 @@
namespace Logging {
static auto kModuleLogScope = ScopeLogger::kConnection;
static string ObjectID(Connection *c) {
+ if (c == nullptr)
+ return "(connection)";
return c->interface_name();
}
}
@@ -395,20 +397,25 @@
IPAddress *peer,
IPAddress *gateway,
const IPAddress &trusted_ip) {
+ SLOG(nullptr, 2) << __func__
+ << " local " << local->ToString()
+ << ", peer " << peer->ToString()
+ << ", gateway " << gateway->ToString()
+ << ", trusted_ip " << trusted_ip.ToString();
if (!gateway->IsValid()) {
LOG(WARNING) << "No gateway address was provided for this connection.";
return false;
}
if (peer->IsValid()) {
- if (!gateway->Equals(*peer)) {
+ if (!gateway->HasSameAddressAs(*peer)) {
LOG(WARNING) << "Gateway address "
<< gateway->ToString()
<< " does not match peer address "
<< peer->ToString();
return false;
}
- if (gateway->Equals(trusted_ip)) {
+ if (gateway->HasSameAddressAs(trusted_ip)) {
// In order to send outgoing traffic in a point-to-point network,
// the gateway IP address isn't of significance. As opposed to
// broadcast networks, we never ARP for the gateway IP address,
diff --git a/shill/connection_unittest.cc b/shill/connection_unittest.cc
index f163f9a..506f9c9 100644
--- a/shill/connection_unittest.cc
+++ b/shill/connection_unittest.cc
@@ -867,7 +867,8 @@
// should be modified to allow routing to work correctly.
ASSERT_TRUE(gateway.SetAddressFromString(kUnreachableGateway));
ASSERT_TRUE(peer.SetAddressFromString(kUnreachableGateway));
- ASSERT_TRUE(trusted_ip.SetAddressFromString(kUnreachableGateway));
+ ASSERT_TRUE(trusted_ip.SetAddressAndPrefixFromString(
+ string(kUnreachableGateway) + "/32"));
EXPECT_TRUE(Connection::FixGatewayReachability(
&local, &peer, &gateway, trusted_ip));
EXPECT_TRUE(peer.IsDefault());
diff --git a/shill/net/ip_address.cc b/shill/net/ip_address.cc
index 035daa0..f5ff240 100644
--- a/shill/net/ip_address.cc
+++ b/shill/net/ip_address.cc
@@ -222,6 +222,15 @@
return out;
}
+bool IPAddress::Equals(const IPAddress &b) const {
+ return family_ == b.family_ && address_.Equals(b.address_) &&
+ prefix_ == b.prefix_;
+}
+
+bool IPAddress::HasSameAddressAs(const IPAddress &b) const {
+ return family_ == b.family_ && address_.Equals(b.address_);
+}
+
IPAddress IPAddress::MaskWith(const IPAddress &b) const {
CHECK(IsValid());
CHECK(b.IsValid());
diff --git a/shill/net/ip_address.h b/shill/net/ip_address.h
index 927544a..464902f 100644
--- a/shill/net/ip_address.h
+++ b/shill/net/ip_address.h
@@ -102,10 +102,11 @@
// Similar to IntoString, but returns by value. Convenient for logging.
std::string ToString() const;
- bool Equals(const IPAddress &b) const {
- return family_ == b.family_ && address_.Equals(b.address_) &&
- prefix_ == b.prefix_;
- }
+ // Returns whether |b| has the same family, address and prefix as |this|.
+ bool Equals(const IPAddress &b) const;
+
+ // Returns whether |b| has the same family and address as |this|.
+ bool HasSameAddressAs(const IPAddress &b) const;
// Perform an AND operation between the address data of |this| and that
// of |b|. Returns an IPAddress containing the result of the operation.
diff --git a/shill/net/ip_address_unittest.cc b/shill/net/ip_address_unittest.cc
index bc6f75e..234e609 100644
--- a/shill/net/ip_address_unittest.cc
+++ b/shill/net/ip_address_unittest.cc
@@ -147,6 +147,21 @@
EXPECT_TRUE(kAddress1.Equals(address.address()));
}
+TEST_F(IPAddressTest, HasSameAddressAs) {
+ const string kString1(kV4String1);
+ IPAddress address0(IPAddress::kFamilyIPv4);
+ EXPECT_TRUE(address0.SetAddressAndPrefixFromString(kString1 + "/0"));
+ IPAddress address1(IPAddress::kFamilyIPv4);
+ EXPECT_TRUE(address1.SetAddressAndPrefixFromString(kString1 + "/10"));
+ IPAddress address2(IPAddress::kFamilyIPv4);
+ EXPECT_TRUE(address2.SetAddressAndPrefixFromString(kString1 + "/0"));
+
+ EXPECT_FALSE(address0.Equals(address1));
+ EXPECT_TRUE(address0.Equals(address2));
+ EXPECT_TRUE(address0.HasSameAddressAs(address1));
+ EXPECT_TRUE(address0.HasSameAddressAs(address2));
+}
+
struct PrefixMapping {
PrefixMapping() : family(IPAddress::kFamilyUnknown), prefix(0) {}
PrefixMapping(IPAddress::Family family_in,
