fix: issues with packet broadcast error reporting

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.

Author: JFreegman

other/bootstrap_daemon/docker/tox-bootstrapd.sha256

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

toxcore/group_chats.c

@@ -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 {

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,15 +623,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 +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)

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()