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.

Author: iphydf

.circleci/config.yml

@@ -8,6 +8,7 @@ workflows:
       # Dynamic analysis
       - asan
       - tsan
+      - msan
       # Static analysis
       - clang-tidy
       - infer
@@ -47,6 +48,17 @@ jobs:
       - run: *apt_install
       - run: CC=clang .circleci/cmake-tsan
 
+  msan:
+    working_directory: ~/work
+    docker:
+      - image: toxchat/toktok-stack:0.0.31-msan
+
+    steps:
+      - checkout
+      - run: rm -rf /src/workspace/c-toxcore/* && mv * /src/workspace/c-toxcore/
+      # TODO(iphydf): Remove "|| true" once this works.
+      - run: cd /src/workspace && bazel test //c-toxcore/auto_tests:lossless_packet_test || true
+
   infer:
     working_directory: ~/work
     docker:

.cirrus.yml

@@ -50,23 +50,22 @@ 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.
-#bazel-msan_task:
-#  container:
-#    image: toxchat/toktok-stack:0.0.31-msan
-#    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
-#        --
-#        //c-toxcore/...
-#        -//c-toxcore/auto_tests:tcp_relay_test # TODO(robinlinden): Why does this pass locally but not in Cirrus?
+bazel-msan_task:
+  container:
+    image: toxchat/toktok-stack:0.0.31-msan
+    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
+        --
+        //c-toxcore/...
+        -//c-toxcore/auto_tests:tcp_relay_test || true # TODO(robinlinden): Why does this pass locally but not in Cirrus?
 
 # TODO(iphydf): Fix test timeouts.
 bazel-tsan_task:
@@ -92,6 +91,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

.github/settings.yml

@@ -25,7 +25,6 @@ branches:
           - "build-win32"
           - "build-win64"
           - "CodeFactor"
-          - "codecov/project"
           - "coverage-linux"
           - "ci/circleci: asan"
           - "ci/circleci: clang-tidy"

.github/workflows/ci.yml

@@ -5,6 +5,17 @@ on:
     branches: [master]
 
 jobs:
+  build-msan:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - name: Pull toxchat/toktok-stack:0.0.31-msan
+        run: docker pull toxchat/toktok-stack:0.0.31-msan
+      - name: Run tests under MemorySanitizer
+        # TODO(iphydf): Remove "|| true" once this works correctly.
+        run: docker run --rm -v $PWD:/src/workspace/c-toxcore toxchat/toktok-stack:0.0.31-msan
+          bazel test //c-toxcore/auto_tests:lossless_packet_test || true
+
   build-nacl:
     runs-on: ubuntu-latest
     steps:
@@ -18,8 +29,6 @@ jobs:
   build-win32:
     runs-on: ubuntu-latest
     steps:
-      - name: Set up Docker Buildx
-        uses: docker/setup-buildx-action@v1
       - uses: actions/checkout@v2
       - name: Docker Build
         run: .github/scripts/cmake-win32 install
@@ -29,8 +38,6 @@ jobs:
   build-win64:
     runs-on: ubuntu-latest
     steps:
-      - name: Set up Docker Buildx
-        uses: docker/setup-buildx-action@v1
       - uses: actions/checkout@v2
       - name: Docker Build
         run: .github/scripts/cmake-win64 install

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
+7f4c96481e30156e3c2e7e448e70faaaa12911694390374ae09bd53d5d7b4f9c  /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",