From 072e3beb3f41ab9ed7c26a7eb2c44fa1c509db48 Mon Sep 17 00:00:00 2001 From: jfreegman Date: Thu, 11 Jan 2024 14:54:58 -0500 Subject: [PATCH] fix: issues with packet broadcast error reporting commit 5b9c420c introduced some undesirable behaviour with packet send functions returning error when they shouldn't. We now only return an error if the packet fails to be added to the send queue or cannot be wrapped/encrypted. We no longer error if we fail to send the packet over the wire, because toxcore will keep trying to re-send the packet until the connection times out. Additionally, we now make sure that our packet broadcast functions aren't returning an error when failing to send packets to peers that we have not successfully handshaked with yet, since this is expected behaviour. --- .../docker/tox-bootstrapd.sha256 | 2 +- toxcore/group_chats.c | 18 +++++++++++---- toxcore/group_connection.c | 23 ++++++++++++------- toxcore/group_connection.h | 14 ++++++----- 4 files changed, 37 insertions(+), 20 deletions(-) diff --git a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 index 726a1ed7..f632c821 100644 --- a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 +++ b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 @@ -1 +1 @@ -c98b35f18f351c9a3a05137e69a43e634ab5963d364b9337e89ad89469ebd8c2 /usr/local/bin/tox-bootstrapd +a6701e463517907cdb450f3210389633feb6c1cc3a1d3faec48c8b99a4aebf3c /usr/local/bin/tox-bootstrapd diff --git a/toxcore/group_chats.c b/toxcore/group_chats.c index a8a67d48..4157b52a 100644 --- a/toxcore/group_chats.c +++ b/toxcore/group_chats.c @@ -2264,12 +2264,14 @@ FAILED_INVITE: /** @brief Sends a lossless packet of type and length to all confirmed peers. * - * Return true if packet is successfully sent to at least one peer. + * Return true if packet is successfully sent to at least one peer or the + * group is empty. */ non_null() static bool send_gc_lossless_packet_all_peers(const GC_Chat *chat, const uint8_t *data, uint16_t length, uint8_t type) { uint32_t sent = 0; + uint32_t confirmed_peers = 0; for (uint32_t i = 1; i < chat->numpeers; ++i) { GC_Connection *gconn = get_gc_connection(chat, i); @@ -2280,22 +2282,26 @@ static bool send_gc_lossless_packet_all_peers(const GC_Chat *chat, const uint8_t continue; } + ++confirmed_peers; + if (send_lossless_group_packet(chat, gconn, data, length, type)) { ++sent; } } - return sent > 0 || chat->numpeers <= 1; + return sent > 0 || confirmed_peers == 0; } /** @brief Sends a lossy packet of type and length to all confirmed peers. * - * Return true if packet is successfully sent to at least one peer. + * Return true if packet is successfully sent to at least one peer or the + * group is empty. */ non_null() static bool send_gc_lossy_packet_all_peers(const GC_Chat *chat, const uint8_t *data, uint16_t length, uint8_t type) { uint32_t sent = 0; + uint32_t confirmed_peers = 0; for (uint32_t i = 1; i < chat->numpeers; ++i) { const GC_Connection *gconn = get_gc_connection(chat, i); @@ -2306,12 +2312,14 @@ static bool send_gc_lossy_packet_all_peers(const GC_Chat *chat, const uint8_t *d continue; } + ++confirmed_peers; + if (send_lossy_group_packet(chat, gconn, data, length, type)) { ++sent; } } - return sent > 0 || chat->numpeers <= 1; + return sent > 0 || confirmed_peers == 0; } /** @brief Creates packet with broadcast header info followed by data of length. @@ -5318,7 +5326,7 @@ static int handle_gc_message_ack(const GC_Chat *chat, GC_Connection *gconn, cons if (gcc_encrypt_and_send_lossless_packet(chat, gconn, gconn->send_array[idx].data, gconn->send_array[idx].data_length, gconn->send_array[idx].message_id, - gconn->send_array[idx].packet_type)) { + gconn->send_array[idx].packet_type) == 0) { gconn->send_array[idx].last_send_try = tm; LOGGER_DEBUG(chat->log, "Re-sent requested packet %llu", (unsigned long long)message_id); } else { diff --git a/toxcore/group_connection.c b/toxcore/group_connection.c index 7f3c0928..75663906 100644 --- a/toxcore/group_connection.c +++ b/toxcore/group_connection.c @@ -137,7 +137,7 @@ static bool create_array_entry(const Logger *log, const Mono_Time *mono_time, GC /** @brief Adds data of length to gconn's send_array. * - * Returns true on success and increments gconn's send_message_id. + * Returns true and increments gconn's send_message_id on success. */ non_null(1, 2, 3) nullable(4) static bool add_to_send_array(const Logger *log, const Mono_Time *mono_time, GC_Connection *gconn, const uint8_t *data, @@ -171,8 +171,15 @@ int gcc_send_lossless_packet(const GC_Chat *chat, GC_Connection *gconn, const ui return -1; } - if (!gcc_encrypt_and_send_lossless_packet(chat, gconn, data, length, message_id, packet_type)) { - LOGGER_DEBUG(chat->log, "Failed to send payload: (type: 0x%02x, length: %d)", packet_type, length); + // If the packet fails to wrap/encrypt, we remove it from the send array, since trying to-resend + // the same bad packet probably won't help much. Otherwise we don't care if it doesn't successfully + // send through the wire as it will keep retrying until the connection times out. + if (gcc_encrypt_and_send_lossless_packet(chat, gconn, data, length, message_id, packet_type) == -1) { + const uint16_t idx = gcc_get_array_index(message_id); + GC_Message_Array_Entry *array_entry = &gconn->send_array[idx]; + clear_array_entry(array_entry); + gconn->send_message_id = message_id; + LOGGER_ERROR(chat->log, "Failed to encrypt payload: (type: 0x%02x, length: %d)", packet_type, length); return -2; } @@ -616,7 +623,7 @@ bool gcc_send_packet(const GC_Chat *chat, const GC_Connection *gconn, const uint return ret == 0 || direct_send_attempt; } -bool gcc_encrypt_and_send_lossless_packet(const GC_Chat *chat, const GC_Connection *gconn, const uint8_t *data, +int gcc_encrypt_and_send_lossless_packet(const GC_Chat *chat, const GC_Connection *gconn, const uint8_t *data, uint16_t length, uint64_t message_id, uint8_t packet_type) { const uint16_t packet_size = gc_get_wrapped_packet_size(length, NET_PACKET_GC_LOSSLESS); @@ -624,7 +631,7 @@ bool gcc_encrypt_and_send_lossless_packet(const GC_Chat *chat, const GC_Connecti if (packet == nullptr) { LOGGER_ERROR(chat->log, "Failed to allocate memory for packet buffer"); - return false; + return -1; } const int enc_len = group_packet_wrap( @@ -634,18 +641,18 @@ bool gcc_encrypt_and_send_lossless_packet(const GC_Chat *chat, const GC_Connecti if (enc_len < 0) { LOGGER_ERROR(chat->log, "Failed to wrap packet (type: 0x%02x, error: %d)", packet_type, enc_len); free(packet); - return false; + return -1; } if (!gcc_send_packet(chat, gconn, packet, (uint16_t)enc_len)) { LOGGER_DEBUG(chat->log, "Failed to send packet (type: 0x%02x, enc_len: %d)", packet_type, enc_len); free(packet); - return false; + return -2; } free(packet); - return true; + return 0; } void gcc_make_session_shared_key(GC_Connection *gconn, const uint8_t *sender_pk) diff --git a/toxcore/group_connection.h b/toxcore/group_connection.h index 5987f010..fa7a3c94 100644 --- a/toxcore/group_connection.h +++ b/toxcore/group_connection.h @@ -143,11 +143,11 @@ bool gcc_send_packet(const GC_Chat *chat, const GC_Connection *gconn, const uint /** @brief Sends a lossless packet to `gconn` comprised of `data` of size `length`. * * This function will add the packet to the lossless send array, encrypt/wrap it using the - * shared key associated with `gconn`, and send it over the wire. + * shared key associated with `gconn`, and try to send it over the wire. * - * Return 0 on success. + * Return 0 if the packet was successfully encrypted and added to the send array. * Return -1 if the packet couldn't be added to the send array. - * Return -2 if the packet failed to be encrypted or failed to send. + * Return -2 if the packet failed to be wrapped or encrypted. */ non_null(1, 2) nullable(3) int gcc_send_lossless_packet(const GC_Chat *chat, GC_Connection *gconn, const uint8_t *data, uint16_t length, @@ -172,11 +172,13 @@ bool gcc_send_lossless_packet_fragments(const GC_Chat *chat, GC_Connection *gcon * * This function does not add the packet to the send array. * - * Return true on success. + * Return 0 on success. + * Return -1 if packet wrapping and encryption fails. + * Return -2 if the packet fails to send. */ non_null(1, 2) nullable(3) -bool gcc_encrypt_and_send_lossless_packet(const GC_Chat *chat, const GC_Connection *gconn, const uint8_t *data, - uint16_t length, uint64_t message_id, uint8_t packet_type); +int gcc_encrypt_and_send_lossless_packet(const GC_Chat *chat, const GC_Connection *gconn, const uint8_t *data, + uint16_t length, uint64_t message_id, uint8_t packet_type); /** @brief Called when a peer leaves the group. */ non_null()