Skip to content

Commit

Permalink
cleanup: Use a struct for the ip_ntoa buffer.
Browse files Browse the repository at this point in the history 
Every use of this function needs to allocate the same buffer. None of
the callers uses a differently sized buffer, so we might as well put it
in a struct and have the type checker prove the buffer size is correct.

Also rename `ip_ntoa` to `net_ip_ntoa` to avoid clashes with ESP-IDF
system libraries which define this function as well.
  • Loading branch information
@iphydf
iphydf committed Apr 3, 2022
1 parent 378febf commit 45a5e19
Show file tree
Hide file tree
Showing 8 changed files with 70 additions and 81 deletions.
12 changes: 6 additions & 6 deletions auto_tests/network_test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ static void test_addr_resolv_localhost(void)
ck_assert_msg(res, "Resolver failed: %d, %s", error, strerror);
net_kill_strerror(strerror);

char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str;
ck_assert_msg(net_family_is_ipv4(ip.family), "Expected family TOX_AF_INET, got %u.", ip.family.value);
const uint32_t loopback = get_ip4_loopback().uint32;
ck_assert_msg(ip.ip.v4.uint32 == loopback, "Expected 127.0.0.1, got %s.",
ip_ntoa(&ip, ip_str, sizeof(ip_str)));
net_ip_ntoa(&ip, &ip_str));

ip_init(&ip, 1); // ipv6enabled = 1
res = addr_resolve_or_parse_ip(ns, localhost, &ip, nullptr);
Expand All @@ -62,7 +62,7 @@ static void test_addr_resolv_localhost(void)
ip.family.value);
IP6 ip6_loopback = get_ip6_loopback();
ck_assert_msg(!memcmp(&ip.ip.v6, &ip6_loopback, sizeof(IP6)), "Expected ::1, got %s.",
ip_ntoa(&ip, ip_str, sizeof(ip_str)));
net_ip_ntoa(&ip, &ip_str));

if (localhost_split) {
printf("Localhost seems to be split in two.\n");
Expand All @@ -85,17 +85,17 @@ static void test_addr_resolv_localhost(void)
ck_assert_msg(net_family_is_ipv6(ip.family), "Expected family TOX_AF_INET6 (%d), got %u.", TOX_AF_INET6,
ip.family.value);
ck_assert_msg(!memcmp(&ip.ip.v6, &ip6_loopback, sizeof(IP6)), "Expected ::1, got %s.",
ip_ntoa(&ip, ip_str, sizeof(ip_str)));
net_ip_ntoa(&ip, &ip_str));

ck_assert_msg(net_family_is_ipv4(extra.family), "Expected family TOX_AF_INET (%d), got %u.", TOX_AF_INET,
extra.family.value);
ck_assert_msg(extra.ip.v4.uint32 == loopback, "Expected 127.0.0.1, got %s.",
ip_ntoa(&ip, ip_str, sizeof(ip_str)));
net_ip_ntoa(&ip, &ip_str));
#elif 0
// TODO(iphydf): Fix this to work on IPv6-supporting systems.
ck_assert_msg(net_family_is_ipv4(ip.family), "Expected family TOX_AF_INET (%d), got %u.", TOX_AF_INET, ip.family.value);
ck_assert_msg(ip.ip.v4.uint32 == loopback, "Expected 127.0.0.1, got %s.",
ip_ntoa(&ip, ip_str, sizeof(ip_str)));
net_ip_ntoa(&ip, &ip_str));
#endif
}

Expand Down
2 changes: 1 addition & 1 deletion other/bootstrap_daemon/docker/tox-bootstrapd.sha256
View file
Original file line number Diff line number Diff line change
@@ -1 +1 @@
60f76a2186014870804b8dbacc16c68e8b019e02699584d007327a72de9e53de /usr/local/bin/tox-bootstrapd
1d456899c46201827710b274b820a906f7a8da229b2fad3cfde68b33fbe59ae2 /usr/local/bin/tox-bootstrapd
11 changes: 6 additions & 5 deletions toxcore/DHT.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -482,10 +482,10 @@ int pack_ip_port(const Logger *logger, uint8_t *data, uint16_t length, const IP_
is_ipv4 = false;
family = TOX_TCP_INET6;
} else {
char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str;
// TODO(iphydf): Find out why we're trying to pack invalid IPs, stop
// doing that, and turn this into an error.
LOGGER_TRACE(logger, "cannot pack invalid IP: %s", ip_ntoa(&ip_port->ip, ip_str, sizeof(ip_str)));
LOGGER_TRACE(logger, "cannot pack invalid IP: %s", net_ip_ntoa(&ip_port->ip, &ip_str));
return -1;
}

Expand Down Expand Up @@ -781,12 +781,13 @@ static void update_client(const Logger *log, const Mono_Time *mono_time, int ind
}

if (!ipport_equal(&assoc->ip_port, ip_port)) {
char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str_from;
Ip_Ntoa ip_str_to;
LOGGER_TRACE(log, "coipil[%u]: switching ipv%d from %s:%u to %s:%u",
index, ip_version,
ip_ntoa(&assoc->ip_port.ip, ip_str, sizeof(ip_str)),
net_ip_ntoa(&assoc->ip_port.ip, &ip_str_from),
net_ntohs(assoc->ip_port.port),
ip_ntoa(&ip_port->ip, ip_str, sizeof(ip_str)),
net_ip_ntoa(&ip_port->ip, &ip_str_to),
net_ntohs(ip_port->port));
}

Expand Down
8 changes: 4 additions & 4 deletions toxcore/Messenger.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2419,10 +2419,10 @@ void do_messenger(Messenger *m, void *userdata)
last_pinged = 999;
}

char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str;
char id_str[IDSTRING_LEN];
LOGGER_TRACE(m->log, "C[%2u] %s:%u [%3u] %s",
client, ip_ntoa(&assoc->ip_port.ip, ip_str, sizeof(ip_str)),
client, net_ip_ntoa(&assoc->ip_port.ip, &ip_str),
net_ntohs(assoc->ip_port.port), last_pinged,
id_to_string(cptr->public_key, id_str, sizeof(id_str)));
}
Expand Down Expand Up @@ -2492,10 +2492,10 @@ void do_messenger(Messenger *m, void *userdata)
last_pinged = 999;
}

char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str;
char id_str[IDSTRING_LEN];
LOGGER_TRACE(m->log, "F[%2u] => C[%2u] %s:%u [%3u] %s",
friend_idx, client, ip_ntoa(&assoc->ip_port.ip, ip_str, sizeof(ip_str)),
friend_idx, client, net_ip_ntoa(&assoc->ip_port.ip, &ip_str),
net_ntohs(assoc->ip_port.port), last_pinged,
id_to_string(cptr->public_key, id_str, sizeof(id_str)));
}
Expand Down
61 changes: 30 additions & 31 deletions toxcore/network.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -670,25 +670,25 @@ static void loglogdata(const Logger *log, const char *message, const uint8_t *bu
uint16_t buflen, const IP_Port *ip_port, long res)
{
if (res < 0) { /* Windows doesn't necessarily know `%zu` */
char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str;
const int error = net_error();
char *strerror = net_new_strerror(error);
LOGGER_TRACE(log, "[%2u] %s %3u%c %s:%u (%u: %s) | %08x%08x...%02x",
buffer[0], message, min_u16(buflen, 999), 'E',
ip_ntoa(&ip_port->ip, ip_str, sizeof(ip_str)), net_ntohs(ip_port->port), error,
net_ip_ntoa(&ip_port->ip, &ip_str), net_ntohs(ip_port->port), error,
strerror, data_0(buflen, buffer), data_1(buflen, buffer), buffer[buflen - 1]);
net_kill_strerror(strerror);
} else if ((res > 0) && ((size_t)res <= buflen)) {
char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str;
LOGGER_TRACE(log, "[%2u] %s %3u%c %s:%u (%u: %s) | %08x%08x...%02x",
buffer[0], message, min_u16(res, 999), (size_t)res < buflen ? '<' : '=',
ip_ntoa(&ip_port->ip, ip_str, sizeof(ip_str)), net_ntohs(ip_port->port), 0, "OK",
net_ip_ntoa(&ip_port->ip, &ip_str), net_ntohs(ip_port->port), 0, "OK",
data_0(buflen, buffer), data_1(buflen, buffer), buffer[buflen - 1]);
} else { /* empty or overwrite */
char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str;
LOGGER_TRACE(log, "[%2u] %s %lu%c%u %s:%u (%u: %s) | %08x%08x...%02x",
buffer[0], message, res, res == 0 ? '!' : '>', buflen,
ip_ntoa(&ip_port->ip, ip_str, sizeof(ip_str)), net_ntohs(ip_port->port), 0, "OK",
net_ip_ntoa(&ip_port->ip, &ip_str), net_ntohs(ip_port->port), 0, "OK",
data_0(buflen, buffer), data_1(buflen, buffer), buffer[buflen - 1]);
}
}
Expand Down Expand Up @@ -1198,8 +1198,8 @@ Networking_Core *new_networking_ex(
if (res == 0) {
temp->port = *portptr;

char ip_str[IP_NTOA_LEN];
LOGGER_DEBUG(log, "Bound successfully to %s:%u", ip_ntoa(ip, ip_str, sizeof(ip_str)),
Ip_Ntoa ip_str;
LOGGER_DEBUG(log, "Bound successfully to %s:%u", net_ip_ntoa(ip, &ip_str),
net_ntohs(temp->port));

/* errno isn't reset on success, only set on failure, the failed
Expand All @@ -1225,11 +1225,11 @@ Networking_Core *new_networking_ex(
*portptr = net_htons(port_to_try);
}

char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str;
int neterror = net_error();
char *strerror = net_new_strerror(neterror);
LOGGER_ERROR(log, "failed to bind socket: %d, %s IP: %s port_from: %u port_to: %u", neterror, strerror,
ip_ntoa(ip, ip_str, sizeof(ip_str)), port_from, port_to);
net_ip_ntoa(ip, &ip_str), port_from, port_to);
net_kill_strerror(strerror);
kill_networking(temp);

Expand Down Expand Up @@ -1402,34 +1402,31 @@ void ipport_copy(IP_Port *target, const IP_Port *source)
*target = *source;
}

/** @brief converts ip into a string
/** @brief Converts IP into a string.
*
* @param ip_str must be of length at least IP_NTOA_LEN
* Writes error message into the buffer on error.
*
* writes error message into the buffer on error
* @param ip_str contains a buffer of the required size.
*
* @return ip_str
* @return Pointer to the buffer inside `ip_str` containing the IP string.
*/
const char *ip_ntoa(const IP *ip, char *ip_str, size_t length)
const char *net_ip_ntoa(const IP *ip, Ip_Ntoa *ip_str)
{
if (length < IP_NTOA_LEN) {
snprintf(ip_str, length, "Bad buf length");
return ip_str;
}
assert(ip_str != nullptr);

if (ip == nullptr) {
snprintf(ip_str, length, "(IP invalid: NULL)");
return ip_str;
snprintf(ip_str->buf, sizeof(ip_str->buf), "(IP invalid: NULL)");
return ip_str->buf;
}

if (!ip_parse_addr(ip, ip_str, length)) {
snprintf(ip_str, length, "(IP invalid, family %u)", ip->family.value);
return ip_str;
if (!ip_parse_addr(ip, ip_str->buf, sizeof(ip_str->buf))) {
snprintf(ip_str->buf, sizeof(ip_str->buf), "(IP invalid, family %u)", ip->family.value);
return ip_str->buf;
}

/* brute force protection against lacking termination */
ip_str[length - 1] = '\0';
return ip_str;
ip_str->buf[sizeof(ip_str->buf) - 1] = '\0';
return ip_str->buf;
}

bool ip_parse_addr(const IP *ip, char *address, size_t length)
Expand Down Expand Up @@ -1611,8 +1608,6 @@ bool net_connect(const Logger *log, Socket sock, const IP_Port *ip_port)
{
struct sockaddr_storage addr = {0};
size_t addrsize;
char ip_str[IP_NTOA_LEN];
ip_ntoa(&ip_port->ip, ip_str, sizeof(ip_str));

if (net_family_is_ipv4(ip_port->ip.family)) {
struct sockaddr_in *addr4 = (struct sockaddr_in *)&addr;
Expand All @@ -1629,15 +1624,18 @@ bool net_connect(const Logger *log, Socket sock, const IP_Port *ip_port)
fill_addr6(&ip_port->ip.ip.v6, &addr6->sin6_addr);
addr6->sin6_port = ip_port->port;
} else {
LOGGER_ERROR(log, "cannot connect to %s:%d which is neither IPv4 nor IPv6", ip_str, ip_port->port);
Ip_Ntoa ip_str;
LOGGER_ERROR(log, "cannot connect to %s:%d which is neither IPv4 nor IPv6",
net_ip_ntoa(&ip_port->ip, &ip_str), ip_port->port);
return false;
}

#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
return addrsize != 0;
#else
Ip_Ntoa ip_str;
LOGGER_DEBUG(log, "connecting socket %d to %s:%d",
(int)sock.sock, ip_ntoa(&ip_port->ip, ip_str, sizeof(ip_str)), net_ntohs(ip_port->port));
(int)sock.sock, net_ip_ntoa(&ip_port->ip, &ip_str), net_ntohs(ip_port->port));
errno = 0;

if (connect(sock.sock, (struct sockaddr *)&addr, addrsize) == -1) {
Expand All @@ -1646,7 +1644,8 @@ bool net_connect(const Logger *log, Socket sock, const IP_Port *ip_port)
// Non-blocking socket: "Operation in progress" means it's connecting.
if (!should_ignore_connect_error(error)) {
char *net_strerror = net_new_strerror(error);
LOGGER_ERROR(log, "failed to connect to %s:%d: %d (%s)", ip_str, ip_port->port, error, net_strerror);
LOGGER_ERROR(log, "failed to connect to %s:%d: %d (%s)",
ip_str.buf, ip_port->port, error, net_strerror);
net_kill_strerror(net_strerror);
return false;
}
Expand Down
15 changes: 10 additions & 5 deletions toxcore/network.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,16 +328,21 @@ bool ipv6_ipv4_in_v6(const IP6 *a);

/** this would be TOX_INET6_ADDRSTRLEN, but it might be too short for the error message */
#define IP_NTOA_LEN 96 // TODO(irungentoo): magic number. Why not INET6_ADDRSTRLEN ?
/** @brief converts ip into a string

typedef struct Ip_Ntoa {
char buf[IP_NTOA_LEN];
} Ip_Ntoa;

/** @brief Converts IP into a string.
*
* @param ip_str must be of length at least IP_NTOA_LEN
* Writes error message into the buffer on error.
*
* writes error message into the buffer on error
* @param ip_str contains a buffer of the required size.
*
* @return ip_str
* @return Pointer to the buffer inside `ip_str` containing the IP string.
*/
non_null()
const char *ip_ntoa(const IP *ip, char *ip_str, size_t length);
const char *net_ip_ntoa(const IP *ip, Ip_Ntoa *ip_str);

/**
* Parses IP structure into an address string.
Expand Down
35 changes: 10 additions & 25 deletions toxcore/network_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,58 +6,43 @@ namespace {

TEST(IpNtoa, DoesntWriteOutOfBounds)
{
char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str;
IP ip;
ip.family = net_family_ipv6;
ip.ip.v6.uint64[0] = -1;
ip.ip.v6.uint64[1] = -1;

ip_ntoa(&ip, ip_str, sizeof(ip_str));

EXPECT_EQ(std::string(ip_str), "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff");
EXPECT_LT(std::string(ip_str).length(), IP_NTOA_LEN);
}

TEST(IpNtoa, DoesntOverrunSmallBuffer)
{
// We have to try really hard to trick the compiler here not to realise that
// 10 is too small a buffer for the snprintf inside ip_ntoa and error out.
std::istringstream ss("10");
size_t len;
ss >> len;
std::vector<char> ip_str(len);
IP *ip = nullptr;

ip_ntoa(ip, ip_str.data(), ip_str.size());
net_ip_ntoa(&ip, &ip_str);

EXPECT_EQ(std::string(ip_str.data()), "Bad buf l");
EXPECT_EQ(std::string(ip_str.buf), "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff");
EXPECT_LT(std::string(ip_str.buf).length(), IP_NTOA_LEN);
}

TEST(IpNtoa, ReportsInvalidIpFamily)
{
char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str;
IP ip;
ip.family.value = 255 - net_family_ipv6.value;
ip.ip.v4.uint32 = 0;

ip_ntoa(&ip, ip_str, sizeof(ip_str));
net_ip_ntoa(&ip, &ip_str);

EXPECT_EQ(std::string(ip_str), "(IP invalid, family 245)");
EXPECT_EQ(std::string(ip_str.buf), "(IP invalid, family 245)");
}

TEST(IpNtoa, FormatsIPv4)
{
char ip_str[IP_NTOA_LEN];
Ip_Ntoa ip_str;
IP ip;
ip.family = net_family_ipv4;
ip.ip.v4.uint8[0] = 192;
ip.ip.v4.uint8[1] = 168;
ip.ip.v4.uint8[2] = 0;
ip.ip.v4.uint8[3] = 13;

ip_ntoa(&ip, ip_str, sizeof(ip_str));
net_ip_ntoa(&ip, &ip_str);

EXPECT_EQ(std::string(ip_str), "192.168.0.13");
EXPECT_EQ(std::string(ip_str.buf), "192.168.0.13");
}

TEST(IpParseAddr, FormatsIPv4)
Expand Down
7 changes: 3 additions & 4 deletions toxcore/tox.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,10 +315,9 @@ static void tox_dht_get_nodes_response_handler(const DHT *dht, const Node_format
return;
}

char ip[IP_NTOA_LEN];
ip_ntoa(&node->ip_port.ip, ip, sizeof(ip));

tox_data->tox->dht_get_nodes_response_callback(tox_data->tox, node->public_key, ip, net_ntohs(node->ip_port.port),
Ip_Ntoa ip_str;
tox_data->tox->dht_get_nodes_response_callback(
tox_data->tox, node->public_key, net_ip_ntoa(&node->ip_port.ip, &ip_str), net_ntohs(node->ip_port.port),
tox_data->user_data);
}

Expand Down

0 comments on commit 45a5e19

Please sign in to comment.