refactor: Use Bin_Pack for packing Node_format.

other/bootstrap_daemon/docker/tox-bootstrapd.sha256

@@ -1 +1 @@
-96618672392dc44a2f0a480f6415380e3fc588ace646345d31a297e80f7c271f  /usr/local/bin/tox-bootstrapd
+97015923b4bee5f557e2c8b60c766392c9610c661acf82e659044ffa536d3c9c  /usr/local/bin/tox-bootstrapd

toxcore/BUILD.bazel

@@ -285,6 +285,7 @@ cc_library(
     ],
     deps = [
         ":LAN_discovery",
+        ":bin_pack",
         ":ccompat",
         ":crypto_core",
         ":logger",

toxcore/DHT.c

@@ -9,10 +9,12 @@
 #include "DHT.h"
 
 #include <assert.h>
+#include <limits.h>
 #include <stdlib.h>
 #include <string.h>
 
 #include "LAN_discovery.h"
+#include "bin_pack.h"
 #include "ccompat.h"
 #include "logger.h"
 #include "mono_time.h"
@@ -452,19 +454,32 @@ int packed_node_size(Family ip_family)
 }
 
 
-/** @brief Pack an IP_Port structure into data of max size length.
+/** @brief Packs an IP structure.
  *
- * Packed_length is the offset of data currently packed.
+ * It's the caller's responsibility to make sure `is_ipv4` tells the truth. This
+ * function is an implementation detail of @ref bin_pack_ip_port.
  *
- * @return size of packed IP_Port data on success.
- * @retval -1 on failure.
+ * @param is_ipv4 whether this IP is an IP4 or IP6.
+ *
+ * @retval true on success.
  */
-int pack_ip_port(const Logger *logger, uint8_t *data, uint16_t length, const IP_Port *ip_port)
+non_null()
+static bool bin_pack_ip(Bin_Pack *bp, const IP *ip, bool is_ipv4)
 {
-    if (data == nullptr) {
-        return -1;
+    if (is_ipv4) {
+        return bin_pack_bin_b(bp, ip->ip.v4.uint8, SIZE_IP4);
+    } else {
+        return bin_pack_bin_b(bp, ip->ip.v6.uint8, SIZE_IP6);
     }
+}
 
+/** @brief Packs an IP_Port structure.
+ *
+ * @retval true on success.
+ */
+non_null()
+static bool bin_pack_ip_port(Bin_Pack *bp, const IP_Port *ip_port)
+{
     bool is_ipv4;
     uint8_t family;
 
@@ -482,36 +497,41 @@ 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];
-        // 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)));
-        return -1;
+        return false;
     }
 
-    if (is_ipv4) {
-        const uint32_t size = 1 + SIZE_IP4 + sizeof(uint16_t);
+    return bin_pack_u08_b(bp, family)
+           && bin_pack_ip(bp, &ip_port->ip, is_ipv4)
+           && bin_pack_u16_b(bp, net_ntohs(ip_port->port));
+}
 
-        if (size > length) {
-            return -1;
-        }
+non_null()
+static bool bin_pack_ip_port_handler(Bin_Pack *bp, const void *obj)
+{
+    return bin_pack_ip_port(bp, (const IP_Port *)obj);
+}
 
-        data[0] = family;
-        memcpy(data + 1, &ip_port->ip.ip.v4, SIZE_IP4);
-        memcpy(data + 1 + SIZE_IP4, &ip_port->port, sizeof(uint16_t));
-        return size;
-    } else {
-        const uint32_t size = 1 + SIZE_IP6 + sizeof(uint16_t);
+/** @brief Pack an IP_Port structure into data of max size length.
+ *
+ * Packed_length is the offset of data currently packed.
+ *
+ * @return size of packed IP_Port data on success.
+ * @retval -1 on failure.
+ */
+int pack_ip_port(uint8_t *data, uint16_t length, const IP_Port *ip_port)
+{
+    const uint32_t size = bin_pack_obj_size(bin_pack_ip_port_handler, ip_port);
 
-        if (size > length) {
-            return -1;
-        }
+    if (size > length) {
+        return -1;
+    }
 
-        data[0] = family;
-        memcpy(data + 1, &ip_port->ip.ip.v6, SIZE_IP6);
-        memcpy(data + 1 + SIZE_IP6, &ip_port->port, sizeof(uint16_t));
-        return size;
+    if (!bin_pack_obj(bin_pack_ip_port_handler, ip_port, data, length)) {
+        return -1;
     }
+
+    assert(size < INT_MAX);
+    return (int)size;
 }
 
 /** @brief Encrypt plain and write resulting DHT packet into packet with max size length.
@@ -560,6 +580,7 @@ int dht_create_packet(const Random *rng, const uint8_t public_key[CRYPTO_PUBLIC_
  * @return size of unpacked ip_port on success.
  * @retval -1 on failure.
  */
+non_null()
 int unpack_ip_port(IP_Port *ip_port, const uint8_t *data, uint16_t length, bool tcp_enabled)
 {
     if (data == nullptr) {
@@ -620,38 +641,30 @@ int unpack_ip_port(IP_Port *ip_port, const uint8_t *data, uint16_t length, bool
     }
 }
 
+/** @brief Pack a single node from a node array.
+ *
+ * @retval true on success.
+ */
+non_null()
+static bool bin_pack_node_handler(Bin_Pack *bp, const void *arr, uint32_t index)
+{
+    const Node_format *nodes = (const Node_format *)arr;
+    return bin_pack_ip_port(bp, &nodes[index].ip_port)
+           && bin_pack_bin_b(bp, nodes[index].public_key, CRYPTO_PUBLIC_KEY_SIZE);
+}
+
 /** @brief Pack number of nodes into data of maxlength length.
  *
  * @return length of packed nodes on success.
  * @retval -1 on failure.
  */
-int pack_nodes(const Logger *logger, uint8_t *data, uint16_t length, const Node_format *nodes, uint16_t number)
+int pack_nodes(uint8_t *data, uint16_t length, const Node_format *nodes, uint16_t number)
 {
-    uint32_t packed_length = 0;
-
-    for (uint32_t i = 0; i < number && packed_length < length; ++i) {
-        const int ipp_size = pack_ip_port(logger, data + packed_length, length - packed_length, &nodes[i].ip_port);
-
-        if (ipp_size == -1) {
-            return -1;
-        }
-
-        packed_length += ipp_size;
-
-        if (packed_length + CRYPTO_PUBLIC_KEY_SIZE > length) {
-            return -1;
-        }
-
-        memcpy(data + packed_length, nodes[i].public_key, CRYPTO_PUBLIC_KEY_SIZE);
-        packed_length += CRYPTO_PUBLIC_KEY_SIZE;
-
-#ifndef NDEBUG
-        const uint32_t increment = ipp_size + CRYPTO_PUBLIC_KEY_SIZE;
-#endif
-        assert(increment == PACKED_NODE_SIZE_IP4 || increment == PACKED_NODE_SIZE_IP6);
+    const uint32_t size = bin_pack_obj_array_size(bin_pack_node_handler, nodes, number);
+    if (!bin_pack_obj_array(bin_pack_node_handler, nodes, number, data, length)) {
+        return -1;
     }
-
-    return packed_length;
+    return size;
 }
 
 /** @brief Unpack data of length into nodes of size max_num_nodes.
@@ -1494,7 +1507,7 @@ bool dht_getnodes(DHT *dht, const IP_Port *ip_port, const uint8_t *public_key, c
     memcpy(receiver.public_key, public_key, CRYPTO_PUBLIC_KEY_SIZE);
     receiver.ip_port = *ip_port;
 
-    if (pack_nodes(dht->log, plain_message, sizeof(plain_message), &receiver, 1) == -1) {
+    if (pack_nodes(plain_message, sizeof(plain_message), &receiver, 1) == -1) {
         return false;
     }
 
@@ -1555,7 +1568,7 @@ static int sendnodes_ipv6(const DHT *dht, const IP_Port *ip_port, const uint8_t
     int nodes_length = 0;
 
     if (num_nodes > 0) {
-        nodes_length = pack_nodes(dht->log, plain + 1, node_format_size * MAX_SENT_NODES, nodes_list, num_nodes);
+        nodes_length = pack_nodes(plain + 1, node_format_size * MAX_SENT_NODES, nodes_list, num_nodes);
 
         if (nodes_length <= 0) {
             return -1;
@@ -2906,8 +2919,9 @@ void dht_save(const DHT *dht, uint8_t *data)
         }
     }
 
-    state_write_section_header(old_data, DHT_STATE_COOKIE_TYPE, pack_nodes(dht->log, data, sizeof(Node_format) * num,
-                               clients, num), DHT_STATE_TYPE_NODES);
+    state_write_section_header(
+            old_data, DHT_STATE_COOKIE_TYPE, pack_nodes(data, sizeof(Node_format) * num, clients, num),
+            DHT_STATE_TYPE_NODES);
 
     free(clients);
 }

toxcore/DHT.h

@@ -199,7 +199,17 @@ int packed_node_size(Family ip_family);
  * @retval -1 on failure.
  */
 non_null()
-int pack_ip_port(const Logger *logger, uint8_t *data, uint16_t length, const IP_Port *ip_port);
+int pack_ip_port(uint8_t *data, uint16_t length, const IP_Port *ip_port);
+
+/** @brief Unpack IP_Port structure from data of max size length into ip_port.
+ *
+ * len_processed is the offset of data currently unpacked.
+ *
+ * @return size of unpacked ip_port on success.
+ * @retval -1 on failure.
+ */
+non_null()
+int unpack_ip_port(IP_Port *ip_port, const uint8_t *data, uint16_t length, bool tcp_enabled);
 
 /** @brief Encrypt plain and write resulting DHT packet into packet with max size length.
  *
@@ -213,23 +223,13 @@ int dht_create_packet(const Random *rng,
                       const uint8_t *plain, size_t plain_length,
                       uint8_t *packet, size_t length);
 
-/** @brief Unpack IP_Port structure from data of max size length into ip_port.
- *
- * len_processed is the offset of data currently unpacked.
- *
- * @return size of unpacked ip_port on success.
- * @retval -1 on failure.
- */
-non_null()
-int unpack_ip_port(IP_Port *ip_port, const uint8_t *data, uint16_t length, bool tcp_enabled);
-
 /** @brief Pack number of nodes into data of maxlength length.
  *
  * @return length of packed nodes on success.
  * @retval -1 on failure.
  */
 non_null()
-int pack_nodes(const Logger *logger, uint8_t *data, uint16_t length, const Node_format *nodes, uint16_t number);
+int pack_nodes(uint8_t *data, uint16_t length, const Node_format *nodes, uint16_t number);
 
 /** @brief Unpack data of length into nodes of size max_num_nodes.
  * Put the length of the data processed in processed_data_len.

toxcore/DHT_fuzz_test.cc

@@ -1,6 +1,8 @@
 #include "DHT.h"
 
+#include <cassert>
 #include <cstdlib>
+#include <cstring>
 #include <vector>
 
 #include "../testing/fuzzing/fuzz_support.h"
@@ -31,11 +33,20 @@ void TestUnpackNodes(Fuzz_Data &input)
     if (packed_count > 0) {
         Logger *logger = logger_new();
         std::vector<uint8_t> packed(packed_count * PACKED_NODE_SIZE_IP6);
-        const int packed_size
-            = pack_nodes(logger, packed.data(), packed.size(), nodes, packed_count);
+        const int packed_size = pack_nodes(packed.data(), packed.size(), nodes, packed_count);
         LOGGER_ASSERT(logger, packed_size == processed_data_len,
             "packed size (%d) != unpacked size (%d)", packed_size, processed_data_len);
         logger_kill(logger);
+
+        // Check that packed nodes can be unpacked again and result in the
+        // original unpacked nodes.
+        Node_format nodes2[node_count];
+        uint16_t processed_data_len2;
+        const int packed_count2 = unpack_nodes(
+            nodes2, node_count, &processed_data_len2, packed.data(), packed.size(), tcp_enabled);
+        assert(processed_data_len2 == processed_data_len);
+        assert(packed_count2 == packed_count);
+        assert(memcmp(nodes, nodes2, sizeof(Node_format) * packed_count) == 0);
     }
 }
 

toxcore/Messenger.c

@@ -2981,7 +2981,7 @@ static uint8_t *save_tcp_relays(const Messenger *m, uint8_t *data)
     uint32_t num = m->num_loaded_relays;
     num += copy_connected_tcp_relays(m->net_crypto, relays + num, NUM_SAVED_TCP_RELAYS - num);
 
-    const int l = pack_nodes(m->log, data, NUM_SAVED_TCP_RELAYS * packed_node_size(net_family_tcp_ipv6), relays, num);
+    const int l = pack_nodes(data, NUM_SAVED_TCP_RELAYS * packed_node_size(net_family_tcp_ipv6), relays, num);
 
     if (l > 0) {
         const uint32_t len = l;
@@ -3025,7 +3025,7 @@ static uint8_t *save_path_nodes(const Messenger *m, uint8_t *data)
     data = state_write_section_header(data, STATE_COOKIE_TYPE, 0, STATE_TYPE_PATH_NODE);
     memset(nodes, 0, sizeof(nodes));
     const unsigned int num = onion_backup_nodes(m->onion_c, nodes, NUM_SAVED_PATH_NODES);
-    const int l = pack_nodes(m->log, data, NUM_SAVED_PATH_NODES * packed_node_size(net_family_tcp_ipv6), nodes, num);
+    const int l = pack_nodes(data, NUM_SAVED_PATH_NODES * packed_node_size(net_family_tcp_ipv6), nodes, num);
 
     if (l > 0) {
         const uint32_t len = l;

toxcore/TCP_client.c

@@ -542,7 +542,7 @@ int send_forward_request_tcp(const Logger *logger, TCP_Client_Connection *con, c
 
     VLA(uint8_t, packet, 1 + MAX_PACKED_IPPORT_SIZE + length);
     packet[0] = TCP_PACKET_FORWARD_REQUEST;
-    const int ipport_length = pack_ip_port(logger, packet + 1, MAX_PACKED_IPPORT_SIZE, dest);
+    const int ipport_length = pack_ip_port(packet + 1, MAX_PACKED_IPPORT_SIZE, dest);
 
     if (ipport_length == -1) {
         return 0;

toxcore/announce.c

@@ -275,7 +275,7 @@ static int create_data_search_to_auth(const Logger *logger, const uint8_t *data_
     memcpy(dest, data_public_key, CRYPTO_PUBLIC_KEY_SIZE);
     memcpy(dest + CRYPTO_PUBLIC_KEY_SIZE, requester_key, CRYPTO_PUBLIC_KEY_SIZE);
 
-    const int ipport_length = pack_ip_port(logger, dest + CRYPTO_PUBLIC_KEY_SIZE * 2, MAX_PACKED_IPPORT_SIZE, source);
+    const int ipport_length = pack_ip_port(dest + CRYPTO_PUBLIC_KEY_SIZE * 2, MAX_PACKED_IPPORT_SIZE, source);
 
     if (ipport_length == -1) {
         return -1;
@@ -353,7 +353,7 @@ static int create_reply_plain_data_search_request(Announcements *announce,
     *p = num_nodes;
     ++p;
 
-    p += pack_nodes(announce->log, p, nodes_max_length, nodes_list, num_nodes);
+    p += pack_nodes(p, nodes_max_length, nodes_list, num_nodes);
 
     const uint32_t reply_len = p - reply;
 

toxcore/bin_pack.c

@@ -77,6 +77,28 @@ uint32_t bin_pack_obj_size(bin_pack_cb *callback, const void *obj)
     return bp.bytes_pos;
 }
 
+uint32_t bin_pack_obj_array_size(bin_pack_array_cb *callback, const void *arr, uint32_t count)
+{
+    Bin_Pack bp;
+    bin_pack_init(&bp, nullptr, 0);
+    for (uint32_t i = 0; i < count; ++i) {
+        callback(&bp, arr, i);
+    }
+    return bp.bytes_pos;
+}
+
+bool bin_pack_obj_array(bin_pack_array_cb *callback, const void *arr, uint32_t count, uint8_t *buf, uint32_t buf_size)
+{
+    Bin_Pack bp;
+    bin_pack_init(&bp, buf, buf_size);
+    for (uint32_t i = 0; i < count; ++i) {
+        if (!callback(&bp, arr, i)) {
+            return false;
+        }
+    }
+    return true;
+}
+
 Bin_Pack *bin_pack_new(uint8_t *buf, uint32_t buf_size)
 {
     Bin_Pack *bp = (Bin_Pack *)calloc(1, sizeof(Bin_Pack));