Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net: save the network type explicitly in CNetAddr #19534

Merged
merged 2 commits into from
Jul 29, 2020
Merged

net: save the network type explicitly in CNetAddr #19534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
100c64a
net: document `enum Network`
vasild May 14, 2020
bcfebb6
net: save the network type explicitly in CNetAddr
vasild May 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 43 additions & 36 deletions src/netaddress.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,35 @@ CNetAddr::CNetAddr()

void CNetAddr::SetIP(const CNetAddr& ipIn)
{
m_net = ipIn.m_net;
memcpy(ip, ipIn.ip, sizeof(ip));
}

void CNetAddr::SetLegacyIPv6(const uint8_t ipv6[16])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to pass around fixed-length data with a raw data pointer (of arbitrary length) when type safe alternatives like span or std::array (probably not applicable in this case) are available?

Copy link
Member

@laanwj laanwj Jul 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input has to be 16 bytes, in this case. Is there a way to enforce this in the type, for spans?
(sure, the [16] doesn't do anything here either except as a signal to the programmer)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but I presumed spans could be of non-dynamic extent as well. Does this not work?

const std::span<uint8_t, 16>& ipv6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in this case the [16] is just a "signal to the programmer". In SetRaw() we have a bare pointer const uint8_t* when calling this function.

I have removed SetRaw() in the subsequent commit and will revisit this code.

Some experiments:

int f1(const std::span<int, 3>& p)
{
    return p[5]; // no warning
}

int f2(const int (&p)[3])
{
    return p[5]; // warning: array index 5 is past the end of the array (which contains 3 elements) [-Warray-bounds]
}

int f3(const std::array<int, 3>& p)
{
    return p[5]; // no warning
}

int main(int, char**) {
    int a[3];
    int b[2];

    f1(a);
    f1(b); // error: no known conversion from 'int [2]' to 'const std::span<int, 3>'

    f2(a);
    f2(b); // error: no known conversion from 'int [2]' to 'const int [3]'

    f3(std::array<int, 3>{a[0], a[1], a[2]});

    return 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in #19628 I changed the argument to const uint8_t (&ipv6)[ADDR_IPv6_SIZE] mostly to make the interface clean. From 3 callers 2 need an abusive typecasts because they don't have an array (one has bare pointer (the fuzz tests) and the other has struct in6_addr).

Feel free to comment there: https://github.com/bitcoin/bitcoin/pull/19628/files#diff-b64569708508232923e5fe3059396334R28

{
if (memcmp(ipv6, pchIPv4, sizeof(pchIPv4)) == 0) {
m_net = NET_IPV4;
} else if (memcmp(ipv6, pchOnionCat, sizeof(pchOnionCat)) == 0) {
m_net = NET_ONION;
} else if (memcmp(ipv6, g_internal_prefix, sizeof(g_internal_prefix)) == 0) {
m_net = NET_INTERNAL;
} else {
m_net = NET_IPV6;
}
memcpy(ip, ipv6, 16);
}

void CNetAddr::SetRaw(Network network, const uint8_t *ip_in)
{
switch(network)
{
case NET_IPV4:
m_net = NET_IPV4;
memcpy(ip, pchIPv4, 12);
memcpy(ip+12, ip_in, 4);
break;
case NET_IPV6:
memcpy(ip, ip_in, 16);
SetLegacyIPv6(ip_in);
break;
default:
assert(!"invalid network");
Expand All @@ -66,6 +82,7 @@ bool CNetAddr::SetInternal(const std::string &name)
if (name.empty()) {
return false;
}
m_net = NET_INTERNAL;
unsigned char hash[32] = {};
CSHA256().Write((const unsigned char*)name.data(), name.size()).Finalize(hash);
memcpy(ip, g_internal_prefix, sizeof(g_internal_prefix));
Expand All @@ -89,6 +106,7 @@ bool CNetAddr::SetSpecial(const std::string &strName)
std::vector<unsigned char> vchAddr = DecodeBase32(strName.substr(0, strName.size() - 6).c_str());
if (vchAddr.size() != 16-sizeof(pchOnionCat))
return false;
m_net = NET_ONION;
memcpy(ip, pchOnionCat, sizeof(pchOnionCat));
for (unsigned int i=0; i<16-sizeof(pchOnionCat); i++)
ip[i + sizeof(pchOnionCat)] = vchAddr[i];
Expand Down Expand Up @@ -123,15 +141,9 @@ bool CNetAddr::IsBindAny() const
return true;
}

bool CNetAddr::IsIPv4() const
{
return (memcmp(ip, pchIPv4, sizeof(pchIPv4)) == 0);
}
bool CNetAddr::IsIPv4() const { return m_net == NET_IPV4; }

bool CNetAddr::IsIPv6() const
{
return (!IsIPv4() && !IsTor() && !IsInternal());
}
bool CNetAddr::IsIPv6() const { return m_net == NET_IPV6; }

bool CNetAddr::IsRFC1918() const
{
Expand Down Expand Up @@ -165,50 +177,54 @@ bool CNetAddr::IsRFC5737() const

bool CNetAddr::IsRFC3849() const
{
return GetByte(15) == 0x20 && GetByte(14) == 0x01 && GetByte(13) == 0x0D && GetByte(12) == 0xB8;
return IsIPv6() && GetByte(15) == 0x20 && GetByte(14) == 0x01 &&
GetByte(13) == 0x0D && GetByte(12) == 0xB8;
}

bool CNetAddr::IsRFC3964() const
{
return (GetByte(15) == 0x20 && GetByte(14) == 0x02);
return IsIPv6() && GetByte(15) == 0x20 && GetByte(14) == 0x02;
}

bool CNetAddr::IsRFC6052() const
{
static const unsigned char pchRFC6052[] = {0,0x64,0xFF,0x9B,0,0,0,0,0,0,0,0};
return (memcmp(ip, pchRFC6052, sizeof(pchRFC6052)) == 0);
return IsIPv6() && memcmp(ip, pchRFC6052, sizeof(pchRFC6052)) == 0;
}

bool CNetAddr::IsRFC4380() const
{
return (GetByte(15) == 0x20 && GetByte(14) == 0x01 && GetByte(13) == 0 && GetByte(12) == 0);
return IsIPv6() && GetByte(15) == 0x20 && GetByte(14) == 0x01 && GetByte(13) == 0 &&
GetByte(12) == 0;
}

bool CNetAddr::IsRFC4862() const
{
static const unsigned char pchRFC4862[] = {0xFE,0x80,0,0,0,0,0,0};
return (memcmp(ip, pchRFC4862, sizeof(pchRFC4862)) == 0);
return IsIPv6() && memcmp(ip, pchRFC4862, sizeof(pchRFC4862)) == 0;
}

bool CNetAddr::IsRFC4193() const
{
return ((GetByte(15) & 0xFE) == 0xFC);
return IsIPv6() && (GetByte(15) & 0xFE) == 0xFC;
}

bool CNetAddr::IsRFC6145() const
{
static const unsigned char pchRFC6145[] = {0,0,0,0,0,0,0,0,0xFF,0xFF,0,0};
return (memcmp(ip, pchRFC6145, sizeof(pchRFC6145)) == 0);
return IsIPv6() && memcmp(ip, pchRFC6145, sizeof(pchRFC6145)) == 0;
}

bool CNetAddr::IsRFC4843() const
{
return (GetByte(15) == 0x20 && GetByte(14) == 0x01 && GetByte(13) == 0x00 && (GetByte(12) & 0xF0) == 0x10);
return IsIPv6() && GetByte(15) == 0x20 && GetByte(14) == 0x01 &&
GetByte(13) == 0x00 && (GetByte(12) & 0xF0) == 0x10;
}

bool CNetAddr::IsRFC7343() const
{
return (GetByte(15) == 0x20 && GetByte(14) == 0x01 && GetByte(13) == 0x00 && (GetByte(12) & 0xF0) == 0x20);
return IsIPv6() && GetByte(15) == 0x20 && GetByte(14) == 0x01 &&
GetByte(13) == 0x00 && (GetByte(12) & 0xF0) == 0x20;
}

bool CNetAddr::IsHeNet() const
Expand All @@ -222,10 +238,7 @@ bool CNetAddr::IsHeNet() const
*
* @see CNetAddr::SetSpecial(const std::string &)
*/
bool CNetAddr::IsTor() const
{
return (memcmp(ip, pchOnionCat, sizeof(pchOnionCat)) == 0);
}
bool CNetAddr::IsTor() const { return m_net == NET_ONION; }

bool CNetAddr::IsLocal() const
{
Expand All @@ -235,7 +248,7 @@ bool CNetAddr::IsLocal() const

// IPv6 loopback (::1/128)
static const unsigned char pchLocal[16] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1};
if (memcmp(ip, pchLocal, 16) == 0)
if (IsIPv6() && memcmp(ip, pchLocal, 16) == 0)
return true;
Comment on lines -238 to 252
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general some of this might be optimized out by the compiler,
And i would prefer for the transition period something like this that we can refactor out when all works fine
to an compiler and bytecode friendly version.

I would prefer constructions for all those ipv6 overrides in the rest of the code to look like this
suggested change:

if  (memcmp(ip, pchLocal, 16) == 0) && IsIPv6();

And some logging here and in the other overrides i.e.

LogPrint(BCLog::ADDV2, "old isXXXX() says so, ADDRv2 m_net did override with  value = xxx .....


return false;
Expand All @@ -259,12 +272,12 @@ bool CNetAddr::IsValid() const
// header20 vectorlen3 addr26 addr26 addr26 header20 vectorlen3 addr26 addr26 addr26...
// so if the first length field is garbled, it reads the second batch
// of addr misaligned by 3 bytes.
if (memcmp(ip, pchIPv4+3, sizeof(pchIPv4)-3) == 0)
if (IsIPv6() && memcmp(ip, pchIPv4+3, sizeof(pchIPv4)-3) == 0)
return false;

// unspecified IPv6 address (::/128)
unsigned char ipNone6[16] = {};
if (memcmp(ip, ipNone6, 16) == 0)
if (IsIPv6() && memcmp(ip, ipNone6, 16) == 0)
return false;

// documentation IPv6 address
Expand Down Expand Up @@ -311,7 +324,7 @@ bool CNetAddr::IsRoutable() const
*/
bool CNetAddr::IsInternal() const
{
return memcmp(ip, g_internal_prefix, sizeof(g_internal_prefix)) == 0;
return m_net == NET_INTERNAL;
}

enum Network CNetAddr::GetNetwork() const
Expand All @@ -322,13 +335,7 @@ enum Network CNetAddr::GetNetwork() const
if (!IsRoutable())
return NET_UNROUTABLE;

if (IsIPv4())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this code still needed in GetNetwork()?

    if (IsInternal())
        return NET_INTERNAL;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question - no. But it was not necessary before this PR either. Some good simplification can be done here even on master. I will see whether it is better to add a new commit to this PR or file it as a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction - it is needed, otherwise this test would fail:

https://github.com/bitcoin/bitcoin/blob/476436b/src/test/netbase_tests.cpp#L45

Anyway, I made some simplifications on top of this PR that remove the "extending" of enum Network with NET_UNKNOWN and NET_TEREDO and also removed GetExtNetwork(). To avoid distractions, I will not append those commits to this PR but would rather open a new PR against master, once this PR gets merged.

Copy link
Contributor

@jonatack jonatack Jul 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vasild, looking forward to reviewing the simplifications.

return NET_IPV4;

if (IsTor())
return NET_ONION;

return NET_IPV6;
return m_net;
}

std::string CNetAddr::ToStringIP() const
Expand Down Expand Up @@ -362,12 +369,12 @@ std::string CNetAddr::ToString() const

bool operator==(const CNetAddr& a, const CNetAddr& b)
{
return (memcmp(a.ip, b.ip, 16) == 0);
return a.m_net == b.m_net && memcmp(a.ip, b.ip, 16) == 0;
}

bool operator<(const CNetAddr& a, const CNetAddr& b)
{
return (memcmp(a.ip, b.ip, 16) < 0);
return a.m_net < b.m_net || (a.m_net == b.m_net && memcmp(a.ip, b.ip, 16) < 0);
}

/**
Expand Down Expand Up @@ -813,7 +820,7 @@ CSubNet::CSubNet(const CNetAddr &addr):
*/
bool CSubNet::Match(const CNetAddr &addr) const
{
if (!valid || !addr.IsValid())
if (!valid || !addr.IsValid() || network.m_net != addr.m_net)
return false;
for(int x=0; x<16; ++x)
if ((addr.ip[x] & netmask[x]) != network.ip[x])
Expand Down
61 changes: 59 additions & 2 deletions src/netaddress.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,50 @@
#include <string>
#include <vector>

/**
* A network type.
* @note An address may belong to more than one network, for example `10.0.0.1`
* belongs to both `NET_UNROUTABLE` and `NET_IPV4`.
* Keep these sequential starting from 0 and `NET_MAX` as the last entry.
* We have loops like `for (int i = 0; i < NET_MAX; i++)` that expect to iterate
* over all enum values and also `GetExtNetwork()` "extends" this enum by
* introducing standalone constants starting from `NET_MAX`.
*/
enum Network
{
/// Addresses from these networks are not publicly routable on the global Internet.
NET_UNROUTABLE = 0,

/// IPv4
NET_IPV4,

/// IPv6
NET_IPV6,

/// TORv2
NET_ONION,

/// A set of dummy addresses that map a name to an IPv6 address. These
vasild marked this conversation as resolved.
Show resolved Hide resolved
/// addresses belong to RFC4193's fc00::/7 subnet (unique-local addresses).
/// We use them to map a string or FQDN to an IPv6 address in CAddrMan to
/// keep track of which DNS seeds were used.
NET_INTERNAL,

/// Dummy value to indicate the number of NET_* constants.
NET_MAX,
};

/** IP address (IPv6, or IPv4 using mapped IPv6 range (::FFFF:0:0/96)) */
/**
* Network address.
*/
class CNetAddr
{
protected:
/**
* Network to which this address belongs.
*/
Network m_net{NET_IPV6};

unsigned char ip[16]; // in network byte order
uint32_t scopeId{0}; // for scoped/link-local ipv6 addresses

Expand All @@ -39,6 +68,14 @@ class CNetAddr
explicit CNetAddr(const struct in_addr& ipv4Addr);
void SetIP(const CNetAddr& ip);

/**
* Set from a legacy IPv6 address.
* Legacy IPv6 address may be a normal IPv6 address, or another address
* (e.g. IPv4) disguised as IPv6. This encoding is used in the legacy
* `addr` encoding.
*/
void SetLegacyIPv6(const uint8_t ipv6[16]);

/**
* Set raw IPv4 or IPv6 address (in network byte order)
* @note Only NET_IPV4 and NET_IPV6 are allowed for network.
Expand Down Expand Up @@ -100,7 +137,27 @@ class CNetAddr
friend bool operator!=(const CNetAddr& a, const CNetAddr& b) { return !(a == b); }
friend bool operator<(const CNetAddr& a, const CNetAddr& b);

SERIALIZE_METHODS(CNetAddr, obj) { READWRITE(obj.ip); }
/**
* Serialize to a stream.
*/
template <typename Stream>
void Serialize(Stream& s) const
{
s << ip;
}

/**
* Unserialize from a stream.
*/
template <typename Stream>
void Unserialize(Stream& s)
{
unsigned char ip_temp[sizeof(ip)];
s >> ip_temp;
// Use SetLegacyIPv6() so that m_net is set correctly. For example
// ::FFFF:0102:0304 should be set as m_net=NET_IPV4 (1.2.3.4).
SetLegacyIPv6(ip_temp);
}

friend class CSubNet;
};
Expand Down
13 changes: 11 additions & 2 deletions src/test/netbase_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,14 @@ BOOST_AUTO_TEST_CASE(onioncat_test)

}

BOOST_AUTO_TEST_CASE(embedded_test)
{
CNetAddr addr1(ResolveIP("1.2.3.4"));
CNetAddr addr2(ResolveIP("::FFFF:0102:0304"));
BOOST_CHECK(addr2.IsIPv4());
BOOST_CHECK_EQUAL(addr1.ToString(), addr2.ToString());
}

BOOST_AUTO_TEST_CASE(subnet_test)
{

Expand All @@ -158,12 +166,13 @@ BOOST_AUTO_TEST_CASE(subnet_test)
BOOST_CHECK(ResolveSubNet("1.2.2.1/24").Match(ResolveIP("1.2.2.4")));
BOOST_CHECK(ResolveSubNet("1.2.2.110/31").Match(ResolveIP("1.2.2.111")));
BOOST_CHECK(ResolveSubNet("1.2.2.20/26").Match(ResolveIP("1.2.2.63")));
// All-Matching IPv6 Matches arbitrary IPv4 and IPv6
// All-Matching IPv6 Matches arbitrary IPv6
BOOST_CHECK(ResolveSubNet("::/0").Match(ResolveIP("1:2:3:4:5:6:7:1234")));
// But not `::` or `0.0.0.0` because they are considered invalid addresses
BOOST_CHECK(!ResolveSubNet("::/0").Match(ResolveIP("::")));
BOOST_CHECK(!ResolveSubNet("::/0").Match(ResolveIP("0.0.0.0")));
BOOST_CHECK(ResolveSubNet("::/0").Match(ResolveIP("1.2.3.4")));
// Addresses from one network (IPv4) don't belong to subnets of another network (IPv6)
BOOST_CHECK(!ResolveSubNet("::/0").Match(ResolveIP("1.2.3.4")));
// All-Matching IPv4 does not Match IPv6
BOOST_CHECK(!ResolveSubNet("0.0.0.0/0").Match(ResolveIP("1:2:3:4:5:6:7:1234")));
// Invalid subnets Match nothing (not even invalid addresses)
Expand Down