Add some tests for ping_array.

No timeout test here yet, because we don't yet have the ability to
manipulate time at will, so we would have to actually sleep.

CMakeLists.txt

@@ -362,6 +362,7 @@ unit_test(toxav ring_buffer)
 unit_test(toxav rtp)
 unit_test(toxcore crypto_core)
 unit_test(toxcore mono_time)
+unit_test(toxcore ping_array)
 unit_test(toxcore util)
 
 ################################################################################

other/astyle/format-source

@@ -49,15 +49,25 @@ if grep '<unresolved>' */*.h; then
   exit 1
 fi
 
+CC_SOURCES=`find . '(' -name '*.cc' ')'`
+CC_SOURCES="$CC_SOURCES toxcore/ping_array.c"
+
+for bin in clang-format-6.0 clang-format-5.0 clang-format; do
+  if which "$bin"; then
+    "$bin" -i -style='{BasedOnStyle: Google, ColumnLimit: 100}' $CC_SOURCES
+    break
+  fi
+done
+
 FIND="find ."
-FIND="$FIND '(' -name '*.[ch]' -or -name '*.cpp' ')'"
+FIND="$FIND '(' -name '*.[ch]' ')'"
 FIND="$FIND -and -not -name '*.api.h'"
 FIND="$FIND -and -not -wholename './super_donators/*'"
 FIND="$FIND -and -not -wholename './third_party/*'"
 FIND="$FIND -and -not -wholename './toxencryptsave/crypto_pwhash*'"
 
-SOURCES=`eval "$FIND"`
+C_SOURCES=`eval "$FIND"`
 
-$ASTYLE -n --options=other/astyle/astylerc $SOURCES
+$ASTYLE -n --options=other/astyle/astylerc $C_SOURCES
 
 git diff --exit-code

testing/random_testing.cc

@@ -283,7 +283,8 @@ bool attempt_action(Global_State *toxes, std::mt19937 *rng) {
 int main() {
   std::vector<Action> const actions = {
       {
-          10, "creates a new conference",
+          10,
+          "creates a new conference",
           [](Local_State const &state) {
             return tox_conference_get_chatlist_size(state.tox()) < MAX_CONFERENCES_PER_USER;
           },
@@ -294,7 +295,8 @@ int main() {
           },
       },
       {
-          10, "invites a random friend to a conference",
+          10,
+          "invites a random friend to a conference",
           [](Local_State const &state) {
             return tox_conference_get_chatlist_size(state.tox()) != 0;
           },
@@ -302,15 +304,15 @@ int main() {
             size_t chat_count = tox_conference_get_chatlist_size(state->tox());
             assert(chat_count != 0);  // Condition above.
             TOX_ERR_CONFERENCE_INVITE err;
-            tox_conference_invite(
+            tox_conference_invite(state->tox(), rnd->friend_selector(*rng),
-                state->tox(), rnd->friend_selector(*rng),
+                                  state->next_invite % chat_count, &err);
-                state->next_invite % chat_count, &err);
             state->next_invite++;
             assert(err == TOX_ERR_CONFERENCE_INVITE_OK);
           },
       },
       {
-          10, "deletes the last conference",
+          10,
+          "deletes the last conference",
           [](Local_State const &state) {
             return tox_conference_get_chatlist_size(state.tox()) != 0;
           },
@@ -322,7 +324,8 @@ int main() {
           },
       },
       {
-          10, "sends a message to the last conference",
+          10,
+          "sends a message to the last conference",
           [](Local_State const &state) {
             return tox_conference_get_chatlist_size(state.tox()) != 0;
           },
@@ -344,7 +347,9 @@ int main() {
           },
       },
       {
-          10, "changes their name", [](Local_State const &state) { return true; },
+          10,
+          "changes their name",
+          [](Local_State const &state) { return true; },
           [](Local_State *state, Random *rnd, std::mt19937 *rng) {
             std::vector<uint8_t> name(rnd->name_length_selector(*rng));
             for (uint8_t &byte : name) {
@@ -359,7 +364,9 @@ int main() {
           },
       },
       {
-          10, "sets their name to empty", [](Local_State const &state) { return true; },
+          10,
+          "sets their name to empty",
+          [](Local_State const &state) { return true; },
           [](Local_State *state, Random *rnd, std::mt19937 *rng) {
             TOX_ERR_SET_INFO err;
             tox_self_set_name(state->tox(), nullptr, 0, &err);

toxcore/BUILD.bazel

@@ -113,6 +113,15 @@ cc_library(
     deps = [":network"],
 )
 
+cc_test(
+    name = "ping_array_test",
+    srcs = ["ping_array_test.cc"],
+    deps = [
+        ":ping_array",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 cc_library(
     name = "DHT",
     srcs = [

toxcore/crypto_core_test.cc

@@ -10,8 +10,10 @@ enum {
   /**
    * The size of the arrays to compare. This was chosen to take around 2000
    * CPU clocks on x86_64.
+   *
+   * This is 1MiB.
    */
-  CRYPTO_TEST_MEMCMP_SIZE = 1024 * 1024,  // 1 MiB
+  CRYPTO_TEST_MEMCMP_SIZE = 1024 * 1024,
   /**
    * The number of times we run memcmp in the test.
    *

toxcore/ping_array.api.h

@@ -22,10 +22,15 @@
  * You should have received a copy of the GNU General Public License
  * along with Tox.  If not, see <http://www.gnu.org/licenses/>.
  */
-#ifndef PING_ARRAY_H
+#ifndef C_TOXCORE_TOXCORE_PING_ARRAY_H
-#define PING_ARRAY_H
+#define C_TOXCORE_TOXCORE_PING_ARRAY_H
 
-#include "network.h"
+#include <stddef.h>
+#include <stdint.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
 %}
 
 class mono_Time { struct this; }
@@ -36,11 +41,11 @@ struct this;
 
 /**
  * Initialize a Ping_Array.
- * size represents the total size of the array and should be a power of 2.
- * timeout represents the maximum timeout in seconds for the entry.
  *
- * return 0 on success.
+ * @param size represents the total size of the array and should be a power of 2.
- * return -1 on failure.
+ * @param timeout represents the maximum timeout in seconds for the entry.
+ *
+ * @return 0 on success, -1 on failure.
  */
 static this new(uint32_t size, uint32_t timeout);
 
@@ -52,8 +57,7 @@ void kill();
 /**
  * Add a data with length to the Ping_Array list and return a ping_id.
  *
- * return ping_id on success.
+ * @return ping_id on success, 0 on failure.
- * return 0 on failure.
  */
 uint64_t add(const mono_Time::this *mono_time, const uint8_t *data, uint32_t length);
 
@@ -62,13 +66,16 @@ uint64_t add(const mono_Time::this *mono_time, const uint8_t *data, uint32_t len
  *
  * On success, copies the data into data of length,
  *
- * return length of data copied on success.
+ * @return length of data copied on success, -1 on failure.
- * return -1 on failure.
  */
 int32_t check(const mono_Time::this *mono_time, uint8_t[length] data, uint64_t ping_id);
 
 }
 
 %{
+#ifdef __cplusplus
+}  // extern "C"
 #endif
+
+#endif  // C_TOXCORE_TOXCORE_PING_ARRAY_H
 %}

toxcore/ping_array.c

@@ -34,7 +34,6 @@
 #include "mono_time.h"
 #include "util.h"
 
-
 typedef struct Ping_Array_Entry {
     void *data;
     uint32_t length;
@@ -46,25 +45,23 @@ struct Ping_Array {
     Ping_Array_Entry *entries;
 
     uint32_t last_deleted; /* number representing the next entry to be deleted. */
-    uint32_t last_added; /* number representing the last entry to be added. */
+    uint32_t last_added;   /* number representing the last entry to be added. */
-    uint32_t total_size; /* The length of entries */
+    uint32_t total_size;   /* The length of entries */
-    uint32_t timeout; /* The timeout after which entries are cleared. */
+    uint32_t timeout;      /* The timeout after which entries are cleared. */
 };
 
-/* Initialize a Ping_Array.
- * size represents the total size of the array and should be a power of 2.
- * timeout represents the maximum timeout in seconds for the entry.
- *
- * return 0 on success.
- * return -1 on failure.
- */
 Ping_Array *ping_array_new(uint32_t size, uint32_t timeout)
 {
     if (size == 0 || timeout == 0) {
         return nullptr;
     }
 
-    Ping_Array *empty_array = (Ping_Array *)calloc(1, sizeof(Ping_Array));
+    if ((size & (size - 1)) != 0) {
+        // Not a power of 2.
+        return nullptr;
+    }
+
+    Ping_Array *const empty_array = (Ping_Array *)calloc(1, sizeof(Ping_Array));
 
     if (empty_array == nullptr) {
         return nullptr;
@@ -86,19 +83,15 @@ Ping_Array *ping_array_new(uint32_t size, uint32_t timeout)
 
 static void clear_entry(Ping_Array *array, uint32_t index)
 {
+    const Ping_Array_Entry empty = {nullptr};
     free(array->entries[index].data);
-    array->entries[index].data = nullptr;
+    array->entries[index] = empty;
-    array->entries[index].length = 0;
-    array->entries[index].time = 0;
-    array->entries[index].ping_id = 0;
 }
 
-/* Free all the allocated memory in a Ping_Array.
- */
 void ping_array_kill(Ping_Array *array)
 {
     while (array->last_deleted != array->last_added) {
-        uint32_t index = array->last_deleted % array->total_size;
+        const uint32_t index = array->last_deleted % array->total_size;
         clear_entry(array, index);
         ++array->last_deleted;
     }
@@ -112,7 +105,7 @@ void ping_array_kill(Ping_Array *array)
 static void ping_array_clear_timedout(Ping_Array *array, const Mono_Time *mono_time)
 {
     while (array->last_deleted != array->last_added) {
-        uint32_t index = array->last_deleted % array->total_size;
+        const uint32_t index = array->last_deleted % array->total_size;
 
         if (!mono_time_is_timeout(mono_time, array->entries[index].time, array->timeout)) {
             break;
@@ -123,15 +116,11 @@ static void ping_array_clear_timedout(Ping_Array *array, const Mono_Time *mono_t
     }
 }
 
-/* Add a data with length to the Ping_Array list and return a ping_id.
+uint64_t ping_array_add(Ping_Array *array, const Mono_Time *mono_time, const uint8_t *data,
- *
+                        uint32_t length)
- * return ping_id on success.
- * return 0 on failure.
- */
-uint64_t ping_array_add(Ping_Array *array, const Mono_Time *mono_time, const uint8_t *data, uint32_t length)
 {
     ping_array_clear_timedout(array, mono_time);
-    uint32_t index = array->last_added % array->total_size;
+    const uint32_t index = array->last_added % array->total_size;
 
     if (array->entries[index].data != nullptr) {
         array->last_deleted = array->last_added - array->total_size;
@@ -161,21 +150,14 @@ uint64_t ping_array_add(Ping_Array *array, const Mono_Time *mono_time, const uin
     return ping_id;
 }
 
-
+int32_t ping_array_check(Ping_Array *array, const Mono_Time *mono_time, uint8_t *data,
-/* Check if ping_id is valid and not timed out.
+                         size_t length, uint64_t ping_id)
- *
- * On success, copies the data into data of length,
- *
- * return length of data copied on success.
- * return -1 on failure.
- */
-int32_t ping_array_check(Ping_Array *array, const Mono_Time *mono_time, uint8_t *data, size_t length, uint64_t ping_id)
 {
     if (ping_id == 0) {
         return -1;
     }
 
-    uint32_t index = ping_id % array->total_size;
+    const uint32_t index = ping_id % array->total_size;
 
     if (array->entries[index].ping_id != ping_id) {
         return -1;
@@ -189,12 +171,13 @@ int32_t ping_array_check(Ping_Array *array, const Mono_Time *mono_time, uint8_t
         return -1;
     }
 
+    // TODO(iphydf): This can't happen? If it indeed can't, turn it into an assert.
     if (array->entries[index].data == nullptr) {
         return -1;
     }
 
     memcpy(data, array->entries[index].data, array->entries[index].length);
-    uint32_t len = array->entries[index].length;
+    const uint32_t len = array->entries[index].length;
     clear_entry(array, index);
     return len;
 }

toxcore/ping_array.h

@@ -21,10 +21,15 @@
  * You should have received a copy of the GNU General Public License
  * along with Tox.  If not, see <http://www.gnu.org/licenses/>.
  */
-#ifndef PING_ARRAY_H
+#ifndef C_TOXCORE_TOXCORE_PING_ARRAY_H
-#define PING_ARRAY_H
+#define C_TOXCORE_TOXCORE_PING_ARRAY_H
 
-#include "network.h"
+#include <stddef.h>
+#include <stdint.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
 
 #ifndef MONO_TIME_DEFINED
 #define MONO_TIME_DEFINED
@@ -38,11 +43,11 @@ typedef struct Ping_Array Ping_Array;
 
 /**
  * Initialize a Ping_Array.
- * size represents the total size of the array and should be a power of 2.
- * timeout represents the maximum timeout in seconds for the entry.
  *
- * return 0 on success.
+ * @param size represents the total size of the array and should be a power of 2.
- * return -1 on failure.
+ * @param timeout represents the maximum timeout in seconds for the entry.
+ *
+ * @return 0 on success, -1 on failure.
  */
 struct Ping_Array *ping_array_new(uint32_t size, uint32_t timeout);
 
@@ -54,8 +59,7 @@ void ping_array_kill(struct Ping_Array *_array);
 /**
  * Add a data with length to the Ping_Array list and return a ping_id.
  *
- * return ping_id on success.
+ * @return ping_id on success, 0 on failure.
- * return 0 on failure.
  */
 uint64_t ping_array_add(struct Ping_Array *_array, const struct Mono_Time *mono_time, const uint8_t *data,
                         uint32_t length);
@@ -65,10 +69,13 @@ uint64_t ping_array_add(struct Ping_Array *_array, const struct Mono_Time *mono_
  *
  * On success, copies the data into data of length,
  *
- * return length of data copied on success.
+ * @return length of data copied on success, -1 on failure.
- * return -1 on failure.
  */
 int32_t ping_array_check(struct Ping_Array *_array, const struct Mono_Time *mono_time, uint8_t *data, size_t length,
                          uint64_t ping_id);
 
+#ifdef __cplusplus
+}  // extern "C"
 #endif
+
+#endif  // C_TOXCORE_TOXCORE_PING_ARRAY_H

toxcore/ping_array_test.cc

@@ -0,0 +1,119 @@
+#include "ping_array.h"
+
+#include <memory>
+
+#include <gtest/gtest.h>
+#include "mono_time.h"
+
+namespace {
+
+struct Ping_Array_Deleter {
+  void operator()(Ping_Array *arr) { ping_array_kill(arr); }
+};
+
+using Ping_Array_Ptr = std::unique_ptr<Ping_Array, Ping_Array_Deleter>;
+
+struct Mono_Time_Deleter {
+  void operator()(Mono_Time *arr) { mono_time_free(arr); }
+};
+
+using Mono_Time_Ptr = std::unique_ptr<Mono_Time, Mono_Time_Deleter>;
+
+TEST(PingArray, MinimumTimeoutIsOne) {
+  EXPECT_EQ(ping_array_new(1, 0), nullptr);
+  EXPECT_NE(Ping_Array_Ptr(ping_array_new(1, 1)), nullptr);
+}
+
+TEST(PingArray, MinimumArraySizeIsOne) {
+  EXPECT_EQ(ping_array_new(0, 1), nullptr);
+  EXPECT_NE(Ping_Array_Ptr(ping_array_new(1, 1)), nullptr);
+}
+
+TEST(PingArray, ArraySizeMustBePowerOfTwo) {
+  Ping_Array_Ptr arr;
+  arr.reset(ping_array_new(2, 1));
+  EXPECT_NE(arr, nullptr);
+  arr.reset(ping_array_new(4, 1));
+  EXPECT_NE(arr, nullptr);
+  arr.reset(ping_array_new(1024, 1));
+  EXPECT_NE(arr, nullptr);
+
+  EXPECT_EQ(ping_array_new(1023, 1), nullptr);
+  EXPECT_EQ(ping_array_new(1234, 1), nullptr);
+}
+
+TEST(PingArray, StoredDataCanBeRetrieved) {
+  Ping_Array_Ptr const arr(ping_array_new(2, 1));
+  Mono_Time_Ptr const mono_time(mono_time_new());
+
+  uint64_t const ping_id =
+      ping_array_add(arr.get(), mono_time.get(), std::vector<uint8_t>{1, 2, 3, 4}.data(), 4);
+  EXPECT_NE(ping_id, 0);
+
+  std::vector<uint8_t> data(4);
+  EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), data.data(), data.size(), ping_id), 4);
+  EXPECT_EQ(data, std::vector<uint8_t>({1, 2, 3, 4}));
+}
+
+TEST(PingArray, RetrievingDataWithTooSmallOutputBufferHasNoEffect) {
+  Ping_Array_Ptr const arr(ping_array_new(2, 1));
+  Mono_Time_Ptr const mono_time(mono_time_new());
+
+  uint64_t const ping_id =
+      ping_array_add(arr.get(), mono_time.get(), (std::vector<uint8_t>{1, 2, 3, 4}).data(), 4);
+  EXPECT_NE(ping_id, 0);
+
+  std::vector<uint8_t> data(4);
+  EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), data.data(), 3, ping_id), -1);
+  // It doesn't write anything to the data array.
+  EXPECT_EQ(data, std::vector<uint8_t>({0, 0, 0, 0}));
+  // Afterwards, we can still read it.
+  EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), data.data(), 4, ping_id), 4);
+  EXPECT_EQ(data, std::vector<uint8_t>({1, 2, 3, 4}));
+}
+
+TEST(PingArray, ZeroLengthDataCanBeAdded) {
+  Ping_Array_Ptr const arr(ping_array_new(2, 1));
+  Mono_Time_Ptr const mono_time(mono_time_new());
+
+  uint64_t const ping_id = ping_array_add(arr.get(), mono_time.get(), nullptr, 0);
+  EXPECT_NE(ping_id, 0);
+
+  EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, ping_id), 0);
+}
+
+TEST(PingArray, PingId0IsInvalid) {
+  Ping_Array_Ptr const arr(ping_array_new(2, 1));
+  Mono_Time_Ptr const mono_time(mono_time_new());
+
+  EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, 0), -1);
+}
+
+// Protection against replay attacks.
+TEST(PingArray, DataCanOnlyBeRetrievedOnce) {
+  Ping_Array_Ptr const arr(ping_array_new(2, 1));
+  Mono_Time_Ptr const mono_time(mono_time_new());
+
+  uint64_t const ping_id = ping_array_add(arr.get(), mono_time.get(), nullptr, 0);
+  EXPECT_NE(ping_id, 0);
+
+  EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, ping_id), 0);
+  EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, ping_id), -1);
+}
+
+TEST(PingArray, PingIdMustMatchOnCheck) {
+  Ping_Array_Ptr const arr(ping_array_new(1, 1));
+  Mono_Time_Ptr const mono_time(mono_time_new());
+
+  uint64_t const ping_id = ping_array_add(arr.get(), mono_time.get(), nullptr, 0);
+  EXPECT_NE(ping_id, 0);
+
+  uint64_t const bad_ping_id = ping_id == 1 ? 2 : 1;
+
+  // bad_ping_id will also be pointing at the same element, but won't match the
+  // actual ping_id.
+  EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, bad_ping_id), -1);
+  EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, ping_id), 0);
+}
+
+}  // namespace