From 65b3375b98c9cf88256594f3ab38d76f16631d34 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. --- .cirrus.yml | 2 - .../docker/tox-bootstrapd.sha256 | 2 +- toxcore/BUILD.bazel | 34 ++--- toxcore/DHT.c | 119 ++++++++++-------- toxcore/DHT.h | 20 +-- toxcore/DHT_fuzz_test.cc | 12 ++ toxcore/Messenger.c | 6 +- toxcore/bin_pack.c | 44 +++++-- toxcore/bin_pack.h | 67 +++++++++- toxcore/tox_events.c | 8 +- 10 files changed, 212 insertions(+), 102 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index 94eccd87..903d98a5 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -9,7 +9,6 @@ bazel-opt_task: - /src/workspace/tools/inject-repo c-toxcore test_all_script: - cd /src/workspace && bazel test -k - --config=remote --build_tag_filters=-haskell --test_tag_filters=-haskell -- @@ -26,7 +25,6 @@ bazel-dbg_task: - /src/workspace/tools/inject-repo c-toxcore test_all_script: - cd /src/workspace && bazel test -k - --config=remote --build_tag_filters=-haskell --test_tag_filters=-haskell -- diff --git a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 index 6b2fd809..15411eee 100644 --- a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 +++ b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 @@ -1 +1 @@ -2e1ae94d48eb1793ec3b42ed1516c53fb0b6b0e41bc89160e6037ee881a872b4 /usr/local/bin/tox-bootstrapd +8bf35cb22ca6d5d85f5fe8993a6ce9424d2048f9bd6a57ab45dc52a6c8444fb3 /usr/local/bin/tox-bootstrapd diff --git a/toxcore/BUILD.bazel b/toxcore/BUILD.bazel index 9cf548c6..7448c4c4 100644 --- a/toxcore/BUILD.bazel +++ b/toxcore/BUILD.bazel @@ -74,6 +74,22 @@ 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 = [ + ":attributes", + ":ccompat", + ], +) + cc_library( name = "bin_pack", srcs = ["bin_pack.c"], @@ -82,6 +98,7 @@ cc_library( deps = [ ":attributes", ":ccompat", + ":logger", "//c-toxcore/third_party:cmp", ], ) @@ -156,22 +173,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 = [ - ":attributes", - ":ccompat", - ], -) - cc_library( name = "state", srcs = ["state.c"], @@ -337,6 +338,7 @@ cc_library( deps = [ ":LAN_discovery", ":attributes", + ":bin_pack", ":ccompat", ":crypto_core", ":logger", diff --git a/toxcore/DHT.c b/toxcore/DHT.c index 80f477ca..397e18cf 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" @@ -360,12 +362,32 @@ int packed_node_size(Family ip_family) } -int pack_ip_port(const Logger *logger, uint8_t *data, uint16_t length, const IP_Port *ip_port) +/** @brief Packs an IP structure. + * + * 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. + * + * @param is_ipv4 whether this IP is an IP4 or IP6. + * + * @retval true on success. + */ +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; @@ -387,32 +409,34 @@ 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 false; + } + + 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)); +} + +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); +} + +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 (is_ipv4) { - const uint32_t size = 1 + SIZE_IP4 + sizeof(uint16_t); - - if (size > length) { - return -1; - } - - 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); - - 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; } int dht_create_packet(const Memory *mem, const Random *rng, @@ -511,33 +535,25 @@ 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); +} + 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; - assert(increment == PACKED_NODE_SIZE_IP4 || increment == PACKED_NODE_SIZE_IP6); -#endif + 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; } int unpack_nodes(Node_format *nodes, uint16_t max_num_nodes, uint16_t *processed_data_len, const uint8_t *data, @@ -2829,8 +2845,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); mem_delete(dht->mem, clients); } diff --git a/toxcore/DHT.h b/toxcore/DHT.h index 6021958f..3e5ab36a 100644 --- a/toxcore/DHT.h +++ b/toxcore/DHT.h @@ -214,6 +214,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. @@ -226,16 +236,6 @@ int dht_create_packet(const Memory *mem, 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 e9673ae0..a978fddd 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/Messenger.c b/toxcore/Messenger.c index c76aa28e..dddea120 100644 --- a/toxcore/Messenger.c +++ b/toxcore/Messenger.c @@ -3153,7 +3153,7 @@ static void pack_groupchats(const GC_Session *c, Bin_Pack *bp) } non_null() -static bool pack_groupchats_handler(Bin_Pack *bp, const void *obj) +static bool pack_groupchats_handler(Bin_Pack *bp, const Logger *log, const void *obj) { pack_groupchats((const GC_Session *)obj, bp); return true; // TODO(iphydf): Return bool from pack functions. @@ -3163,7 +3163,7 @@ non_null() static uint32_t saved_groups_size(const Messenger *m) { GC_Session *c = m->group_handler; - return bin_pack_obj_size(pack_groupchats_handler, c); + return bin_pack_obj_size(pack_groupchats_handler, m->log, c); } non_null() @@ -3185,7 +3185,7 @@ static uint8_t *groups_save(const Messenger *m, uint8_t *data) data = state_write_section_header(data, STATE_COOKIE_TYPE, len, STATE_TYPE_GROUPS); - if (!bin_pack_obj(pack_groupchats_handler, c, data, len)) { + if (!bin_pack_obj(pack_groupchats_handler, m->log, c, data, len)) { LOGGER_FATAL(m->log, "failed to pack group chats into buffer of length %u", len); return data; } diff --git a/toxcore/bin_pack.c b/toxcore/bin_pack.c index 3575803a..51f2ff5b 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) -{ - Bin_Pack bp; - bin_pack_init(&bp, buf, buf_size); - return callback(&bp, obj); -} - -uint32_t bin_pack_obj_size(bin_pack_cb *callback, const void *obj) +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); - callback(&bp, obj); + 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, logger, 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); + 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 51646c08..6df21e0e 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 cae662e5..0aacb8c4 100644 --- a/toxcore/tox_events.c +++ b/toxcore/tox_events.c @@ -218,20 +218,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 Tox_System *sys, const uint8_t *bytes, uint32_t bytes_size)