From fca3862ced8c14dd38d4b009e20065b3d5afb40e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Fri, 17 Jan 2025 21:25:29 +0100 Subject: [PATCH] Minor refactoring of `net_addr_str` function Replace mostly unused `bool` return value with assertion to ensure the address has a valid type instead of outputting `unknown type` as the address. Also test the assertion failure by changing the existing test case to death tests. See https://google.github.io/googletest/advanced.html#death-tests for details. The death test names must end with `DeathTest` so GoogleTest runs these test cases before all other tests. Fix assertion failure in client due to previous server address being invalid when connecting to the first server by comparing addresses directly instead of converting the invalid address to a string first. Use `bool` instead of `int` for the `add_port` parameter. Minor improvement of doxygen comment. --- src/base/system.cpp | 23 +++++++++++------------ src/base/system.h | 8 +++----- src/engine/client/client.cpp | 8 ++++---- src/engine/client/serverbrowser.cpp | 6 +----- src/test/netaddr.cpp | 25 +++++++++++++++++-------- src/tools/stun.cpp | 2 +- 6 files changed, 37 insertions(+), 35 deletions(-) diff --git a/src/base/system.cpp b/src/base/system.cpp index 62d70859aee..35596d7339e 100644 --- a/src/base/system.cpp +++ b/src/base/system.cpp @@ -1144,36 +1144,35 @@ void net_addr_str_v6(const unsigned short ip[8], int port, char *buffer, int buf } } -bool net_addr_str(const NETADDR *addr, char *string, int max_length, int add_port) +void net_addr_str(const NETADDR *addr, char *string, int max_length, bool add_port) { if(addr->type & NETTYPE_IPV4 || addr->type & NETTYPE_WEBSOCKET_IPV4) { - if(add_port != 0) + if(add_port) + { str_format(string, max_length, "%d.%d.%d.%d:%d", addr->ip[0], addr->ip[1], addr->ip[2], addr->ip[3], addr->port); + } else + { str_format(string, max_length, "%d.%d.%d.%d", addr->ip[0], addr->ip[1], addr->ip[2], addr->ip[3]); + } } else if(addr->type & NETTYPE_IPV6) { - int port = -1; unsigned short ip[8]; - int i; - if(add_port) - { - port = addr->port; - } - for(i = 0; i < 8; i++) + for(int i = 0; i < 8; i++) { ip[i] = (addr->ip[i * 2] << 8) | (addr->ip[i * 2 + 1]); } + int port = add_port ? addr->port : -1; net_addr_str_v6(ip, port, string, max_length); } else { - str_format(string, max_length, "unknown type %d", addr->type); - return false; + char error[64]; + str_format(error, sizeof(error), "unknown NETADDR type %d", addr->type); + dbg_assert(false, error); } - return true; } static int priv_net_extract(const char *hostname, char *host, int max_host, int *port) diff --git a/src/base/system.h b/src/base/system.h index 39fb784ce6e..86f5b4d0d06 100644 --- a/src/base/system.h +++ b/src/base/system.h @@ -865,13 +865,11 @@ int net_addr_comp_noport(const NETADDR *a, const NETADDR *b); * @param addr Address to turn into a string. * @param string Buffer to fill with the string. * @param max_length Maximum size of the string. - * @param add_port add port to string or not + * @param add_port Whether or not to add the port to the string. * - * @return true on success - * - * @remark The string will always be zero terminated + * @remark The string will always be zero terminated. */ -bool net_addr_str(const NETADDR *addr, char *string, int max_length, int add_port); +void net_addr_str(const NETADDR *addr, char *string, int max_length, bool add_port); /** * Turns url string into a network address struct. diff --git a/src/engine/client/client.cpp b/src/engine/client/client.cpp index 1f1cb3dd23d..a19b620d9b4 100644 --- a/src/engine/client/client.cpp +++ b/src/engine/client/client.cpp @@ -540,8 +540,7 @@ void CClient::Connect(const char *pAddress, const char *pPassword) Disconnect(); dbg_assert(m_State == IClient::STATE_OFFLINE, "Disconnect must ensure that client is offline"); - char aLastAddr[NETADDR_MAXSTRSIZE]; - net_addr_str(&ServerAddress(), aLastAddr, sizeof(aLastAddr), true); + const NETADDR LastAddr = ServerAddress(); if(pAddress != m_aConnectAddressStr) str_copy(m_aConnectAddressStr, pAddress); @@ -579,15 +578,16 @@ void CClient::Connect(const char *pAddress, const char *pPassword) { NextAddr.port = 8303; } - char aNextAddr[NETADDR_MAXSTRSIZE]; if(Sixup) NextAddr.type |= NETTYPE_TW7; else OnlySixup = false; + + char aNextAddr[NETADDR_MAXSTRSIZE]; net_addr_str(&NextAddr, aNextAddr, sizeof(aNextAddr), true); log_debug("client", "resolved connect address '%s' to %s", aBuffer, aNextAddr); - if(!str_comp(aNextAddr, aLastAddr)) + if(NextAddr == LastAddr) { m_SendPassword = true; } diff --git a/src/engine/client/serverbrowser.cpp b/src/engine/client/serverbrowser.cpp index 07cd529da78..76a1f555af7 100644 --- a/src/engine/client/serverbrowser.cpp +++ b/src/engine/client/serverbrowser.cpp @@ -702,11 +702,7 @@ void ServerBrowserFormatAddresses(char *pBuffer, int BufferSize, NETADDR *pAddrs return; } char aIpAddr[512]; - if(!net_addr_str(&pAddrs[i], aIpAddr, sizeof(aIpAddr), true)) - { - str_copy(pBuffer, aIpAddr, BufferSize); - return; - } + net_addr_str(&pAddrs[i], aIpAddr, sizeof(aIpAddr), true); if(pAddrs[i].type & NETTYPE_TW7) { str_format( diff --git a/src/test/netaddr.cpp b/src/test/netaddr.cpp index 7b199e39322..c9e5fd26a2f 100644 --- a/src/test/netaddr.cpp +++ b/src/test/netaddr.cpp @@ -134,13 +134,22 @@ TEST(NetAddr, FromStrInvalid) EXPECT_TRUE(net_addr_from_str(&Addr, "[::]:c")); } -TEST(NetAddr, StrInvalid) +TEST(NetAddrDeathTest, StrInvalid1) { - NETADDR Addr = {0}; - char aBuf1[NETADDR_MAXSTRSIZE]; - char aBuf2[NETADDR_MAXSTRSIZE]; - net_addr_str(&Addr, aBuf1, sizeof(aBuf1), true); - EXPECT_STREQ(aBuf1, "unknown type 0"); - net_addr_str(&Addr, aBuf2, sizeof(aBuf2), false); - EXPECT_STREQ(aBuf2, "unknown type 0"); + ASSERT_DEATH({ + NETADDR Addr = {0}; + char aBuf[NETADDR_MAXSTRSIZE]; + net_addr_str(&Addr, aBuf, sizeof(aBuf), true); + }, + ""); +} + +TEST(NetAddrDeathTest, StrInvalid2) +{ + ASSERT_DEATH({ + NETADDR Addr = {0}; + char aBuf[NETADDR_MAXSTRSIZE]; + net_addr_str(&Addr, aBuf, sizeof(aBuf), false); + }, + ""); } diff --git a/src/tools/stun.cpp b/src/tools/stun.cpp index 4329f575ede..1e2e6cd4f8a 100644 --- a/src/tools/stun.cpp +++ b/src/tools/stun.cpp @@ -83,7 +83,7 @@ int main(int argc, const char **argv) if(Success) { char aStunAddr[NETADDR_MAXSTRSIZE]; - net_addr_str(&StunAddr, aStunAddr, sizeof(aStunAddr), 1); + net_addr_str(&StunAddr, aStunAddr, sizeof(aStunAddr), true); log_info("stun", "public ip address: %s", aStunAddr); break; }