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.
This commit is contained in:
jfreegman 2024-01-11 14:54:58 -05:00
parent 6b6718e4d2
commit 072e3beb3f
No known key found for this signature in database
GPG Key ID: 3627F3144076AE63
4 changed files with 37 additions and 20 deletions

View File

@ -1 +1 @@
c98b35f18f351c9a3a05137e69a43e634ab5963d364b9337e89ad89469ebd8c2 /usr/local/bin/tox-bootstrapd a6701e463517907cdb450f3210389633feb6c1cc3a1d3faec48c8b99a4aebf3c /usr/local/bin/tox-bootstrapd

View File

@ -2264,12 +2264,14 @@ FAILED_INVITE:
/** @brief Sends a lossless packet of type and length to all confirmed peers. /** @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() non_null()
static bool send_gc_lossless_packet_all_peers(const GC_Chat *chat, const uint8_t *data, uint16_t length, uint8_t type) 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 sent = 0;
uint32_t confirmed_peers = 0;
for (uint32_t i = 1; i < chat->numpeers; ++i) { for (uint32_t i = 1; i < chat->numpeers; ++i) {
GC_Connection *gconn = get_gc_connection(chat, 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; continue;
} }
++confirmed_peers;
if (send_lossless_group_packet(chat, gconn, data, length, type)) { if (send_lossless_group_packet(chat, gconn, data, length, type)) {
++sent; ++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. /** @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() non_null()
static bool send_gc_lossy_packet_all_peers(const GC_Chat *chat, const uint8_t *data, uint16_t length, uint8_t type) 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 sent = 0;
uint32_t confirmed_peers = 0;
for (uint32_t i = 1; i < chat->numpeers; ++i) { for (uint32_t i = 1; i < chat->numpeers; ++i) {
const GC_Connection *gconn = get_gc_connection(chat, 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; continue;
} }
++confirmed_peers;
if (send_lossy_group_packet(chat, gconn, data, length, type)) { if (send_lossy_group_packet(chat, gconn, data, length, type)) {
++sent; ++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. /** @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, if (gcc_encrypt_and_send_lossless_packet(chat, gconn, gconn->send_array[idx].data,
gconn->send_array[idx].data_length, gconn->send_array[idx].data_length,
gconn->send_array[idx].message_id, 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; gconn->send_array[idx].last_send_try = tm;
LOGGER_DEBUG(chat->log, "Re-sent requested packet %llu", (unsigned long long)message_id); LOGGER_DEBUG(chat->log, "Re-sent requested packet %llu", (unsigned long long)message_id);
} else { } else {

View File

@ -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. /** @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) 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, 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; return -1;
} }
if (!gcc_encrypt_and_send_lossless_packet(chat, gconn, data, length, message_id, packet_type)) { // If the packet fails to wrap/encrypt, we remove it from the send array, since trying to-resend
LOGGER_DEBUG(chat->log, "Failed to send payload: (type: 0x%02x, length: %d)", packet_type, length); // 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; 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; 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) 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); 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) { if (packet == nullptr) {
LOGGER_ERROR(chat->log, "Failed to allocate memory for packet buffer"); LOGGER_ERROR(chat->log, "Failed to allocate memory for packet buffer");
return false; return -1;
} }
const int enc_len = group_packet_wrap( 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) { if (enc_len < 0) {
LOGGER_ERROR(chat->log, "Failed to wrap packet (type: 0x%02x, error: %d)", packet_type, enc_len); LOGGER_ERROR(chat->log, "Failed to wrap packet (type: 0x%02x, error: %d)", packet_type, enc_len);
free(packet); free(packet);
return false; return -1;
} }
if (!gcc_send_packet(chat, gconn, packet, (uint16_t)enc_len)) { 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); LOGGER_DEBUG(chat->log, "Failed to send packet (type: 0x%02x, enc_len: %d)", packet_type, enc_len);
free(packet); free(packet);
return false; return -2;
} }
free(packet); free(packet);
return true; return 0;
} }
void gcc_make_session_shared_key(GC_Connection *gconn, const uint8_t *sender_pk) void gcc_make_session_shared_key(GC_Connection *gconn, const uint8_t *sender_pk)

View File

@ -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`. /** @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 * 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 -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) 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, int gcc_send_lossless_packet(const GC_Chat *chat, GC_Connection *gconn, const uint8_t *data, uint16_t length,
@ -172,10 +172,12 @@ bool gcc_send_lossless_packet_fragments(const GC_Chat *chat, GC_Connection *gcon
* *
* This function does not add the packet to the send array. * 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) 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, 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); uint16_t length, uint64_t message_id, uint8_t packet_type);
/** @brief Called when a peer leaves the group. */ /** @brief Called when a peer leaves the group. */