Skip to content

Commit

Permalink
Rewrite IPAddress without union
Browse files Browse the repository at this point in the history 
Using a union allowed undefined behavior for 8-bit integer INPUT and OUTPUT arguments through a 32-bit integer (and vice versa).
Write throught union::uint8_t[16] and read thoutght union::uint32_t[4]
is a undefined behavior.

C++ standard says (https://timsong-cpp.github.io/cppwp/n4659/class.union#1):
"At most one of the non-static data members of an object of union type
can be active at any time, that is, the value of at most one of the
non-static data members can be stored in a union at any time. [ Note:
One special guarantee is made in order to simplify the use of unions:
If a standard-layout union contains several standard-layout structs
that share a common initial sequence, and if a non-static data member
of an object of this standard-layout union type is active and is one
of the standard-layout structs, it is permitted to inspect the common
initial sequence of any of the standard-layout struct members; see
[class.mem].  — end note ]"

and (https://timsong-cpp.github.io/cppwp/n4659/class.mem#22):
"Two standard-layout unions are layout-compatible if they have the same number of non-static data members and corresponding non-static data members (in any order) have layout-compatible types."

you can't hope that compilers don't seem to do undefined behavior at the moment

Also created espressif/arduino-esp32#8829 PR dependent on changing the current API.
  • Loading branch information
@safocl
safocl committed Nov 8, 2023
1 parent fc9a636 commit 52b367a
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 82 deletions.
123 changes: 50 additions & 73 deletions api/IPAddress.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,53 +19,30 @@

#include "IPAddress.h"
#include "Print.h"
#include <algorithm>
#include <cstdint>

using namespace arduino;

IPAddress::IPAddress() : IPAddress(IPv4) {}
IPAddress::IPAddress() = default;

IPAddress::IPAddress(IPType ip_type)
{
_type = ip_type;
memset(_address.bytes, 0, sizeof(_address.bytes));
}
IPAddress::IPAddress(IPType ip_type) : _type(ip_type) {}

IPAddress::IPAddress(uint8_t first_octet, uint8_t second_octet, uint8_t third_octet, uint8_t fourth_octet)
{
_type = IPv4;
memset(_address.bytes, 0, sizeof(_address.bytes));
_address.bytes[IPADDRESS_V4_BYTES_INDEX] = first_octet;
_address.bytes[IPADDRESS_V4_BYTES_INDEX + 1] = second_octet;
_address.bytes[IPADDRESS_V4_BYTES_INDEX + 2] = third_octet;
_address.bytes[IPADDRESS_V4_BYTES_INDEX + 3] = fourth_octet;
_address[IPADDRESS_V4_BYTES_INDEX] = first_octet;
_address[IPADDRESS_V4_BYTES_INDEX + 1] = second_octet;
_address[IPADDRESS_V4_BYTES_INDEX + 2] = third_octet;
_address[IPADDRESS_V4_BYTES_INDEX + 3] = fourth_octet;
}

IPAddress::IPAddress(uint8_t o1, uint8_t o2, uint8_t o3, uint8_t o4, uint8_t o5, uint8_t o6, uint8_t o7, uint8_t o8, uint8_t o9, uint8_t o10, uint8_t o11, uint8_t o12, uint8_t o13, uint8_t o14, uint8_t o15, uint8_t o16) {
_type = IPv6;
_address.bytes[0] = o1;
_address.bytes[1] = o2;
_address.bytes[2] = o3;
_address.bytes[3] = o4;
_address.bytes[4] = o5;
_address.bytes[5] = o6;
_address.bytes[6] = o7;
_address.bytes[7] = o8;
_address.bytes[8] = o9;
_address.bytes[9] = o10;
_address.bytes[10] = o11;
_address.bytes[11] = o12;
_address.bytes[12] = o13;
_address.bytes[13] = o14;
_address.bytes[14] = o15;
_address.bytes[15] = o16;
}
IPAddress::IPAddress(uint8_t o1, uint8_t o2, uint8_t o3, uint8_t o4, uint8_t o5, uint8_t o6, uint8_t o7, uint8_t o8, uint8_t o9, uint8_t o10, uint8_t o11, uint8_t o12, uint8_t o13, uint8_t o14, uint8_t o15, uint8_t o16) : _address{o1, o2, o3, o4, o5, o6, o7, o8, o9, o10, o11, o12, o13, o14, o15, o16}, _type(IPv6) {}

IPAddress::IPAddress(uint32_t address)
// IPv4 only
IPAddress::IPAddress(uint32_t address)
{
// IPv4 only
_type = IPv4;
memset(_address.bytes, 0, sizeof(_address.bytes));
_address.dword[IPADDRESS_V4_DWORD_INDEX] = address;
uint32_t& addressRef = reinterpret_cast<uint32_t&>(_address[IPADDRESS_V4_BYTES_INDEX]);
addressRef = address;

// NOTE on conversion/comparison and uint32_t:
// These conversions are host platform dependent.
Expand All @@ -78,14 +55,12 @@ IPAddress::IPAddress(uint32_t address)

IPAddress::IPAddress(const uint8_t *address) : IPAddress(IPv4, address) {}

IPAddress::IPAddress(IPType ip_type, const uint8_t *address)
IPAddress::IPAddress(IPType ip_type, const uint8_t *address) : _type(ip_type)
{
_type = ip_type;
if (ip_type == IPv4) {
memset(_address.bytes, 0, sizeof(_address.bytes));
memcpy(&_address.bytes[IPADDRESS_V4_BYTES_INDEX], address, sizeof(uint32_t));
std::copy(address, address + 4, &_address[IPADDRESS_V4_BYTES_INDEX]);
} else {
memcpy(_address.bytes, address, sizeof(_address.bytes));
std::copy(address, address + _address.size(), _address.begin());
}
}

Expand All @@ -97,18 +72,18 @@ IPAddress::IPAddress(const char *address)
String IPAddress::toString4() const
{
char szRet[16];
snprintf(szRet, sizeof(szRet), "%u.%u.%u.%u", _address.bytes[IPADDRESS_V4_BYTES_INDEX], _address.bytes[IPADDRESS_V4_BYTES_INDEX + 1], _address.bytes[IPADDRESS_V4_BYTES_INDEX + 2], _address.bytes[IPADDRESS_V4_BYTES_INDEX + 3]);
snprintf(szRet, sizeof(szRet), "%u.%u.%u.%u", _address[IPADDRESS_V4_BYTES_INDEX], _address[IPADDRESS_V4_BYTES_INDEX + 1], _address[IPADDRESS_V4_BYTES_INDEX + 2], _address[IPADDRESS_V4_BYTES_INDEX + 3]);
return String(szRet);
}

String IPAddress::toString6() const
{
char szRet[40];
snprintf(szRet, sizeof(szRet), "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x",
_address.bytes[0], _address.bytes[1], _address.bytes[2], _address.bytes[3],
_address.bytes[4], _address.bytes[5], _address.bytes[6], _address.bytes[7],
_address.bytes[8], _address.bytes[9], _address.bytes[10], _address.bytes[11],
_address.bytes[12], _address.bytes[13], _address.bytes[14], _address.bytes[15]);
_address[0], _address[1], _address[2], _address[3],
_address[4], _address[5], _address[6], _address[7],
_address[8], _address[9], _address[10], _address[11],
_address[12], _address[13], _address[14], _address[15]);
return String(szRet);
}

Expand All @@ -135,7 +110,7 @@ bool IPAddress::fromString4(const char *address)
int16_t acc = -1; // Accumulator
uint8_t dots = 0;

memset(_address.bytes, 0, sizeof(_address.bytes));
_address.fill(0);
while (*address)
{
char c = *address++;
Expand All @@ -157,7 +132,7 @@ bool IPAddress::fromString4(const char *address)
/* No value between dots, e.g. '1..' */
return false;
}
_address.bytes[IPADDRESS_V4_BYTES_INDEX + dots++] = acc;
_address[IPADDRESS_V4_BYTES_INDEX + dots++] = acc;
acc = -1;
}
else
Expand All @@ -175,7 +150,7 @@ bool IPAddress::fromString4(const char *address)
/* No value between dots, e.g. '1..' */
return false;
}
_address.bytes[IPADDRESS_V4_BYTES_INDEX + 3] = acc;
_address[IPADDRESS_V4_BYTES_INDEX + 3] = acc;
_type = IPv4;
return true;
}
Expand Down Expand Up @@ -215,8 +190,8 @@ bool IPAddress::fromString6(const char *address) {
if (colons == 7)
// too many separators
return false;
_address.bytes[colons * 2] = acc >> 8;
_address.bytes[colons * 2 + 1] = acc & 0xff;
_address[colons * 2] = acc >> 8;
_address[colons * 2 + 1] = acc & 0xff;
colons++;
acc = 0;
}
Expand All @@ -233,15 +208,15 @@ bool IPAddress::fromString6(const char *address) {
// Too many segments (double colon must be at least one zero field)
return false;
}
_address.bytes[colons * 2] = acc >> 8;
_address.bytes[colons * 2 + 1] = acc & 0xff;
_address[colons * 2] = acc >> 8;
_address[colons * 2 + 1] = acc & 0xff;
colons++;

if (double_colons != -1) {
for (int i = colons * 2 - double_colons * 2 - 1; i >= 0; i--)
_address.bytes[16 - colons * 2 + double_colons * 2 + i] = _address.bytes[double_colons * 2 + i];
_address[16 - colons * 2 + double_colons * 2 + i] = _address[double_colons * 2 + i];
for (int i = double_colons * 2; i < 16 - colons * 2 + double_colons * 2; i++)
_address.bytes[i] = 0;
_address[i] = 0;
}

_type = IPv6;
Expand All @@ -252,8 +227,10 @@ IPAddress& IPAddress::operator=(const uint8_t *address)
{
// IPv4 only conversion from byte pointer
_type = IPv4;
memset(_address.bytes, 0, sizeof(_address.bytes));
memcpy(&_address.bytes[IPADDRESS_V4_BYTES_INDEX], address, sizeof(uint32_t));

_address.fill(0);
std::copy(address, address + 4, &_address[IPADDRESS_V4_BYTES_INDEX]);

return *this;
}

Expand All @@ -268,35 +245,35 @@ IPAddress& IPAddress::operator=(uint32_t address)
// IPv4 conversion
// See note on conversion/comparison and uint32_t
_type = IPv4;
memset(_address.bytes, 0, sizeof(_address.bytes));
_address.dword[IPADDRESS_V4_DWORD_INDEX] = address;
_address.fill(0);
uint32_t& addressRef = reinterpret_cast<uint32_t&>(_address[IPADDRESS_V4_BYTES_INDEX]);
addressRef = address;
return *this;
}

bool IPAddress::operator==(const IPAddress& addr) const {
return (addr._type == _type)
&& (memcmp(addr._address.bytes, _address.bytes, sizeof(_address.bytes)) == 0);
return addr._type == _type && std::equal(addr._address.begin(), addr._address.end(), _address.begin());
}

bool IPAddress::operator==(const uint8_t* addr) const
{
// IPv4 only comparison to byte pointer
// Can't support IPv6 as we know our type, but not the length of the pointer
return _type == IPv4 && memcmp(addr, &_address.bytes[IPADDRESS_V4_BYTES_INDEX], sizeof(uint32_t)) == 0;
return _type == IPv4 && std::equal(_address.begin(), _address.end(), addr);
}

uint8_t IPAddress::operator[](int index) const {
if (_type == IPv4) {
return _address.bytes[IPADDRESS_V4_BYTES_INDEX + index];
return _address[IPADDRESS_V4_BYTES_INDEX + index];
}
return _address.bytes[index];
return _address[index];
}

uint8_t& IPAddress::operator[](int index) {
if (_type == IPv4) {
return _address.bytes[IPADDRESS_V4_BYTES_INDEX + index];
return _address[IPADDRESS_V4_BYTES_INDEX + index];
}
return _address.bytes[index];
return _address[index];
}

size_t IPAddress::printTo(Print& p) const
Expand All @@ -310,7 +287,7 @@ size_t IPAddress::printTo(Print& p) const
int8_t current_start = -1;
int8_t current_length = 0;
for (int8_t f = 0; f < 8; f++) {
if (_address.bytes[f * 2] == 0 && _address.bytes[f * 2 + 1] == 0) {
if (_address[f * 2] == 0 && _address[f * 2 + 1] == 0) {
if (current_start == -1) {
current_start = f;
current_length = 1;
Expand All @@ -327,10 +304,10 @@ size_t IPAddress::printTo(Print& p) const
}
for (int f = 0; f < 8; f++) {
if (f < longest_start || f >= longest_start + longest_length) {
uint8_t c1 = _address.bytes[f * 2] >> 4;
uint8_t c2 = _address.bytes[f * 2] & 0xf;
uint8_t c3 = _address.bytes[f * 2 + 1] >> 4;
uint8_t c4 = _address.bytes[f * 2 + 1] & 0xf;
uint8_t c1 = _address[f * 2] >> 4;
uint8_t c2 = _address[f * 2] & 0xf;
uint8_t c3 = _address[f * 2 + 1] >> 4;
uint8_t c4 = _address[f * 2 + 1] & 0xf;
if (c1 > 0) {
n += p.print((char)(c1 < 10 ? '0' + c1 : 'a' + c1 - 10));
}
Expand All @@ -357,10 +334,10 @@ size_t IPAddress::printTo(Print& p) const
// IPv4
for (int i =0; i < 3; i++)
{
n += p.print(_address.bytes[IPADDRESS_V4_BYTES_INDEX + i], DEC);
n += p.print(_address[IPADDRESS_V4_BYTES_INDEX + i], DEC);
n += p.print('.');
}
n += p.print(_address.bytes[IPADDRESS_V4_BYTES_INDEX + 3], DEC);
n += p.print(_address[IPADDRESS_V4_BYTES_INDEX + 3], DEC);
return n;
}

Expand Down
15 changes: 6 additions & 9 deletions api/IPAddress.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@

#pragma once

#include <array>
#include <stdint.h>
#include "Printable.h"
#include "String.h"

#define IPADDRESS_V4_BYTES_INDEX 12
#define IPADDRESS_V4_DWORD_INDEX 3

// forward declarations of global name space friend classes
class EthernetClass;
Expand All @@ -42,17 +42,14 @@ enum IPType {

class IPAddress : public Printable {
private:
union {
uint8_t bytes[16];
uint32_t dword[4];
} _address;
IPType _type;
alignas(alignof(uint32_t)) std::array<uint8_t, 16> _address{};
IPType _type{IPv4};

// Access the raw byte array containing the address. Because this returns a pointer
// to the internal structure rather than a copy of the address this function should only
// be used when you know that the usage of the returned uint8_t* will be transient and not
// stored.
uint8_t* raw_address() { return _type == IPv4 ? &_address.bytes[IPADDRESS_V4_BYTES_INDEX] : _address.bytes; }
uint8_t* raw_address() { return _type == IPv4 ? &_address[IPADDRESS_V4_BYTES_INDEX] : _address.data(); }

public:
// Constructors
Expand All @@ -75,7 +72,7 @@ class IPAddress : public Printable {

// Overloaded cast operator to allow IPAddress objects to be used where a uint32_t is expected
// NOTE: IPv4 only; see implementation note
operator uint32_t() const { return _type == IPv4 ? _address.dword[IPADDRESS_V4_DWORD_INDEX] : 0; };
operator uint32_t() const { return _type == IPv4 ? *reinterpret_cast<const uint32_t*>(&_address[IPADDRESS_V4_BYTES_INDEX]) : 0; };

bool operator==(const IPAddress& addr) const;
bool operator!=(const IPAddress& addr) const { return !(*this == addr); };
Expand Down Expand Up @@ -119,4 +116,4 @@ extern const IPAddress IN6ADDR_ANY;
extern const IPAddress INADDR_NONE;
}

using arduino::IPAddress;
using arduino::IPAddress;

0 comments on commit 52b367a

Please sign in to comment.