Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: issues with packet broadcast error reporting
Browse files Browse the repository at this point in the history
commit 5b9c420 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.
JFreegman committed Jan 11, 2024
1 parent 6b6718e commit f7ba009
Showing 4 changed files with 36 additions and 20 deletions.
2 changes: 1 addition & 1 deletion other/bootstrap_daemon/docker/tox-bootstrapd.sha256
Original file line number Diff line number Diff line change
@@ -1 +1 @@
c98b35f18f351c9a3a05137e69a43e634ab5963d364b9337e89ad89469ebd8c2 /usr/local/bin/tox-bootstrapd
53719042faeed079eeeaea4953d1a67b1fdef48aa4a175a1db6be3c135246f22 /usr/local/bin/tox-bootstrapd

Check failure on line 1 in other/bootstrap_daemon/docker/tox-bootstrapd.sha256

GitHub Actions / docker-bootstrap-node

04f244f6038a3d8968c751ed02086007c422c5c9917b58b7fc34637ca2d089d5 /usr/local/bin/tox-bootstrapd

Check failure on line 1 in other/bootstrap_daemon/docker/tox-bootstrapd.sha256

GitHub Actions / docker-bootstrap-node

04f244f6038a3d8968c751ed02086007c422c5c9917b58b7fc34637ca2d089d5 /usr/local/bin/tox-bootstrapd
18 changes: 13 additions & 5 deletions toxcore/group_chats.c
Original file line number Diff line number Diff line change
@@ -2264,12 +2264,14 @@ static int handle_gc_invite_request(GC_Chat *chat, uint32_t peer_number, const u

/** @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 {
22 changes: 14 additions & 8 deletions toxcore/group_connection.c
Original file line number Diff line number Diff line change
@@ -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,14 @@ 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 donn'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(gconn->send_message_id);
GC_Message_Array_Entry *array_entry = &gconn->send_array[idx];
clear_array_entry(array_entry);
LOGGER_ERROR(chat->log, "Failed to encrypt payload: (type: 0x%02x, length: %d)", packet_type, length);
return -2;
}

@@ -616,15 +622,15 @@ 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);
uint8_t *packet = (uint8_t *)malloc(packet_size);

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 +640,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)
14 changes: 8 additions & 6 deletions toxcore/group_connection.h
Original file line number Diff line number Diff line change
@@ -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()

0 comments on commit f7ba009

Please sign in to comment.