From 6c565689ae2f5db2cd2104bfdebe36a926e7e079 Mon Sep 17 00:00:00 2001 From: Robert Adam Date: Fri, 3 Feb 2023 14:51:10 +0100 Subject: [PATCH] REFAC: Get rid of union in HostAddress The way this union was accessed is not covered by the C++ standard and was therefore undefined behavior. Therefore, the code was rewritten to stick to standard C++. --- src/HostAddress.cpp | 165 +++++++++++++++++++++------------ src/HostAddress.h | 40 ++++---- src/ServerAddress.cpp | 3 - src/ServerAddress.h | 6 +- src/murmur/DBus.cpp | 2 +- src/murmur/MumbleServerIce.cpp | 25 ++--- src/murmur/Server.cpp | 5 +- 7 files changed, 148 insertions(+), 98 deletions(-) diff --git a/src/HostAddress.cpp b/src/HostAddress.cpp index fa5b11036..432377379 100644 --- a/src/HostAddress.cpp +++ b/src/HostAddress.cpp @@ -20,102 +20,129 @@ # endif #endif -HostAddress::HostAddress() { - addr[0] = addr[1] = 0ULL; -} +#include +#include +#include HostAddress::HostAddress(const Q_IPV6ADDR &address) { - memcpy(qip6.c, address.c, 16); + memcpy(m_byteRepresentation.data(), address.c, m_byteRepresentation.size()); } HostAddress::HostAddress(const std::string &address) { - if (address.length() != 16) - addr[0] = addr[1] = 0ULL; - else - for (int i = 0; i < 16; ++i) - qip6[i] = address[i]; + if (address.length() != 16) { + // This is an invalid address -> reset the currently stored address + m_byteRepresentation.fill(0); + } else { + for (std::size_t i = 0; i < m_byteRepresentation.size(); ++i) { + m_byteRepresentation[i] = static_cast< unsigned char >(address[i]); + } + } } HostAddress::HostAddress(const QByteArray &address) { - if (address.length() != 16) - addr[0] = addr[1] = 0ULL; - else - for (int i = 0; i < 16; ++i) - qip6[i] = address[i]; + if (address.length() != 16) { + // This is an invalid address -> reset the currently stored address + m_byteRepresentation.fill(0); + } else { + for (unsigned int i = 0; i < m_byteRepresentation.size(); ++i) { + m_byteRepresentation[i] = static_cast< unsigned char >(address[i]); + } + } } HostAddress::HostAddress(const QHostAddress &address) { if (address.protocol() == QAbstractSocket::IPv6Protocol) { const Q_IPV6ADDR &a = address.toIPv6Address(); - memcpy(qip6.c, a.c, 16); + memcpy(m_byteRepresentation.data(), a.c, m_byteRepresentation.size()); } else { - addr[0] = 0ULL; - shorts[4] = 0; - shorts[5] = 0xffff; - hash[3] = htonl(address.toIPv4Address()); + fromIPv4(address.toIPv4Address()); } } HostAddress::HostAddress(const sockaddr_storage &address) { if (address.ss_family == AF_INET) { const struct sockaddr_in *in = reinterpret_cast< const struct sockaddr_in * >(&address); - addr[0] = 0ULL; - shorts[4] = 0; - shorts[5] = 0xffff; - hash[3] = in->sin_addr.s_addr; + fromIPv4(in->sin_addr.s_addr, false); } else if (address.ss_family == AF_INET6) { const struct sockaddr_in6 *in6 = reinterpret_cast< const struct sockaddr_in6 * >(&address); - memcpy(qip6.c, in6->sin6_addr.s6_addr, 16); + memcpy(m_byteRepresentation.data(), in6->sin6_addr.s6_addr, m_byteRepresentation.size()); } else { - addr[0] = addr[1] = 0ULL; + m_byteRepresentation.fill(0); } } +void HostAddress::fromIPv4(std::uint32_t address, bool convertToNetworkOrder) { + // Store IPv4 address in IPv6 format: + // - address is stored in the 4 last bytes in network byte order + // - the 2 bytes just before that are set to 0xFF respectively + m_byteRepresentation.fill(0); + + m_byteRepresentation[10] = 0xFF; + m_byteRepresentation[11] = 0xFF; + + if (convertToNetworkOrder) { + address = htonl(address); + } + + memcpy(&m_byteRepresentation[12], &address, sizeof(std::uint32_t)); +} + bool HostAddress::operator<(const HostAddress &other) const { - return memcmp(qip6.c, other.qip6.c, 16) < 0; + return m_byteRepresentation < other.m_byteRepresentation; } bool HostAddress::operator==(const HostAddress &other) const { - return ((addr[0] == other.addr[0]) && (addr[1] == other.addr[1])); + return m_byteRepresentation == other.m_byteRepresentation; } -bool HostAddress::match(const HostAddress &netmask, int bits) const { - quint64 mask[2]; +bool HostAddress::match(const HostAddress &netmask, unsigned int bits) const { + for (std::size_t i = 0; i < m_byteRepresentation.size(); ++i) { + if (bits >= 8) { + // Compare full byte + if (m_byteRepresentation[i] != netmask.m_byteRepresentation[i]) { + return false; + } + bits -= 8; + } else { + // Compare only the first bits bits (no this is not a typo) + using mask_t = std::uint8_t; + mask_t mask = + static_cast< mask_t >(std::numeric_limits< mask_t >::max() >> (sizeof(mask_t) * CHAR_BIT - bits)); + mask = static_cast< mask_t >(htons(mask)); - if (bits == 128) { - mask[0] = mask[1] = 0xffffffffffffffffULL; - } else if (bits > 64) { - mask[0] = 0xffffffffffffffffULL; - mask[1] = SWAP64(~((1ULL << (128 - bits)) - 1)); - } else { - mask[0] = SWAP64(~((1ULL << (64 - bits)) - 1)); - mask[1] = 0ULL; + if ((m_byteRepresentation[i] & mask) != (netmask.m_byteRepresentation[i] & mask)) { + return false; + } + + break; + } } - return ((addr[0] & mask[0]) == (netmask.addr[0] & mask[0])) && ((addr[1] & mask[1]) == (netmask.addr[1] & mask[1])); + + return true; } std::string HostAddress::toStdString() const { - return std::string(reinterpret_cast< const char * >(qip6.c), 16); + return std::string(reinterpret_cast< const char * >(m_byteRepresentation.data()), m_byteRepresentation.size()); } bool HostAddress::isV6() const { - return (addr[0] != 0ULL) || (shorts[4] != 0) || (shorts[5] != 0xffff); + std::uint64_t firstEightBytes = *(reinterpret_cast< const std::uint64_t * >(m_byteRepresentation.data())); + std::uint16_t bytesNineAndTen = *(reinterpret_cast< const std::uint16_t * >(&m_byteRepresentation[8])); + std::uint16_t bytesElevenAndTwelve = *(reinterpret_cast< const std::uint16_t * >(&m_byteRepresentation[10])); + return firstEightBytes != 0 || bytesNineAndTen != 0 || bytesElevenAndTwelve != 0xFFFF; } bool HostAddress::isValid() const { - return (addr[0] != 0ULL) || (addr[1] != 0ULL); + return m_byteRepresentation != decltype(m_byteRepresentation){}; } QHostAddress HostAddress::toAddress() const { - if (isV6()) - return QHostAddress(qip6); - else { - return QHostAddress(ntohl(hash[3])); - } + return QHostAddress(m_byteRepresentation.data()); } QByteArray HostAddress::toByteArray() const { - return QByteArray(reinterpret_cast< const char * >(qip6.c), 16); + return QByteArray(reinterpret_cast< const char * >(m_byteRepresentation.data()), + static_cast< int >(m_byteRepresentation.size())); } void HostAddress::toSockaddr(sockaddr_storage *dst) const { @@ -123,16 +150,34 @@ void HostAddress::toSockaddr(sockaddr_storage *dst) const { if (isV6()) { struct sockaddr_in6 *in6 = reinterpret_cast< struct sockaddr_in6 * >(dst); dst->ss_family = AF_INET6; - memcpy(in6->sin6_addr.s6_addr, qip6.c, 16); + memcpy(in6->sin6_addr.s6_addr, m_byteRepresentation.data(), m_byteRepresentation.size()); } else { struct sockaddr_in *in = reinterpret_cast< struct sockaddr_in * >(dst); dst->ss_family = AF_INET; - in->sin_addr.s_addr = hash[3]; + in->sin_addr.s_addr = toIPv4(); } } +std::uint32_t HostAddress::toIPv4() const { + // The IPv4 address is stored in the last four bytes (in network byte order) + return *(reinterpret_cast< const std::uint32_t * >(m_byteRepresentation[12])); +} + +const std::array< std::uint8_t, 16 > &HostAddress::getByteRepresentation() const { + return m_byteRepresentation; +} + +void HostAddress::reset() { + m_byteRepresentation.fill(0); +} + +void HostAddress::setByte(std::size_t idx, std::uint8_t value) { + assert(idx < m_byteRepresentation.size()); + m_byteRepresentation[idx] = value; +} + quint32 qHash(const HostAddress &ha) { - return (ha.hash[0] ^ ha.hash[1] ^ ha.hash[2] ^ ha.hash[3]); + return qHashRange(ha.m_byteRepresentation.begin(), ha.m_byteRepresentation.end()); } QString HostAddress::toString(bool bracketEnclosed) const { @@ -145,21 +190,19 @@ QString HostAddress::toString(bool bracketEnclosed) const { squareBracketOpen = "["; squareBracketClose = "]"; } -#if QT_VERSION >= 0x050500 - str = QString::asprintf("%s%x:%x:%x:%x:%x:%x:%x:%x%s", squareBracketOpen, ntohs(shorts[0]), - ntohs(shorts[1]), ntohs(shorts[2]), ntohs(shorts[3]), ntohs(shorts[4]), - ntohs(shorts[5]), ntohs(shorts[6]), ntohs(shorts[7]), squareBracketClose); -#else - // sprintf() has been deprecated in Qt 5.5 in favor for the static QString::asprintf() - str.sprintf("%s%x:%x:%x:%x:%x:%x:%x:%x%s", squareBracketOpen, ntohs(shorts[0]), ntohs(shorts[1]), - ntohs(shorts[2]), ntohs(shorts[3]), ntohs(shorts[4]), ntohs(shorts[5]), ntohs(shorts[6]), - ntohs(shorts[7]), squareBracketClose); -#endif + + const std::uint16_t *shortArray = reinterpret_cast< const std::uint16_t * >(m_byteRepresentation.data()); + + str = QString::asprintf("%s%x:%x:%x:%x:%x:%x:%x:%x%s", squareBracketOpen, ntohs(shortArray[0]), + ntohs(shortArray[1]), ntohs(shortArray[2]), ntohs(shortArray[3]), + ntohs(shortArray[4]), ntohs(shortArray[5]), ntohs(shortArray[6]), + ntohs(shortArray[7]), squareBracketClose); + return str.replace(QRegExp(QLatin1String("(:0)+")), QLatin1String(":")); } else { return bracketEnclosed ? QLatin1String("[::]") : QLatin1String("::"); } } else { - return QHostAddress(ntohl(hash[3])).toString(); + return toAddress().toString(); } } diff --git a/src/HostAddress.h b/src/HostAddress.h index 247fa2e5d..c9b693e88 100644 --- a/src/HostAddress.h +++ b/src/HostAddress.h @@ -6,34 +6,31 @@ #ifndef MUMBLE_HOSTADDRESS_H_ #define MUMBLE_HOSTADDRESS_H_ -#include -#include -#include -#include -#include +#include +#include +#include +#include + +#include +#include struct HostAddress { - union { - Q_IPV6ADDR qip6; - quint16 shorts[8]; - quint32 hash[4]; - quint64 addr[2]; - }; - - HostAddress(); + HostAddress() = default; HostAddress(const Q_IPV6ADDR &); HostAddress(const std::string &); HostAddress(const QHostAddress &); HostAddress(const QByteArray &); HostAddress(const struct sockaddr_storage &); + void fromIPv4(std::uint32_t address, bool convertToNetworkOrder = true); + bool isV6() const; bool isValid() const; bool operator<(const HostAddress &) const; bool operator==(const HostAddress &) const; - bool match(const HostAddress &, int bits) const; + bool match(const HostAddress &, unsigned int bits) const; QString toString(bool bracketEnclosed = true) const; @@ -41,10 +38,21 @@ struct HostAddress { QHostAddress toAddress() const; QByteArray toByteArray() const; void toSockaddr(struct sockaddr_storage *dst) const; + std::uint32_t toIPv4() const; + + const std::array< std::uint8_t, 16 > &getByteRepresentation() const; + + void reset(); + + void setByte(std::size_t idx, std::uint8_t value); + + friend quint32 qHash(const HostAddress &); + +private: + // Binary representation of an IPv6 address + std::array< std::uint8_t, 16 > m_byteRepresentation; }; Q_DECLARE_TYPEINFO(HostAddress, Q_MOVABLE_TYPE); -quint32 qHash(const HostAddress &); - #endif diff --git a/src/ServerAddress.cpp b/src/ServerAddress.cpp index 048c6f9a5..a67ee7182 100644 --- a/src/ServerAddress.cpp +++ b/src/ServerAddress.cpp @@ -5,9 +5,6 @@ #include "ServerAddress.h" -ServerAddress::ServerAddress() : port(0) { -} - ServerAddress::ServerAddress(HostAddress host_, unsigned short port_) : host(host_), port(port_) { } diff --git a/src/ServerAddress.h b/src/ServerAddress.h index 23b10812f..e0424d96d 100644 --- a/src/ServerAddress.h +++ b/src/ServerAddress.h @@ -14,13 +14,13 @@ /// address consisting of a HostAddress /// and a port. struct ServerAddress { - HostAddress host; - unsigned short port; + HostAddress host = {}; + unsigned short port = 0; /// Construct a default ServerAddress. /// The default ServerAddress value is considered /// invalid per the |isValid| method. - ServerAddress(); + ServerAddress() = default; /// Construct a ServerAddress pointing to |host_| and |port_|. ServerAddress(HostAddress host_, unsigned short port_); diff --git a/src/murmur/DBus.cpp b/src/murmur/DBus.cpp index cecc80aa3..aaa9fe66e 100644 --- a/src/murmur/DBus.cpp +++ b/src/murmur/DBus.cpp @@ -766,7 +766,7 @@ GroupInfo::GroupInfo(const Group *g) : inherited(false) { } BanInfo::BanInfo(const Ban &b) { - address = ntohl(b.haAddress.hash[3]); + address = ntohl(b.haAddress.toIPv4()); bits = b.iMask; } diff --git a/src/murmur/MumbleServerIce.cpp b/src/murmur/MumbleServerIce.cpp index fcec475dc..b2a6f8696 100644 --- a/src/murmur/MumbleServerIce.cpp +++ b/src/murmur/MumbleServerIce.cpp @@ -132,9 +132,9 @@ static void userToUser(const ::User *p, ::MumbleServer::User &mp) { #endif ::MumbleServer::NetAddress addr(16, 0); - const Q_IPV6ADDR &a = u->haAddress.qip6; - for (int i = 0; i < 16; ++i) - addr[i] = a[i]; + for (unsigned int i = 0; i < 16; ++i) { + addr[i] = u->haAddress.getByteRepresentation()[i]; + } mp.address = addr; } @@ -172,9 +172,9 @@ static void groupToGroup(const ::Group *g, ::MumbleServer::Group &mg) { static void banToBan(const ::Ban &b, ::MumbleServer::Ban &mb) { ::MumbleServer::NetAddress addr(16, 0); - const Q_IPV6ADDR &a = b.haAddress.qip6; - for (int i = 0; i < 16; ++i) - addr[i] = a[i]; + for (unsigned int i = 0; i < 16; ++i) { + addr[i] = b.haAddress.getByteRepresentation()[i]; + } mb.address = addr; mb.bits = b.iMask; @@ -186,12 +186,13 @@ static void banToBan(const ::Ban &b, ::MumbleServer::Ban &mb) { } static void banToBan(const ::MumbleServer::Ban &mb, ::Ban &b) { - if (mb.address.size() != 16) - for (int i = 0; i < 16; ++i) - b.haAddress.qip6[i] = 0; - else - for (int i = 0; i < 16; ++i) - b.haAddress.qip6[i] = mb.address[i]; + if (mb.address.size() != 16) { + b.haAddress.reset(); + } else { + for (unsigned int i = 0; i < 16; ++i) { + b.haAddress.setByte(i, mb.address[i]); + } + } b.iMask = mb.bits; b.qsUsername = u8(mb.name); b.qsHash = u8(mb.hash); diff --git a/src/murmur/Server.cpp b/src/murmur/Server.cpp index 55682804a..795b30bea 100644 --- a/src/murmur/Server.cpp +++ b/src/murmur/Server.cpp @@ -1064,7 +1064,8 @@ void Server::sendMessage(ServerUser &u, const unsigned char *data, int len, QByt cmsg->cmsg_len = CMSG_LEN(sizeof(struct in6_pktinfo)); struct in6_pktinfo *pktinfo = reinterpret_cast< struct in6_pktinfo * >(CMSG_DATA(cmsg)); memset(pktinfo, 0, sizeof(*pktinfo)); - memcpy(&pktinfo->ipi6_addr.s6_addr[0], &tcpha.qip6.c[0], sizeof(pktinfo->ipi6_addr.s6_addr)); + memcpy(&pktinfo->ipi6_addr.s6_addr[0], tcpha.getByteRepresentation().data(), + sizeof(pktinfo->ipi6_addr.s6_addr)); } else { cmsg->cmsg_level = IPPROTO_IP; cmsg->cmsg_type = IP_PKTINFO; @@ -1073,7 +1074,7 @@ void Server::sendMessage(ServerUser &u, const unsigned char *data, int len, QByt memset(pktinfo, 0, sizeof(*pktinfo)); if (tcpha.isV6()) return; - pktinfo->ipi_spec_dst.s_addr = tcpha.hash[3]; + pktinfo->ipi_spec_dst.s_addr = tcpha.toIPv4(); }