test: Upgrade cppcheck, fix some warnings.

Also started teaching it about toxcore's alloc/dealloc functions in
hopes of it catching some errors (it doesn't seem to be very good at
this, but maybe better than nothing?).

.circleci/config.yml

@@ -134,15 +134,13 @@ jobs:
       - run:
           apt-get install -y --no-install-recommends
             ca-certificates
-            cppcheck
             g++
             llvm-dev
       - checkout
       - run: git submodule update --init --recursive
       - run: other/analysis/check_includes
       - run: other/analysis/check_logger_levels
       - run: other/analysis/run-clang
-      - run: other/analysis/run-cppcheck
       - run: other/analysis/run-gcc
 
   clang-analyze:

.github/workflows/ci.yml

@@ -13,6 +13,16 @@ jobs:
   common:
     uses: TokTok/ci-tools/.github/workflows/common-ci.yml@master
 
+  cppcheck:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Set up Docker Buildx
+        uses: docker/setup-buildx-action@v3
+      - name: Docker Build
+        uses: docker/build-push-action@v4
+        with:
+          file: other/docker/cppcheck/Dockerfile
+
   mypy:
     runs-on: ubuntu-latest
     steps:

other/analysis/run-cppcheck

@@ -8,9 +8,12 @@ set -e
 
 CPPCHECK=("--enable=all")
 CPPCHECK+=("--inconclusive")
+CPPCHECK+=("--check-level=exhaustive")
+CPPCHECK+=("--inline-suppr")
+CPPCHECK+=("--library=other/docker/cppcheck/toxcore.cfg")
 CPPCHECK+=("--error-exitcode=1")
-# Used for VLA.
-CPPCHECK+=("--suppress=allocaCalled")
+# We don't cast function pointers, which cppcheck suggests here.
+CPPCHECK+=("--suppress=constParameterCallback")
 # False positives in switch statements.
 CPPCHECK+=("--suppress=knownConditionTrueFalse")
 # Cppcheck does not need standard library headers to get proper results.
@@ -19,27 +22,22 @@ CPPCHECK+=("--suppress=missingIncludeSystem")
 CPPCHECK+=("--suppress=signConversion")
 # TODO(iphydf): Fixed in the toxav refactor PR.
 CPPCHECK+=("--suppress=redundantAssignment")
-# We have some redundant nullptr checks in assertions
-CPPCHECK+=("--suppress=nullPointerRedundantCheck")
-# Triggers a false warning in group.c
-CPPCHECK+=("--suppress=AssignmentAddressToInteger")
-# TODO(sudden6): This triggers a false positive, check again later to enable it
-CPPCHECK+=("--suppress=arrayIndexOutOfBoundsCond")
-
-# We're a library. This only works on whole programs.
-CPPCHECK_C=("--suppress=unusedFunction")
 
+# We use this for VLAs.
+CPPCHECK_CXX+=("--suppress=allocaCalled")
 # False positive in auto_tests.
-CPPCHECK_CXX+=("--suppress=shadowArgument")
 CPPCHECK_CXX+=("--suppress=shadowFunction")
-# False positive for callback functions
-CPPCHECK_CXX+=("--suppress=constParameter")
+# False positive in group.c.
+# Using cppcheck-suppress claims the suppression is unused.
+CPPCHECK_CXX+=("--suppress=AssignmentAddressToInteger")
+# We use C style casts because we write C code.
+CPPCHECK_CXX+=("--suppress=cstyleCast")
 # Used in Messenger.c for a static_assert(...)
 CPPCHECK_CXX+=("--suppress=sizeofFunctionCall")
 
 run() {
   echo "Running cppcheck in variant '$*'"
-  cppcheck "${CPPCHECK[@]}" "${CPPCHECK_C[@]}" tox*/*.[ch] tox*/*/*.[ch] "${CPPFLAGS[@]}" "$@"
+  cppcheck -j8 "${CPPCHECK[@]}" "${CPPCHECK_C[@]}" tox*/*.[ch] tox*/*/*.[ch] "${CPPFLAGS[@]}" "$@"
   cppcheck "${CPPCHECK[@]}" "${CPPCHECK_CXX[@]}" amalgamation.cc "${CPPFLAGS[@]}" "$@"
 }
 

other/bootstrap_daemon/docker/tox-bootstrapd.sha256

@@ -1 +1 @@
-f0bff9fe04d56543d95a457afd76c618139eef99a4302337c66d07759d108e8b  /usr/local/bin/tox-bootstrapd
+68432689967d06dd144e5cdfe37751ccc62b2aa85b73a9cc55aff3732dc47fde  /usr/local/bin/tox-bootstrapd

other/bootstrap_daemon/src/tox-bootstrapd.c

@@ -57,7 +57,7 @@ static void sleep_milliseconds(uint32_t ms)
 // returns 1 on success
 //         0 on failure - no keys were read or stored
 
-static int manage_keys(DHT *dht, char *keys_file_path)
+static int manage_keys(DHT *dht, const char *keys_file_path)
 {
     enum { KEYS_SIZE = CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_SECRET_KEY_SIZE };
     uint8_t keys[KEYS_SIZE];

other/docker/cppcheck/Dockerfile

@@ -0,0 +1,30 @@
+FROM alpine:3.19.0
+
+RUN ["apk", "add", "--no-cache", \
+ "bash", \
+ "cppcheck", \
+ "findutils", \
+ "libconfig-dev", \
+ "libsodium-dev", \
+ "libvpx-dev", \
+ "linux-headers", \
+ "make", \
+ "opus-dev"]
+
+COPY other/bootstrap_daemon/ /src/workspace/c-toxcore/other/bootstrap_daemon/
+COPY other/bootstrap_node_packets.* /src/workspace/c-toxcore/other/
+COPY other/fun/ /src/workspace/c-toxcore/other/fun/
+COPY auto_tests/check_compat.h /src/workspace/c-toxcore/auto_tests/
+COPY testing/ /src/workspace/c-toxcore/testing/
+COPY toxav/ /src/workspace/c-toxcore/toxav/
+COPY toxcore/ /src/workspace/c-toxcore/toxcore/
+COPY toxencryptsave/ /src/workspace/c-toxcore/toxencryptsave/
+COPY third_party/cmp/cmp.h /src/workspace/c-toxcore/third_party/cmp/
+COPY other/analysis/run-cppcheck \
+     other/analysis/gen-file.sh \
+     other/analysis/variants.sh \
+     /src/workspace/c-toxcore/other/analysis/
+COPY other/docker/cppcheck/toxcore.cfg \
+     /src/workspace/c-toxcore/other/docker/cppcheck/
+WORKDIR /src/workspace/c-toxcore
+RUN ["other/analysis/run-cppcheck"]

other/docker/cppcheck/run

@@ -0,0 +1,5 @@
+#!/bin/sh
+
+set -eux
+BUILD=cppcheck
+docker build -t "toxchat/c-toxcore:$BUILD" -f "other/docker/$BUILD/Dockerfile" .

other/docker/cppcheck/toxcore.cfg

@@ -0,0 +1,117 @@
+<?xml version="1.0"?>
+<def format="2">
+  <memory>
+    <alloc init="false" buffer-size="malloc:2">mem_balloc</alloc>
+    <alloc init="true" buffer-size="malloc:2">mem_alloc</alloc>
+    <alloc init="true" buffer-size="calloc:2,3">mem_valloc</alloc>
+    <realloc init="false" buffer-size="calloc:3,4">mem_vrealloc</realloc>
+    <dealloc arg="2">mem_delete</dealloc>
+  </memory>
+  <resource>
+    <alloc init="true">bin_pack_new</alloc>
+    <dealloc arg="1">bin_pack_free</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">bin_unpack_new</alloc>
+    <dealloc arg="1">bin_unpack_free</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">friendreq_new</alloc>
+    <dealloc arg="1">friendreq_kill</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">logger_new</alloc>
+    <dealloc arg="1">logger_kill</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">mono_time_new</alloc>
+    <dealloc arg="1">mono_time_free</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">ping_array_new</alloc>
+    <dealloc arg="1">ping_array_kill</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">ping_new</alloc>
+    <dealloc arg="1">ping_kill</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">shared_key_cache_new</alloc>
+    <dealloc arg="1">shared_key_cache_free</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">tox_dispatch_new</alloc>
+    <dealloc arg="1">tox_dispatch_free</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">tox_new</alloc>
+    <dealloc arg="1">tox_kill</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">tox_options_new</alloc>
+    <dealloc arg="1">tox_options_free</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_announcements</alloc>
+    <dealloc arg="1">kill_announcements</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_dht</alloc>
+    <dealloc arg="1">kill_dht</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_dht_groupchats</alloc>
+    <dealloc arg="1">kill_dht_groupchats</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_forwarding</alloc>
+    <dealloc arg="1">kill_forwarding</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_friend_connections</alloc>
+    <dealloc arg="1">kill_friend_connections</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_gca_list</alloc>
+    <dealloc arg="1">kill_gca_list</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_groupchats</alloc>
+    <dealloc arg="1">kill_groupchats</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_messenger</alloc>
+    <dealloc arg="1">kill_messenger</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_net_crypto</alloc>
+    <dealloc arg="1">kill_net_crypto</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_networking_ex</alloc>
+    <alloc init="true">new_networking_no_udp</alloc>
+    <dealloc arg="1">kill_networking</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_onion</alloc>
+    <dealloc arg="1">kill_onion</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_onion_announce</alloc>
+    <dealloc arg="1">kill_onion_announce</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_onion_client</alloc>
+    <dealloc arg="1">kill_onion_client</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_tcp_connections</alloc>
+    <dealloc arg="1">kill_tcp_connections</dealloc>
+  </resource>
+  <resource>
+    <alloc init="true">new_tcp_server</alloc>
+    <dealloc arg="1">kill_tcp_server</dealloc>
+  </resource>
+</def>
+<!-- vim:ft=xml
+ -->

other/fun/create_savedata.c

@@ -41,7 +41,7 @@ static bool create_tox(const unsigned char *const secret_key, Tox **const tox)
     return true;
 }
 
-static bool print_savedata(Tox *const tox)
+static bool print_savedata(const Tox *const tox)
 {
     const size_t savedata_size = tox_get_savedata_size(tox);
     uint8_t *const savedata = (uint8_t *)malloc(savedata_size);

other/fun/save-generator.c

@@ -61,7 +61,7 @@ static void tox_connection_callback(Tox *tox, Tox_Connection connection, void *u
     }
 }
 
-static void print_information(Tox *tox)
+static void print_information(const Tox *tox)
 {
     uint8_t tox_id[TOX_ADDRESS_SIZE];
     char tox_id_str[TOX_ADDRESS_SIZE * 2];

other/fun/sign.c

@@ -21,7 +21,7 @@
 #include "../../testing/misc_tools.h" // hex_string_to_bin
 #include "../../toxcore/ccompat.h"
 
-static int load_file(char *filename, unsigned char **result)
+static int load_file(const char *filename, unsigned char **result)
 {
     int size = 0;
     FILE *f = fopen(filename, "rb");

other/fun/strkey.c

@@ -45,7 +45,7 @@
 
 #define PRINT_TRIES_COUNT
 
-static void print_key(unsigned char *key)
+static void print_key(const unsigned char *key)
 {
     for (size_t i = 0; i < crypto_box_PUBLICKEYBYTES; ++i) {
         if (key[i] < 16) {
@@ -125,7 +125,7 @@ int main(int argc, char *argv[])
             }
         } while (!found);
     } else {
-        unsigned char *p = public_key + offset;
+        const unsigned char *p = public_key + offset;
 
         do {
 #ifdef PRINT_TRIES_COUNT

toxav/groupav.c

@@ -434,7 +434,7 @@ static int handle_group_audio_packet(void *object, uint32_t groupnumber, uint32_
     }
 
     while (decode_audio_packet((Group_AV *)object, peer_av, groupnumber, friendgroupnumber) == 0) {
-        continue;
+        /* Continue. */
     }
 
     return 0;
@@ -612,7 +612,7 @@ static int send_audio_packet(const Group_Chats *g_c, uint32_t groupnumber, const
  * @retval 0 on success.
  * @retval -1 on failure.
  */
-int group_send_audio(Group_Chats *g_c, uint32_t groupnumber, const int16_t *pcm, unsigned int samples, uint8_t channels,
+int group_send_audio(const Group_Chats *g_c, uint32_t groupnumber, const int16_t *pcm, unsigned int samples, uint8_t channels,
                      uint32_t sample_rate)
 {
     Group_AV *group_av = (Group_AV *)group_get_object(g_c, groupnumber);

toxav/groupav.h

@@ -40,7 +40,7 @@ int join_av_groupchat(const Logger *log, Tox *tox, Group_Chats *g_c, uint32_t fr
  * @retval 0 on success.
  * @retval -1 on failure.
  */
-int group_send_audio(Group_Chats *g_c, uint32_t groupnumber, const int16_t *pcm, unsigned int samples, uint8_t channels,
+int group_send_audio(const Group_Chats *g_c, uint32_t groupnumber, const int16_t *pcm, unsigned int samples, uint8_t channels,
                      uint32_t sample_rate);
 
 /** @brief Enable A/V in a groupchat.

toxav/msi.c

@@ -62,8 +62,8 @@ static void msg_init(MSIMessage *dest, MSIRequest request);
 static int msg_parse_in(const Logger *log, MSIMessage *dest, const uint8_t *data, uint16_t length);
 static uint8_t *msg_parse_header_out(MSIHeaderID id, uint8_t *dest, const void *value, uint8_t value_len,
                                      uint16_t *length);
-static int send_message(Messenger *m, uint32_t friend_number, const MSIMessage *msg);
-static int send_error(Messenger *m, uint32_t friend_number, MSIError error);
+static int send_message(const Messenger *m, uint32_t friend_number, const MSIMessage *msg);
+static int send_error(const Messenger *m, uint32_t friend_number, MSIError error);
 static bool invoke_callback(MSICall *call, MSICallbackID cb);
 static MSICall *get_call(MSISession *session, uint32_t friend_number);
 static MSICall *new_call(MSISession *session, uint32_t friend_number);
@@ -444,7 +444,7 @@ static uint8_t *msg_parse_header_out(MSIHeaderID id, uint8_t *dest, const void *
 
     return dest + value_len; /* Set to next position ready to be written */
 }
-static int send_message(Messenger *m, uint32_t friend_number, const MSIMessage *msg)
+static int send_message(const Messenger *m, uint32_t friend_number, const MSIMessage *msg)
 {
     /* Parse and send message */
     assert(m != nullptr);
@@ -489,7 +489,7 @@ static int send_message(Messenger *m, uint32_t friend_number, const MSIMessage *
 
     return -1;
 }
-static int send_error(Messenger *m, uint32_t friend_number, MSIError error)
+static int send_error(const Messenger *m, uint32_t friend_number, MSIError error)
 {
     /* Send error message */
     assert(m != nullptr);

toxcore/DHT.c

@@ -1930,14 +1930,11 @@ static void do_close(DHT *dht)
     for (size_t i = 0; i < LCLIENT_LIST; ++i) {
         Client_data *const client = &dht->close_clientlist[i];
 
-        IPPTsPng *const assocs[] = { &client->assoc6, &client->assoc4, nullptr };
-
-        for (IPPTsPng * const *it = assocs; *it != nullptr; ++it) {
-            IPPTsPng *const assoc = *it;
-
-            if (assoc->timestamp != 0) {
-                assoc->timestamp = badonly;
-            }
+        if (client->assoc4.timestamp != 0) {
+            client->assoc4.timestamp = badonly;
+        }
+        if (client->assoc6.timestamp != 0) {
+            client->assoc6.timestamp = badonly;
         }
     }
 }

toxcore/Messenger.c

@@ -3183,7 +3183,7 @@ static bool pack_groupchats_handler(Bin_Pack *bp, const Logger *log, const void
 non_null()
 static uint32_t saved_groups_size(const Messenger *m)
 {
-    GC_Session *c = m->group_handler;
+    const GC_Session *c = m->group_handler;
     return bin_pack_obj_size(pack_groupchats_handler, m->log, c);
 }
 

toxcore/Messenger.h

@@ -297,7 +297,7 @@ struct Messenger {
     m_friend_connectionstatuschange_internal_cb *friend_connectionstatuschange_internal;
     void *friend_connectionstatuschange_internal_userdata;
 
-    struct Group_Chats *conferences_object; /* Set by new_groupchats()*/
+    struct Group_Chats *conferences_object;
     m_conference_invite_cb *conference_invite;
 
     m_group_invite_cb *group_invite;