fix: Fix some uninitialised memory errors found by valgrind and msan.

Also added a valgrind build to run it on every pull request. I've had to
disable a few tests because valgrind makes those run infinitely slowly,
consistently timing them out.

.cirrus.yml

@@ -50,7 +50,8 @@ bazel-asan_task:
         //c-toxcore/...
         -//c-toxcore/auto_tests:tcp_relay_test # TODO(robinlinden): Why does this pass locally but not in Cirrus?
 
-# TODO(iphydf): Get msan to work properly.
+# TODO(iphydf): Enable once https://github.com/TokTok/toktok-stack/pull/293 is
+# merged and the image has been rebuilt.
 #bazel-msan_task:
 #  container:
 #    image: toxchat/toktok-stack:0.0.31-msan
@@ -65,7 +66,7 @@ bazel-asan_task:
 #        --test_tag_filters=-haskell
 #        --remote_download_minimal
 #        --
-#        //c-toxcore/...
+#        //c-toxcore/auto_tests/...
 #        -//c-toxcore/auto_tests:tcp_relay_test # TODO(robinlinden): Why does this pass locally but not in Cirrus?
 
 # TODO(iphydf): Fix test timeouts.
@@ -92,6 +93,33 @@ bazel-tsan_task:
         -//c-toxcore/auto_tests:tcp_relay_test
         -//c-toxcore/auto_tests:tox_many_test
 
+# TODO(iphydf): Fix timeouts so we can run more of the tests disabled below.
+bazel-valgrind_task:
+  container:
+    image: toxchat/toktok-stack:0.0.31-release
+    cpu: 2
+    memory: 4G
+  configure_script:
+    - /src/workspace/tools/inject-repo c-toxcore
+  test_all_script:
+    - cd /src/workspace && bazel test -k
+        --remote_http_cache=http://$CIRRUS_HTTP_CACHE_HOST
+        --build_tag_filters=-haskell
+        --test_tag_filters=-haskell
+        --remote_download_minimal
+        --config=valgrind
+        --
+        //c-toxcore/...
+        -//c-toxcore/auto_tests:conference_av_test
+        -//c-toxcore/auto_tests:conference_test
+        -//c-toxcore/auto_tests:dht_test
+        -//c-toxcore/auto_tests:encryptsave_test
+        -//c-toxcore/auto_tests:file_transfer_test
+        -//c-toxcore/auto_tests:onion_test
+        -//c-toxcore/auto_tests:tcp_relay_test
+        -//c-toxcore/auto_tests:tox_many_tcp_test
+        -//c-toxcore/auto_tests:tox_many_test
+
 cimple_task:
   container:
     image: toxchat/toktok-stack:0.0.31-release

BUILD.bazel

@@ -8,29 +8,25 @@ project()
 genrule(
     name = "copy_headers",
     srcs = [
-        "//c-toxcore/toxav:public_headers",
-        "//c-toxcore/toxcore:public_headers",
-        "//c-toxcore/toxencryptsave:public_headers",
+        "//c-toxcore/toxav:toxav.h",
+        "//c-toxcore/toxcore:tox.h",
+        "//c-toxcore/toxencryptsave:toxencryptsave.h",
     ],
     outs = [
         "tox/toxav.h",
         "tox/tox.h",
         "tox/toxencryptsave.h",
     ],
     cmd = """
-        cp $(location //c-toxcore/toxav:public_headers) $(GENDIR)/c-toxcore/tox/toxav.h
-        cp $(location //c-toxcore/toxcore:public_headers) $(GENDIR)/c-toxcore/tox/tox.h
-        cp $(location //c-toxcore/toxencryptsave:public_headers) $(GENDIR)/c-toxcore/tox/toxencryptsave.h
+        cp $(location //c-toxcore/toxav:toxav.h) $(GENDIR)/c-toxcore/tox/toxav.h
+        cp $(location //c-toxcore/toxcore:tox.h) $(GENDIR)/c-toxcore/tox/tox.h
+        cp $(location //c-toxcore/toxencryptsave:toxencryptsave.h) $(GENDIR)/c-toxcore/tox/toxencryptsave.h
     """,
 )
 
 cc_library(
     name = "c-toxcore",
-    hdrs = [
-        "tox/tox.h",
-        "tox/toxav.h",
-        "tox/toxencryptsave.h",
-    ],
+    hdrs = [":copy_headers"],
     includes = ["."],
     visibility = ["//visibility:public"],
     deps = [

auto_tests/conference_av_test.c

@@ -107,7 +107,7 @@ static bool toxes_are_disconnected_from_group(uint32_t tox_count, Tox **toxes,
         num_disconnected += disconnected[i];
     }
 
-    for (uint32_t i = 0; i < tox_count; i++) {
+    for (uint32_t i = 0; i < tox_count; ++i) {
         if (disconnected[i]) {
             continue;
         }
@@ -152,7 +152,7 @@ static void disconnect_toxes(uint32_t tox_count, Tox **toxes, State *state,
 
 static bool all_connected_to_group(uint32_t tox_count, Tox **toxes)
 {
-    for (uint32_t i = 0; i < tox_count; i++) {
+    for (uint32_t i = 0; i < tox_count; ++i) {
         if (tox_conference_peer_count(toxes[i], 0, nullptr) < tox_count) {
             return false;
         }
@@ -215,11 +215,11 @@ static bool test_audio(Tox **toxes, State *state, const bool *disabled, bool qui
         printf("testing sending and receiving audio\n");
     }
 
-    int16_t PCM[GROUP_AV_TEST_SAMPLES];
+    const int16_t PCM[GROUP_AV_TEST_SAMPLES] = {0};
 
     reset_received_audio(toxes, state);
 
-    for (uint32_t n = 0; n < GROUP_AV_AUDIO_ITERATIONS; n++) {
+    for (uint32_t n = 0; n < GROUP_AV_AUDIO_ITERATIONS; ++n) {
         for (uint32_t i = 0; i < NUM_AV_GROUP_TOX; ++i) {
             if (disabled[i]) {
                 continue;
@@ -266,7 +266,7 @@ static void test_eventual_audio(Tox **toxes, State *state, const bool *disabled,
 
 static void do_audio(Tox **toxes, State *state, uint32_t iterations)
 {
-    int16_t PCM[GROUP_AV_TEST_SAMPLES];
+    const int16_t PCM[GROUP_AV_TEST_SAMPLES] = {0};
     printf("running audio for %u iterations\n", iterations);
 
     for (uint32_t f = 0; f < iterations; ++f) {

auto_tests/encryptsave_test.c

@@ -153,12 +153,16 @@ static void test_keys(void)
     int ciphertext_length2a = size_large + TOX_PASS_ENCRYPTION_EXTRA_LENGTH;
     int plaintext_length2a = size_large;
     uint8_t *encrypted2a = (uint8_t *)malloc(ciphertext_length2a);
+    ck_assert(encrypted2a != nullptr);
     uint8_t *in_plaintext2a = (uint8_t *)malloc(plaintext_length2a);
+    ck_assert(in_plaintext2a != nullptr);
+    random_bytes(in_plaintext2a, plaintext_length2a);
     ret = tox_pass_encrypt(in_plaintext2a, plaintext_length2a, key_char, 12, encrypted2a, &encerr);
     ck_assert_msg(ret, "tox_pass_encrypt failure 2a: %d", encerr);
 
     // Decryption of same message.
-    uint8_t *out_plaintext2a = (uint8_t *) malloc(plaintext_length2a);
+    uint8_t *out_plaintext2a = (uint8_t *)malloc(plaintext_length2a);
+    ck_assert(out_plaintext2a != nullptr);
     ret = tox_pass_decrypt(encrypted2a, ciphertext_length2a, key_char, 12, out_plaintext2a, &decerr);
     ck_assert_msg(ret, "tox_pass_decrypt failure 2a: %d", decerr);
     ck_assert_msg(memcmp(in_plaintext2a, out_plaintext2a, plaintext_length2a) == 0, "Large message decryption failed");

other/astyle/format-source

@@ -25,7 +25,7 @@ readarray -t CC_SOURCES <<<"$(find . '(' -name '*.cc' ')')"
 CC_SOURCES+=(toxcore/crypto_core.c)
 CC_SOURCES+=(toxcore/ping_array.c)
 
-for bin in clang-format-6.0 clang-format-5.0 clang-format; do
+for bin in clang-format-11 clang-format-7 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

other/bootstrap_daemon/docker/tox-bootstrapd.sha256

@@ -1 +1 @@
-bf2b6ce150f5dc3ed95e8636dd025a13015c98bbf922b7602969560d310045f7  /usr/local/bin/tox-bootstrapd
+ac569584cf55060d8b72ab00951b015a12a3dad540b8cf36adc22594819a1092  /usr/local/bin/tox-bootstrapd

toxav/BUILD.bazel

@@ -3,15 +3,16 @@ load("//tools:no_undefined.bzl", "cc_library")
 
 package(features = ["layering_check"])
 
-filegroup(
-    name = "public_headers",
+exports_files(
     srcs = ["toxav.h"],
     visibility = ["//c-toxcore:__pkg__"],
 )
 
+# Private library with the public API header in it because in toxav, lots of
+# things depend on the public API header.
 cc_library(
-    name = "public",
-    hdrs = [":public_headers"],
+    name = "public_api",
+    hdrs = ["toxav.h"],
 )
 
 cc_library(
@@ -86,7 +87,7 @@ cc_library(
     srcs = ["audio.c"],
     hdrs = ["audio.h"],
     deps = [
-        ":public",
+        ":public_api",
         ":rtp",
         "//c-toxcore/toxcore:logger",
         "//c-toxcore/toxcore:mono_time",
@@ -107,7 +108,7 @@ cc_library(
     ],
     deps = [
         ":audio",
-        ":public",
+        ":public_api",
         ":ring_buffer",
         ":rtp",
         "//c-toxcore/toxcore:logger",

toxcore/BUILD.bazel

@@ -3,8 +3,7 @@ load("//tools:no_undefined.bzl", "cc_library")
 
 package(features = ["layering_check"])
 
-filegroup(
-    name = "public_headers",
+exports_files(
     srcs = ["tox.h"],
     visibility = ["//c-toxcore:__pkg__"],
 )

toxcore/DHT.c

@@ -513,6 +513,10 @@ int unpack_ip_port(IP_Port *ip_port, const uint8_t *data, uint16_t length, bool
         return -1;
     }
 
+    *ip_port = (IP_Port) {
+        0
+    };
+
     if (is_ipv4) {
         const uint32_t size = 1 + SIZE_IP4 + sizeof(uint16_t);
 

toxcore/Messenger.c

@@ -3070,7 +3070,7 @@ static uint32_t tcp_relay_size(const Messenger *m)
 
 static uint8_t *save_tcp_relays(const Messenger *m, uint8_t *data)
 {
-    Node_format relays[NUM_SAVED_TCP_RELAYS];
+    Node_format relays[NUM_SAVED_TCP_RELAYS] = {0};
     uint8_t *temp_data = data;
     data = state_write_section_header(temp_data, STATE_COOKIE_TYPE, 0, STATE_TYPE_TCP_RELAY);
 

toxcore/crypto_core.c

@@ -68,7 +68,7 @@ static void crypto_free(uint8_t *ptr, size_t bytes)
 
     free(ptr);
 }
-#endif // !defined(FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION)
+#endif  // !defined(FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION)
 
 int32_t public_key_cmp(const uint8_t *pk1, const uint8_t *pk2)
 {
@@ -145,8 +145,10 @@ int32_t encrypt_data_symmetric(const uint8_t *secret_key, const uint8_t *nonce,
     }
 
 #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-    memcpy(encrypted, plain, length); // Don't encrypt anything
-    memset(encrypted + length, 0, crypto_box_MACBYTES); // Zero MAC to avoid false alarms of uninitialized memory
+    // Don't encrypt anything.
+    memcpy(encrypted, plain, length);
+    // Zero MAC to avoid uninitialized memory reads.
+    memset(encrypted + length, 0, crypto_box_MACBYTES);
 #else
 
     const size_t size_temp_plain = length + crypto_box_ZEROBYTES;
@@ -161,6 +163,11 @@ int32_t encrypt_data_symmetric(const uint8_t *secret_key, const uint8_t *nonce,
         return -1;
     }
 
+    // crypto_box_afternm requires the entire range of the output array be
+    // initialised with something. It doesn't matter what it's initialised with,
+    // so we'll pick 0x00.
+    memset(temp_encrypted, 0, size_temp_encrypted);
+
     memset(temp_plain, 0, crypto_box_ZEROBYTES);
     // Pad the message with 32 0 bytes.
     memcpy(temp_plain + crypto_box_ZEROBYTES, plain, length);
@@ -189,7 +196,7 @@ int32_t decrypt_data_symmetric(const uint8_t *secret_key, const uint8_t *nonce,
     }
 
 #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-    memcpy(plain, encrypted, length - crypto_box_MACBYTES); // Don't encrypt anything
+    memcpy(plain, encrypted, length - crypto_box_MACBYTES);  // Don't encrypt anything
 #else
 
     const size_t size_temp_plain = length + crypto_box_ZEROBYTES;
@@ -204,6 +211,11 @@ int32_t decrypt_data_symmetric(const uint8_t *secret_key, const uint8_t *nonce,
         return -1;
     }
 
+    // crypto_box_open_afternm requires the entire range of the output array be
+    // initialised with something. It doesn't matter what it's initialised with,
+    // so we'll pick 0x00.
+    memset(temp_plain, 0, size_temp_plain);
+
     memset(temp_encrypted, 0, crypto_box_BOXZEROBYTES);
     // Pad the message with 16 0 bytes.
     memcpy(temp_encrypted + crypto_box_BOXZEROBYTES, encrypted, length);
@@ -319,8 +331,8 @@ void new_symmetric_key(uint8_t *key)
 int32_t crypto_new_keypair(uint8_t *public_key, uint8_t *secret_key)
 {
 #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
+    memset(public_key, 0xaa, CRYPTO_PUBLIC_KEY_SIZE);  // Make MSAN happy
     random_bytes(secret_key, CRYPTO_SECRET_KEY_SIZE);
-    memset(public_key, 0, CRYPTO_PUBLIC_KEY_SIZE); // Make MSAN happy
     crypto_scalarmult_curve25519_base(public_key, secret_key);
     return 0;
 #else

toxcore/crypto_core_test.cc

@@ -3,6 +3,7 @@
 #include <gtest/gtest.h>
 
 #include <algorithm>
+#include <array>
 #include <vector>
 
 namespace {

toxcore/network.h

@@ -138,12 +138,20 @@ typedef enum Net_Packet_Type {
 #define TCP_INET6 (TOX_AF_INET6 + 3)
 #define TCP_FAMILY (TOX_AF_INET6 + 4)
 
+#define SIZE_IP4 4
+#define SIZE_IP6 16
+#define SIZE_IP (1 + SIZE_IP6)
+#define SIZE_PORT 2
+#define SIZE_IPPORT (SIZE_IP + SIZE_PORT)
+
 typedef union IP4 {
     uint32_t uint32;
     uint16_t uint16[2];
     uint8_t uint8[4];
 } IP4;
 
+static_assert(sizeof(IP4) == SIZE_IP4, "IP4 size must be 4");
+
 IP4 get_ip4_loopback(void);
 extern const IP4 ip4_broadcast;
 
@@ -154,6 +162,10 @@ typedef union IP6 {
     uint64_t uint64[2];
 } IP6;
 
+// TODO(iphydf): Stop relying on this. We memcpy this struct (and IP4 above)
+// into packets but really should be serialising it properly.
+static_assert(sizeof(IP6) == SIZE_IP6, "IP6 size must be 16");
+
 IP6 get_ip6_loopback(void);
 extern const IP6 ip6_broadcast;
 
@@ -190,12 +202,6 @@ size_t net_unpack_u64(const uint8_t *bytes, uint64_t *v);
 /** Does the IP6 struct a contain an IPv4 address in an IPv6 one? */
 bool ipv6_ipv4_in_v6(IP6 a);
 
-#define SIZE_IP4 4
-#define SIZE_IP6 16
-#define SIZE_IP (1 + SIZE_IP6)
-#define SIZE_PORT 2
-#define SIZE_IPPORT (SIZE_IP + SIZE_PORT)
-
 #define TOX_ENABLE_IPV6_DEFAULT true
 
 /* addr_resolve return values */

toxcore/onion.c

@@ -357,7 +357,7 @@ int onion_send_1(const Onion *onion, const uint8_t *plain, uint16_t len, IP_Port
     uint8_t ip_port[SIZE_IPPORT];
     ipport_pack(ip_port, &source);
 
-    uint8_t data[ONION_MAX_PACKET_SIZE];
+    uint8_t data[ONION_MAX_PACKET_SIZE] = {0};
     data[0] = NET_PACKET_ONION_SEND_1;
     memcpy(data + 1, nonce, CRYPTO_NONCE_SIZE);
     memcpy(data + 1 + CRYPTO_NONCE_SIZE, plain + SIZE_IPPORT, len - SIZE_IPPORT);
@@ -411,7 +411,7 @@ static int handle_send_1(void *object, IP_Port source, const uint8_t *packet, ui
         return 1;
     }
 
-    uint8_t data[ONION_MAX_PACKET_SIZE];
+    uint8_t data[ONION_MAX_PACKET_SIZE] = {0};
     data[0] = NET_PACKET_ONION_SEND_2;
     memcpy(data + 1, packet + 1, CRYPTO_NONCE_SIZE);
     memcpy(data + 1 + CRYPTO_NONCE_SIZE, plain + SIZE_IPPORT, len - SIZE_IPPORT);
@@ -478,7 +478,7 @@ static int handle_send_2(void *object, IP_Port source, const uint8_t *packet, ui
         return 1;
     }
 
-    uint8_t data[ONION_MAX_PACKET_SIZE];
+    uint8_t data[ONION_MAX_PACKET_SIZE] = {0};
     memcpy(data, plain + SIZE_IPPORT, len - SIZE_IPPORT);
     uint16_t data_len = len - SIZE_IPPORT;
     uint8_t *ret_part = data + (len - SIZE_IPPORT);
@@ -537,7 +537,7 @@ static int handle_recv_3(void *object, IP_Port source, const uint8_t *packet, ui
         return 1;
     }
 
-    uint8_t data[ONION_MAX_PACKET_SIZE];
+    uint8_t data[ONION_MAX_PACKET_SIZE] = {0};
     data[0] = NET_PACKET_ONION_RECV_2;
     memcpy(data + 1, plain + SIZE_IPPORT, RETURN_2);
     memcpy(data + 1 + RETURN_2, packet + 1 + RETURN_3, length - (1 + RETURN_3));
@@ -584,7 +584,7 @@ static int handle_recv_2(void *object, IP_Port source, const uint8_t *packet, ui
         return 1;
     }
 
-    uint8_t data[ONION_MAX_PACKET_SIZE];
+    uint8_t data[ONION_MAX_PACKET_SIZE] = {0};
     data[0] = NET_PACKET_ONION_RECV_1;
     memcpy(data + 1, plain + SIZE_IPPORT, RETURN_1);
     memcpy(data + 1 + RETURN_1, packet + 1 + RETURN_2, length - (1 + RETURN_2));