From f41e3f0b50d66d5cb38034f95a2115cd3fccf1e8 Mon Sep 17 00:00:00 2001 From: iphydf Date: Wed, 30 Mar 2022 23:10:44 +0000 Subject: [PATCH] refactor: Use Bin_Pack for packing Node_format. --- .../docker/tox-bootstrapd.sha256 | 2 +- toxcore/BUILD.bazel | 44 ++++--- toxcore/DHT.c | 121 ++++++++++-------- toxcore/DHT.h | 20 +-- toxcore/DHT_fuzz_test.cc | 12 ++ toxcore/bin_pack.c | 34 ++++- toxcore/bin_pack.h | 67 +++++++++- toxcore/tox_events.c | 8 +- 8 files changed, 210 insertions(+), 98 deletions(-) diff --git a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 index ac5852d8bd7..4a47cfff8dd 100644 --- a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 +++ b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 @@ -1 +1 @@ -39a74721e1730757bdb96020acebe25bafb450a4298e4bc9556e3ad2d59260b1 /usr/local/bin/tox-bootstrapd +a9779b7c0cf68947fed84adf3e52c8e259e1119aff5679a61ed4d5ea8f2a217e /usr/local/bin/tox-bootstrapd diff --git a/toxcore/BUILD.bazel b/toxcore/BUILD.bazel index 0656707d157..823e9791d92 100644 --- a/toxcore/BUILD.bazel +++ b/toxcore/BUILD.bazel @@ -15,6 +15,27 @@ cc_library( visibility = ["//c-toxcore:__subpackages__"], ) +cc_library( + name = "ccompat", + srcs = ["ccompat.c"], + hdrs = ["ccompat.h"], + visibility = ["//c-toxcore:__subpackages__"], + deps = [":attributes"], +) + +cc_library( + name = "logger", + srcs = ["logger.c"], + hdrs = ["logger.h"], + visibility = [ + "//c-toxcore/auto_tests:__pkg__", + "//c-toxcore/other:__pkg__", + "//c-toxcore/other/bootstrap_daemon:__pkg__", + "//c-toxcore/toxav:__pkg__", + ], + deps = [":ccompat"], +) + cc_library( name = "bin_pack", srcs = ["bin_pack.c"], @@ -22,6 +43,7 @@ cc_library( visibility = ["//c-toxcore:__subpackages__"], deps = [ ":ccompat", + ":logger", "//c-toxcore/third_party:cmp", ], ) @@ -49,14 +71,6 @@ cc_test( ], ) -cc_library( - name = "ccompat", - srcs = ["ccompat.c"], - hdrs = ["ccompat.h"], - visibility = ["//c-toxcore:__subpackages__"], - deps = [":attributes"], -) - cc_library( name = "crypto_core", srcs = ["crypto_core.c"], @@ -100,19 +114,6 @@ cc_test( ], ) -cc_library( - name = "logger", - srcs = ["logger.c"], - hdrs = ["logger.h"], - visibility = [ - "//c-toxcore/auto_tests:__pkg__", - "//c-toxcore/other:__pkg__", - "//c-toxcore/other/bootstrap_daemon:__pkg__", - "//c-toxcore/toxav:__pkg__", - ], - deps = [":ccompat"], -) - cc_library( name = "state", srcs = ["state.c"], @@ -285,6 +286,7 @@ cc_library( ], deps = [ ":LAN_discovery", + ":bin_pack", ":ccompat", ":crypto_core", ":logger", diff --git a/toxcore/DHT.c b/toxcore/DHT.c index 2d92af7669d..398f8fc3e1f 100644 --- a/toxcore/DHT.c +++ b/toxcore/DHT.c @@ -9,10 +9,12 @@ #include "DHT.h" #include +#include #include #include #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 Logger *logger, const IP_Port *ip_port) +{ bool is_ipv4; uint8_t family; @@ -486,32 +501,41 @@ int pack_ip_port(const Logger *logger, uint8_t *data, uint16_t length, const IP_ // 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", net_ip_ntoa(&ip_port->ip, &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 Logger *logger, const void *obj) +{ + return bin_pack_ip_port(bp, logger, (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(const Logger *logger, uint8_t *data, uint16_t length, const IP_Port *ip_port) +{ + const uint32_t size = bin_pack_obj_size(bin_pack_ip_port_handler, logger, 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, logger, 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. @@ -620,6 +644,18 @@ 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 Logger *logger, const void *arr, uint32_t index) +{ + const Node_format *nodes = (const Node_format *)arr; + return bin_pack_ip_port(bp, logger, &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. @@ -627,31 +663,11 @@ int unpack_ip_port(IP_Port *ip_port, const uint8_t *data, uint16_t length, bool */ int pack_nodes(const Logger *logger, 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, logger, nodes, number); + if (!bin_pack_obj_array(bin_pack_node_handler, logger, nodes, number, data, length)) { + return -1; } - - return packed_length; + return size; } /** @brief Unpack data of length into nodes of size max_num_nodes. @@ -2907,8 +2923,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(dht->log, data, sizeof(Node_format) * num, clients, num), + DHT_STATE_TYPE_NODES); free(clients); } diff --git a/toxcore/DHT.h b/toxcore/DHT.h index 0109ed1b2ec..bb34927ec59 100644 --- a/toxcore/DHT.h +++ b/toxcore/DHT.h @@ -201,6 +201,16 @@ int packed_node_size(Family ip_family); non_null() int pack_ip_port(const Logger *logger, 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. * * @return size of packet on success. @@ -213,16 +223,6 @@ 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. diff --git a/toxcore/DHT_fuzz_test.cc b/toxcore/DHT_fuzz_test.cc index e9673ae0ee5..a978fddd2a4 100644 --- a/toxcore/DHT_fuzz_test.cc +++ b/toxcore/DHT_fuzz_test.cc @@ -1,6 +1,8 @@ #include "DHT.h" +#include #include +#include #include #include "../testing/fuzzing/fuzz_support.h" @@ -36,6 +38,16 @@ void TestUnpackNodes(Fuzz_Data &input) 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); } } diff --git a/toxcore/bin_pack.c b/toxcore/bin_pack.c index ca8940ee448..ce16d9e261a 100644 --- a/toxcore/bin_pack.c +++ b/toxcore/bin_pack.c @@ -62,21 +62,47 @@ static void bin_pack_init(Bin_Pack *bp, uint8_t *buf, uint32_t buf_size) cmp_init(&bp->ctx, bp, null_reader, null_skipper, buf_writer); } -bool bin_pack_obj(bin_pack_cb *callback, const void *obj, uint8_t *buf, uint32_t buf_size) +uint32_t bin_pack_obj_size(bin_pack_cb *callback, const Logger *logger, const void *obj) +{ + Bin_Pack bp; + bin_pack_init(&bp, nullptr, 0); + if (!callback(&bp, logger, obj)) { + return UINT32_MAX; + } + return bp.bytes_pos; +} + +bool bin_pack_obj(bin_pack_cb *callback, const Logger *logger, const void *obj, uint8_t *buf, uint32_t buf_size) { Bin_Pack bp; bin_pack_init(&bp, buf, buf_size); - return callback(&bp, obj); + return callback(&bp, logger, obj); } -uint32_t bin_pack_obj_size(bin_pack_cb *callback, const void *obj) +uint32_t bin_pack_obj_array_size(bin_pack_array_cb *callback, const Logger *logger, const void *arr, uint32_t count) { Bin_Pack bp; bin_pack_init(&bp, nullptr, 0); - callback(&bp, obj); + for (uint32_t i = 0; i < count; ++i) { + if (!callback(&bp, logger, arr, i)) { + return UINT32_MAX; + } + } return bp.bytes_pos; } +bool bin_pack_obj_array(bin_pack_array_cb *callback, const Logger *logger, 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, logger, 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)); diff --git a/toxcore/bin_pack.h b/toxcore/bin_pack.h index 542f533b883..205e084d36c 100644 --- a/toxcore/bin_pack.h +++ b/toxcore/bin_pack.h @@ -8,6 +8,7 @@ #include #include "attributes.h" +#include "logger.h" #ifdef __cplusplus extern "C" { @@ -23,18 +24,29 @@ typedef struct Bin_Pack Bin_Pack; * This function would typically cast the `void *` to the actual object pointer type and then call * more appropriately typed packing functions. */ -typedef bool bin_pack_cb(Bin_Pack *bp, const void *obj); +typedef bool bin_pack_cb(Bin_Pack *bp, const Logger *logger, const void *obj); + +/** @brief Function used to pack an array of objects. + * + * This function would typically cast the `void *` to the actual object pointer type and then call + * more appropriately typed packing functions. + * + * @param arr is the object array as void pointer. + * @param index is the index in the object array that is currently being packed. + */ +typedef bool bin_pack_array_cb(Bin_Pack *bp, const Logger *logger, const void *arr, uint32_t index); /** @brief Determine the serialised size of an object. * * @param callback The function called on the created packer and packed object. + * @param logger Optional logger object to pass to the callback. * @param obj The object to be packed, passed as `obj` to the callback. * - * @return The packed size of the passed object according to the callback. UINT32_MAX in case of - * errors such as buffer overflow. + * @return The packed size of the passed object according to the callback. + * @retval UINT32_MAX in case of errors such as buffer overflow. */ -non_null(1) nullable(2) -uint32_t bin_pack_obj_size(bin_pack_cb *callback, const void *obj); +non_null(1) nullable(2, 3) +uint32_t bin_pack_obj_size(bin_pack_cb *callback, const Logger *logger, const void *obj); /** @brief Pack an object into a buffer of a given size. * @@ -45,14 +57,57 @@ uint32_t bin_pack_obj_size(bin_pack_cb *callback, const void *obj); * overflows `uint32_t`, this function returns `false`. * * @param callback The function called on the created packer and packed object. + * @param logger Optional logger object to pass to the callback. * @param obj The object to be packed, passed as `obj` to the callback. * @param buf A byte array large enough to hold the serialised representation of `obj`. * @param buf_size The size of the byte array. Can be `UINT32_MAX` to disable bounds checking. * * @retval false if an error occurred (e.g. buffer overflow). */ +non_null(1, 4) nullable(2, 3) +bool bin_pack_obj(bin_pack_cb *callback, const Logger *logger, const void *obj, uint8_t *buf, uint32_t buf_size); + +/** @brief Determine the serialised size of an object array. + * + * Calls the callback `count` times with increasing `index` argument from 0 to + * `count`. This function is here just so we don't need to write the same + * trivial loop many times and so we don't need an extra struct just to contain + * an array with size so it can be passed to `bin_pack_obj_size`. + * + * @param callback The function called on the created packer and each object to + * be packed. + * @param logger Optional logger object to pass to the callback. + * @param arr The object array to be packed, passed as `arr` to the callback. + * @param count The number of elements in the object array. + * + * @return The packed size of the passed object array according to the callback. + * @retval UINT32_MAX in case of errors such as buffer overflow. + */ non_null(1, 3) nullable(2) -bool bin_pack_obj(bin_pack_cb *callback, const void *obj, uint8_t *buf, uint32_t buf_size); +uint32_t bin_pack_obj_array_size(bin_pack_array_cb *callback, const Logger *logger, const void *arr, uint32_t count); + +/** @brief Pack an object array into a buffer of a given size. + * + * Calls the callback `count` times with increasing `index` argument from 0 to + * `count`. This function is here just so we don't need to write the same + * trivial loop many times and so we don't need an extra struct just to contain + * an array with size so it can be passed to `bin_pack_obj`. + * + * Similar to `bin_pack_obj` but for arrays. Does not write the array length, so + * if you need that, write it manually using `bin_pack_array`. + * + * @param callback The function called on the created packer and packed object + * array. + * @param logger Optional logger object to pass to the callback. + * @param arr The object array to be packed, passed as `arr` to the callback. + * @param count The number of elements in the object array. + * @param buf A byte array large enough to hold the serialised representation of `arr`. + * @param buf_size The size of the byte array. Can be `UINT32_MAX` to disable bounds checking. + * + * @retval false if an error occurred (e.g. buffer overflow). + */ +non_null(1, 3, 5) nullable(2) +bool bin_pack_obj_array(bin_pack_array_cb *callback, const Logger *logger, const void *arr, uint32_t count, uint8_t *buf, uint32_t buf_size); /** @brief Allocate a new packer object. * diff --git a/toxcore/tox_events.c b/toxcore/tox_events.c index 91a7767f7b6..4bfe844829d 100644 --- a/toxcore/tox_events.c +++ b/toxcore/tox_events.c @@ -215,20 +215,20 @@ bool tox_events_unpack(Tox_Events *events, Bin_Unpack *bu) return true; } -non_null(1) nullable(2) -static bool tox_events_bin_pack_handler(Bin_Pack *bp, const void *obj) +non_null(1) nullable(2, 3) +static bool tox_events_bin_pack_handler(Bin_Pack *bp, const Logger *logger, const void *obj) { return tox_events_pack((const Tox_Events *)obj, bp); } uint32_t tox_events_bytes_size(const Tox_Events *events) { - return bin_pack_obj_size(tox_events_bin_pack_handler, events); + return bin_pack_obj_size(tox_events_bin_pack_handler, nullptr, events); } void tox_events_get_bytes(const Tox_Events *events, uint8_t *bytes) { - bin_pack_obj(tox_events_bin_pack_handler, events, bytes, UINT32_MAX); + bin_pack_obj(tox_events_bin_pack_handler, nullptr, events, bytes, UINT32_MAX); } Tox_Events *tox_events_load(const uint8_t *bytes, uint32_t bytes_size)