From 6b6718e4d216363eddc3e6f6c2c94ec57d091dea Mon Sep 17 00:00:00 2001
From: jfreegman <jfreegman@gmail.com>
Date: Thu, 11 Jan 2024 10:55:10 -0500
Subject: [PATCH] cleanup: Make group packet entry creation less error-prone

We always assumed that create_array_entry() would only be called
with an empty array entry and wouldn't modify entries on error.
We now explicitly require both conditions, and also give an
error in the case of a non-null data pointer with a zero
length field, as this indicates a logic error.

Checks for an empty array entry that precede a call to
create_array_entry() are now redundant. It should be noted that
a non-empty entry doesn't necessarily indicate an error. This
condition can be triggered if packets are being sent or
received faster than they can be processed/acknowledged,
which is common when spamming messages on poor connections.
---
 .../docker/tox-bootstrapd.sha256              |  2 +-
 toxcore/group_connection.c                    | 40 +++++++++----------
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256
index 924fa7cf..726a1ed7 100644
--- a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256
+++ b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256
@@ -1 +1 @@
-423687bc6adc323174a2ab4e2ee8443e6c21c4d6509942af469b4bc16db6bfa4  /usr/local/bin/tox-bootstrapd
+c98b35f18f351c9a3a05137e69a43e634ab5963d364b9337e89ad89469ebd8c2  /usr/local/bin/tox-bootstrapd
diff --git a/toxcore/group_connection.c b/toxcore/group_connection.c
index 7f49b603..7f3c0928 100644
--- a/toxcore/group_connection.c
+++ b/toxcore/group_connection.c
@@ -85,18 +85,32 @@ void gcc_set_recv_message_id(GC_Connection *gconn, uint64_t id)
 }
 
 /** @brief Puts packet data in array_entry.
+ *
+ * Requires an empty array entry to be passed, and must not modify the passed
+ * array entry on error.
  *
  * Return true on success.
  */
-non_null(1, 2) nullable(3)
-static bool create_array_entry(const Mono_Time *mono_time, GC_Message_Array_Entry *array_entry, const uint8_t *data,
-                               uint16_t length, uint8_t packet_type, uint64_t message_id)
+non_null(1, 2, 3) nullable(4)
+static bool create_array_entry(const Logger *log, const Mono_Time *mono_time, GC_Message_Array_Entry *array_entry,
+        const uint8_t *data, uint16_t length, uint8_t packet_type, uint64_t message_id)
 {
+    if (!array_entry_is_empty(array_entry)) {
+        LOGGER_WARNING(log, "Failed to create array entry; entry is not empty.");
+        return false;
+    }
+
     if (length == 0) {
+        if (data != nullptr) {
+            LOGGER_FATAL(log, "Got non-null data with zero length (type %d)", packet_type); // should never happen
+            return false;
+        }
+
         array_entry->data = nullptr;
         array_entry->data_length = 0;
     } else {
         if (data == nullptr) {
+            LOGGER_FATAL(log, "Got null data with non-zero length (type %u)", packet_type); // should never happen
             return false;
         }
 
@@ -138,13 +152,7 @@ static bool add_to_send_array(const Logger *log, const Mono_Time *mono_time, GC_
     const uint16_t idx = gcc_get_array_index(gconn->send_message_id);
     GC_Message_Array_Entry *array_entry = &gconn->send_array[idx];
 
-    if (!array_entry_is_empty(array_entry)) {
-        LOGGER_DEBUG(log, "Send array entry isn't empty");
-        return false;
-    }
-
-    if (!create_array_entry(mono_time, array_entry, data, length, packet_type, gconn->send_message_id)) {
-        LOGGER_WARNING(log, "Failed to create array entry");
+    if (!create_array_entry(log, mono_time, array_entry, data, length, packet_type, gconn->send_message_id)) {
         return false;
     }
 
@@ -335,17 +343,7 @@ static bool store_in_recv_array(const Logger *log, const Mono_Time *mono_time, G
     const uint16_t idx = gcc_get_array_index(message_id);
     GC_Message_Array_Entry *ary_entry = &gconn->recv_array[idx];
 
-    if (!array_entry_is_empty(ary_entry)) {
-        LOGGER_DEBUG(log, "Recv array is not empty");
-        return false;
-    }
-
-    if (!create_array_entry(mono_time, ary_entry, data, length, packet_type, message_id)) {
-        LOGGER_WARNING(log, "Failed to create array entry");
-        return false;
-    }
-
-    return true;
+    return create_array_entry(log, mono_time, ary_entry, data, length, packet_type, message_id);
 }
 
 /**