From 52b367a899923f713ffbbb4bb3eeea1520fcad35 Mon Sep 17 00:00:00 2001 From: safocl Date: Wed, 8 Nov 2023 17:29:52 +0400 Subject: [PATCH 1/5] Rewrite IPAddress without union 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. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 https://github.com/espressif/arduino-esp32/pull/8829 PR dependent on changing the current API. --- api/IPAddress.cpp | 123 +++++++++++++++++++--------------------------- api/IPAddress.h | 15 +++--- 2 files changed, 56 insertions(+), 82 deletions(-) diff --git a/api/IPAddress.cpp b/api/IPAddress.cpp index 05b41bc1..180371c8 100644 --- a/api/IPAddress.cpp +++ b/api/IPAddress.cpp @@ -19,53 +19,30 @@ #include "IPAddress.h" #include "Print.h" +#include +#include 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(_address[IPADDRESS_V4_BYTES_INDEX]); + addressRef = address; // NOTE on conversion/comparison and uint32_t: // These conversions are host platform dependent. @@ -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()); } } @@ -97,7 +72,7 @@ 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); } @@ -105,10 +80,10 @@ 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); } @@ -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++; @@ -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 @@ -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; } @@ -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; } @@ -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; @@ -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; } @@ -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(_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 @@ -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; @@ -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)); } @@ -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; } diff --git a/api/IPAddress.h b/api/IPAddress.h index 28dde3be..ef8556d1 100644 --- a/api/IPAddress.h +++ b/api/IPAddress.h @@ -19,12 +19,12 @@ #pragma once +#include #include #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; @@ -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 _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 @@ -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(&_address[IPADDRESS_V4_BYTES_INDEX]) : 0; }; bool operator==(const IPAddress& addr) const; bool operator!=(const IPAddress& addr) const { return !(*this == addr); }; @@ -119,4 +116,4 @@ extern const IPAddress IN6ADDR_ANY; extern const IPAddress INADDR_NONE; } -using arduino::IPAddress; \ No newline at end of file +using arduino::IPAddress; From b44f1b121f90d9481d88124ef44e9e122d8b0efc Mon Sep 17 00:00:00 2001 From: safocl Date: Wed, 8 Nov 2023 18:08:29 +0400 Subject: [PATCH 2/5] Fix comparison operator error Begin index didn't be selected. --- api/IPAddress.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/IPAddress.cpp b/api/IPAddress.cpp index 180371c8..8eeabbae 100644 --- a/api/IPAddress.cpp +++ b/api/IPAddress.cpp @@ -259,7 +259,7 @@ 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 && std::equal(_address.begin(), _address.end(), addr); + return _type == IPv4 && std::equal(_address.begin() + IPADDRESS_V4_BYTES_INDEX, _address.end(), addr); } uint8_t IPAddress::operator[](int index) const { From d85523f1cb389210432f1e279528cf22a7d916c8 Mon Sep 17 00:00:00 2001 From: safocl Date: Tue, 28 Nov 2023 16:35:35 +0400 Subject: [PATCH 3/5] Rewrite IPAddress from std::array to C-style array. --- api/IPAddress.cpp | 19 +++++++++---------- api/IPAddress.h | 5 ++--- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/api/IPAddress.cpp b/api/IPAddress.cpp index 8eeabbae..8ebb0afe 100644 --- a/api/IPAddress.cpp +++ b/api/IPAddress.cpp @@ -19,8 +19,6 @@ #include "IPAddress.h" #include "Print.h" -#include -#include using namespace arduino; @@ -58,9 +56,9 @@ IPAddress::IPAddress(const uint8_t *address) : IPAddress(IPv4, address) {} IPAddress::IPAddress(IPType ip_type, const uint8_t *address) : _type(ip_type) { if (ip_type == IPv4) { - std::copy(address, address + 4, &_address[IPADDRESS_V4_BYTES_INDEX]); + memcpy(&_address[IPADDRESS_V4_BYTES_INDEX], address, sizeof(uint32_t)); } else { - std::copy(address, address + _address.size(), _address.begin()); + memcpy(_address, address, sizeof(_address)); } } @@ -110,7 +108,7 @@ bool IPAddress::fromString4(const char *address) int16_t acc = -1; // Accumulator uint8_t dots = 0; - _address.fill(0); + memset(_address, 0, sizeof(_address)); while (*address) { char c = *address++; @@ -228,8 +226,8 @@ IPAddress& IPAddress::operator=(const uint8_t *address) // IPv4 only conversion from byte pointer _type = IPv4; - _address.fill(0); - std::copy(address, address + 4, &_address[IPADDRESS_V4_BYTES_INDEX]); + memset(_address, 0, sizeof(_address)); + memcpy(&_address[IPADDRESS_V4_BYTES_INDEX], address, sizeof(uint32_t)); return *this; } @@ -245,21 +243,22 @@ IPAddress& IPAddress::operator=(uint32_t address) // IPv4 conversion // See note on conversion/comparison and uint32_t _type = IPv4; - _address.fill(0); + memset(_address, 0, sizeof(_address)); uint32_t& addressRef = reinterpret_cast(_address[IPADDRESS_V4_BYTES_INDEX]); addressRef = address; return *this; } bool IPAddress::operator==(const IPAddress& addr) const { - return addr._type == _type && std::equal(addr._address.begin(), addr._address.end(), _address.begin()); + return (addr._type == _type) + && (memcmp(addr._address, _address, sizeof(_address)) == 0); } 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 && std::equal(_address.begin() + IPADDRESS_V4_BYTES_INDEX, _address.end(), addr); + return _type == IPv4 && memcmp(addr, &_address[IPADDRESS_V4_BYTES_INDEX], sizeof(uint32_t)) == 0; } uint8_t IPAddress::operator[](int index) const { diff --git a/api/IPAddress.h b/api/IPAddress.h index 3098f6cd..dd6257a6 100644 --- a/api/IPAddress.h +++ b/api/IPAddress.h @@ -19,7 +19,6 @@ #pragma once -#include #include #include "Printable.h" #include "String.h" @@ -42,14 +41,14 @@ enum IPType { class IPAddress : public Printable { private: - alignas(alignof(uint32_t)) std::array _address{}; + alignas(alignof(uint32_t)) uint8_t _address[16]{}; 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[IPADDRESS_V4_BYTES_INDEX] : _address.data(); } + uint8_t* raw_address() { return _type == IPv4 ? &_address[IPADDRESS_V4_BYTES_INDEX] : _address; } public: // Constructors From dc7b5b5e5b690d21fb58f43090e0f03956a8ded2 Mon Sep 17 00:00:00 2001 From: safocl Date: Fri, 31 Jan 2025 05:36:44 +0400 Subject: [PATCH 4/5] rewrite(IPAddress): fix implementation fix implementation and add discussion questions (safe using of IPAddress). --- api/IPAddress.cpp | 33 +++++++++++++++++++++++++++++---- api/IPAddress.h | 17 +++++++++++++++-- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/api/IPAddress.cpp b/api/IPAddress.cpp index 8ebb0afe..0bf84ab7 100644 --- a/api/IPAddress.cpp +++ b/api/IPAddress.cpp @@ -39,8 +39,34 @@ IPAddress::IPAddress(uint8_t o1, uint8_t o2, uint8_t o3, uint8_t o4, uint8_t o5, // IPv4 only IPAddress::IPAddress(uint32_t address) { - uint32_t& addressRef = reinterpret_cast(_address[IPADDRESS_V4_BYTES_INDEX]); - addressRef = address; + memcpy(&_address[IPADDRESS_V4_BYTES_INDEX], &address, 4); // This method guarantees a defined behavior. Any pointer conversions to write to ADDRESS storage (as a multibyte integer) are undefined behavior when the lifetime of the multibyte type has not previously started. + + // C++ standard draft [basic.life#7](https://eel.is/c++draft/basic.life#7) +// Before the lifetime of an object has started but after the storage which the object +// will occupy has been allocated or, after the lifetime of an object has ended and +// before the storage which the object occupied is reused or released, any pointer that +// represents the address of the storage location where the object will be or was +// located may be used but only in limited ways. For an object under construction or +// destruction, see [class.cdtor]. Otherwise, such a pointer refers to allocated storage +// ([basic.stc.dynamic.allocation]), and using the pointer as if the pointer were of +// type void* is well-defined. Indirection through such a pointer is permitted but the +// resulting lvalue may only be used in limited ways, as described below. +// The program has undefined behavior if +// --the pointer is used as the operand of a delete-expression, +// --the pointer is used as the operand of a static_cast ([expr.static.cast]), except +// when the conversion is to pointer to cv void, or to pointer to cv void and subsequently +// to pointer to cv char, cv unsigned char, or cv std::byte ([cstddef.syn]), or + + // C++ standard draft [basic.life#8](https://eel.is/c++draft/basic.life#8) +// Similarly, before the lifetime of an object has started but after the storage which +// the object will occupy has been allocated or, after the lifetime of an object has +// ended and before the storage which the object occupied is reused or released, any +// glvalue that refers to the original object may be used but only in limited ways. +// For an object under construction or destruction, see [class.cdtor]. Otherwise, such +// a glvalue refers to allocated storage ([basic.stc.dynamic.allocation]), and using +// the properties of the glvalue that do not depend on its value is well-defined. +// The program has undefined behavior if +// -- the glvalue is used to access the object, or // NOTE on conversion/comparison and uint32_t: // These conversions are host platform dependent. @@ -244,8 +270,7 @@ IPAddress& IPAddress::operator=(uint32_t address) // See note on conversion/comparison and uint32_t _type = IPv4; memset(_address, 0, sizeof(_address)); - uint32_t& addressRef = reinterpret_cast(_address[IPADDRESS_V4_BYTES_INDEX]); - addressRef = address; + memcpy(&_address[IPADDRESS_V4_BYTES_INDEX], &address, 4); return *this; } diff --git a/api/IPAddress.h b/api/IPAddress.h index dd6257a6..096b33eb 100644 --- a/api/IPAddress.h +++ b/api/IPAddress.h @@ -19,6 +19,7 @@ #pragma once +#include #include #include "Printable.h" #include "String.h" @@ -41,7 +42,10 @@ enum IPType { class IPAddress : public Printable { private: - alignas(alignof(uint32_t)) uint8_t _address[16]{}; + alignas(alignof(uint32_t)) uint8_t _address[16]{}; // If the implementation does not require + // storage as a multibyte integer, you can + // remove the storage field alignment. + // Address (as uint32) is accessed by copying. IPType _type{IPv4}; // Access the raw byte array containing the address. Because this returns a pointer @@ -59,6 +63,7 @@ class IPAddress : public Printable { IPAddress(uint8_t first_octet, uint8_t second_octet, uint8_t third_octet, uint8_t fourth_octet); 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); // IPv4; see implementation note + // NOTE: address MUST BE BigEndian. IPAddress(uint32_t address); // Default IPv4 IPAddress(const uint8_t *address); @@ -71,7 +76,15 @@ 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 ? *reinterpret_cast(&_address[IPADDRESS_V4_BYTES_INDEX]) : 0; }; + // NOTE: Data of the returned integer in the native endianness, but relevant ordering is a BigEndian. + // The user is responsible for ensuring that the value is converted to BigEndian. + operator uint32_t() const { + uint32_t ret; + memcpy(&ret, &_address[IPADDRESS_V4_BYTES_INDEX], 4); + // NOTE: maybe use the placement-new for starting of the integer type lifetime in the storage when constructing an IPAddress? + // FIXME: need endianness checking? how do this with the arduino-api? + return _type == IPv4 ? ret : 0; + }; bool operator==(const IPAddress& addr) const; bool operator!=(const IPAddress& addr) const { return !(*this == addr); }; From 17c62feac959ad1c6d45b6b9907d32b116c7d475 Mon Sep 17 00:00:00 2001 From: safocl Date: Sun, 9 Feb 2025 13:49:40 +0400 Subject: [PATCH 5/5] refactor: adaptation for discussion some code refactored (`placement-new` rather than `memcpy`; `raw_address` rather than `&_address[IPADDRESS_V4_BYTES_INDEX]`). other parts of the C++ standard have been added and embedded into the code for discussion. --- api/IPAddress.cpp | 64 ++++++++++++++++++++++++++++++++--------------- api/IPAddress.h | 8 ++++-- 2 files changed, 50 insertions(+), 22 deletions(-) diff --git a/api/IPAddress.cpp b/api/IPAddress.cpp index 0bf84ab7..f056827f 100644 --- a/api/IPAddress.cpp +++ b/api/IPAddress.cpp @@ -19,6 +19,8 @@ #include "IPAddress.h" #include "Print.h" +#include +#include using namespace arduino; @@ -39,9 +41,40 @@ IPAddress::IPAddress(uint8_t o1, uint8_t o2, uint8_t o3, uint8_t o4, uint8_t o5, // IPv4 only IPAddress::IPAddress(uint32_t address) { - memcpy(&_address[IPADDRESS_V4_BYTES_INDEX], &address, 4); // This method guarantees a defined behavior. Any pointer conversions to write to ADDRESS storage (as a multibyte integer) are undefined behavior when the lifetime of the multibyte type has not previously started. - - // C++ standard draft [basic.life#7](https://eel.is/c++draft/basic.life#7) + // memcpy(raw_address(), &address, 4); // This method guarantees a defined behavior. + // But lifetime started when: + // [basic.life#2] https://eel.is/c++draft/basic.life#2 + //(2.1) -- storage with the proper alignment and size for type T is obtained, and + //(2.2) -- its initialization (if any) is complete (including vacuous initialization) ([dcl.init]), + // + // The statement: {#Any pointer conversions to write to ADDRESS storage (as a multibyte integer) + // are undefined behavior when the lifetime of the multibyte type has not previously started.#} + // only apply to c++17 and earlier. Since C++20 array of bytes implicitly creates the inner objects. + +// C++20: https://timsong-cpp.github.io/cppwp/n4861/intro.object#13 +// 13 An operation that begins the lifetime of an array of char, unsigned char, or std::byte implicitly creates objects within +// the region of storage occupied by the array. [ Note: The array object provides storage for these objects. — end note ] + +// C++23: https://timsong-cpp.github.io/cppwp/n4950/intro.object#13 +// 13 An operation that begins the lifetime of an array of unsigned char or std::byte implicitly creates objects within the +// region of storage occupied by the array. +// [Note 5: The array object provides storage for these objects. — end note] + +// Current draft: https://eel.is/c++draft/intro.object#14 +// 14 Except during constant evaluation, an operation that begins the lifetime of an array of unsigned char or std::byte implicitly +// creates objects within the region of storage occupied by the array. +// [Note 5: The array object provides storage for these objects. — end note] + +// *reinterpret_cast(_address[IPADDRESS_V4_BYTES_INDEX]) = address; // This valid initialization in the `_address` storage since C++20. + // now the pointer `_address[IPADDRESS_V4_BYTES_INDEX]` points to a multibyte int. + + new (&_address[IPADDRESS_V4_BYTES_INDEX]) uint32_t (address); // But the new-expression is better for understanding and looks nicer (for trivial types, the + // new expression only begins its lifetime and does not perform any other actions). + // NOTE: https://en.cppreference.com/w/cpp/language/new#Notes + + // NOTE: new-expression and reinterpret_cast require alignment of the storage, but memcpy does not. + +// C++ standard draft [basic.life#7](https://eel.is/c++draft/basic.life#7) // Before the lifetime of an object has started but after the storage which the object // will occupy has been allocated or, after the lifetime of an object has ended and // before the storage which the object occupied is reused or released, any pointer that @@ -57,7 +90,7 @@ IPAddress::IPAddress(uint32_t address) // when the conversion is to pointer to cv void, or to pointer to cv void and subsequently // to pointer to cv char, cv unsigned char, or cv std::byte ([cstddef.syn]), or - // C++ standard draft [basic.life#8](https://eel.is/c++draft/basic.life#8) +// C++ standard draft [basic.life#8](https://eel.is/c++draft/basic.life#8) // Similarly, before the lifetime of an object has started but after the storage which // the object will occupy has been allocated or, after the lifetime of an object has // ended and before the storage which the object occupied is reused or released, any @@ -81,11 +114,8 @@ IPAddress::IPAddress(const uint8_t *address) : IPAddress(IPv4, address) {} IPAddress::IPAddress(IPType ip_type, const uint8_t *address) : _type(ip_type) { - if (ip_type == IPv4) { - memcpy(&_address[IPADDRESS_V4_BYTES_INDEX], address, sizeof(uint32_t)); - } else { - memcpy(_address, address, sizeof(_address)); - } + const size_t copy_size = (ip_type == IPv4) ? sizeof(uint32_t) : sizeof(_address); + memcpy(raw_address(), address, copy_size); } IPAddress::IPAddress(const char *address) @@ -253,7 +283,7 @@ IPAddress& IPAddress::operator=(const uint8_t *address) _type = IPv4; memset(_address, 0, sizeof(_address)); - memcpy(&_address[IPADDRESS_V4_BYTES_INDEX], address, sizeof(uint32_t)); + memcpy(raw_address(), address, sizeof(uint32_t)); return *this; } @@ -270,7 +300,7 @@ IPAddress& IPAddress::operator=(uint32_t address) // See note on conversion/comparison and uint32_t _type = IPv4; memset(_address, 0, sizeof(_address)); - memcpy(&_address[IPADDRESS_V4_BYTES_INDEX], &address, 4); + new (raw_address()) uint32_t (address); // See the comments in corresponding constructor. return *this; } @@ -283,21 +313,15 @@ 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[IPADDRESS_V4_BYTES_INDEX], sizeof(uint32_t)) == 0; + return _type == IPv4 && memcmp(addr, raw_address(), sizeof(uint32_t)) == 0; } uint8_t IPAddress::operator[](int index) const { - if (_type == IPv4) { - return _address[IPADDRESS_V4_BYTES_INDEX + index]; - } - return _address[index]; + return *(raw_address() + index); } uint8_t& IPAddress::operator[](int index) { - if (_type == IPv4) { - return _address[IPADDRESS_V4_BYTES_INDEX + index]; - } - return _address[index]; + return *(raw_address() + index); } size_t IPAddress::printTo(Print& p) const diff --git a/api/IPAddress.h b/api/IPAddress.h index 096b33eb..3c2e845b 100644 --- a/api/IPAddress.h +++ b/api/IPAddress.h @@ -52,7 +52,10 @@ class IPAddress : public Printable { // 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. + const uint8_t* raw_address() const { return _type == IPv4 ? &_address[IPADDRESS_V4_BYTES_INDEX] : _address; } uint8_t* raw_address() { return _type == IPv4 ? &_address[IPADDRESS_V4_BYTES_INDEX] : _address; } + // NOTE: If IPADDRESS_V4_BYTES_INDEX region can be initialized with a multibyte int, then a cast to unsigned char is required. + public: // Constructors @@ -80,8 +83,9 @@ class IPAddress : public Printable { // The user is responsible for ensuring that the value is converted to BigEndian. operator uint32_t() const { uint32_t ret; - memcpy(&ret, &_address[IPADDRESS_V4_BYTES_INDEX], 4); - // NOTE: maybe use the placement-new for starting of the integer type lifetime in the storage when constructing an IPAddress? + memcpy(&ret, raw_address(), 4); + // NOTE: maybe use the placement-new (or standard initialisation of uint32_t since C++20) + // for starting of the integer type lifetime in the storage when constructing an IPAddress? // FIXME: need endianness checking? how do this with the arduino-api? return _type == IPv4 ? ret : 0; };