Merge pull request #546 from atsign-foundation/fix-func-leaks

fix: functional test leaks

.github/workflows/build_source_tests.yaml

@@ -16,9 +16,9 @@ jobs:
       matrix:
         include:
           - cc: "gcc"
-            cflags: "-Wall -Wextra -Werror-implicit-function-declaration"
+            cflags: "-std=c99 -Wall -Wextra -Werror-implicit-function-declaration"
           - cc: "clang"
-            cflags: "-Wall -Wextra -Werror-implicit-function-declaration"
+            cflags: "-std=c99 -Wall -Wextra -Werror-implicit-function-declaration"
     runs-on: "ubuntu-latest"
     steps:
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2

.github/workflows/tests.yaml

@@ -15,15 +15,42 @@ jobs:
     runs-on: "ubuntu-latest"
     steps:
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+
+      - name: Install valgrind
+        run: |
+          sudo apt-get update
+          sudo apt-get install automake autoconf libtool libsubunit-dev valgrind
+
       - name: Build atSDK
         run: |
-          cmake -S . -B build -DATSDK_BUILD_TESTS="unit" -DCMAKE_BUILD_TYPE="Debug"
+          cmake -S . -B build \
+            -DATSDK_BUILD_TESTS="unit" \
+            -DCMAKE_BUILD_TYPE="Debug" \
+            -DCMAKE_C_FLAGS="-std=c99 -Wall -Wextra -Werror-implicit-function-declaration" \
+            -DATSDK_MEMCHECK=ON
+
           cmake --build build --target all
 
       - name: Run tests
         run: ctest --test-dir build --output-on-failure --timeout 5 -VV
 
+      # Unit tests are so fast we can run them serially and they will still
+      # finish before the functional tests.
+      # Long timeout because RSA keygen test sometimes takes a while
+      - name: Run memcheck tests
+        run: ctest -T memcheck --test-dir build --output-on-failure --timeout 60 -VV
+
   functional-tests:
+    strategy:
+      fail-fast: true
+      matrix:
+        include:
+          - type: normal
+            flags: ""
+            valgrind: false
+          - type: memcheck
+            flags: "-T memcheck"
+            valgrind: true
     runs-on: "ubuntu-latest"
     steps:
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
@@ -55,11 +82,19 @@ jobs:
           cmake --build build
           for i in {1..3}; do ./build/check_docker_readiness && break || sleep 5; done
 
+      - name: Install valgrind
+        if: ${{ matrix.valgrind == true }}
+        run: |
+          sudo apt-get update
+          sudo apt-get install automake autoconf libtool libsubunit-dev valgrind
+
       - name: Install atSDK
         run: |
           cmake -S . -B build                                       \
             -DATSDK_BUILD_TESTS="func"                              \
             -DCMAKE_BUILD_TYPE=Debug                                \
+            -DATSDK_MEMCHECK=ON                                     \
+            -DCMAKE_C_FLAGS="-std=c99 -Wall -Wextra -Werror-implicit-function-declaration" \
             -DATDIRECTORY_HOST="\"vip.ve.atsign.zone\""             \
             -DATDIRECTORY_PORT=64                                   \
             -DFIRST_ATSIGN="\"@alice🛠\""                           \
@@ -71,9 +106,9 @@ jobs:
 
           cmake --build build
 
-      - name: Run Functional Tests
+      - name: Run ${{ matrix.type }} Functional Tests
         run: |
-          ctest --test-dir build/tests/functional_tests -VV --timeout 90
+          ctest ${{ matrix.flags }} --test-dir build  --output-on-failure --timeout 90 -VV
 
   build-examples:
     runs-on: "ubuntu-latest"

justfile

@@ -16,6 +16,8 @@ unit_dir := quote(justfile_directory() / "build/unit-") + postfix
 func_dir := quote(justfile_directory() / "build/func-") + postfix
 memcheck_dir := quote(justfile_directory() / "build/memcheck-") + postfix
 
+c_flags := "-std=c99 -Wall -Wextra -Werror-implicit-function-declaration"
+
 # SETUP COMMANDS
 
 setup: configure-test-all
@@ -101,7 +103,7 @@ configure-debug:
     -DCMAKE_INSTALL_PREFIX="$HOME/.local/" \
     -DCMAKE_BUILD_TYPE=Debug \
     -DCMAKE_C_COMPILER=$C_COMPILER \
-    -DCMAKE_C_FLAGS="-std=c99 -Wno-error" \
+    -DCMAKE_C_FLAGS={{ c_flags }}\
     -DATSDK_BUILD_TESTS=OFF \
     -DATSDK_MEMCHECK=OFF
 
@@ -121,7 +123,7 @@ configure-test-unit:
     -G "$GENERATOR" \
     -DCMAKE_BUILD_TYPE=Debug \
     -DCMAKE_C_COMPILER=$C_COMPILER \
-    -DCMAKE_C_FLAGS="-std=c99 -Wno-error " \
+    -DCMAKE_C_FLAGS={{ c_flags }}\
     -DATSDK_BUILD_TESTS="unit" \
     -DATSDK_MEMCHECK=OFF \
     -DFIRST_ATSIGN="\"$FIRST_ATSIGN\"" \
@@ -133,7 +135,7 @@ configure-test-func:
     -G "$GENERATOR" \
     -DCMAKE_BUILD_TYPE=Debug \
     -DCMAKE_C_COMPILER=$C_COMPILER \
-    -DCMAKE_C_FLAGS="-std=c99 -Wno-error" \
+    -DCMAKE_C_FLAGS={{ c_flags }}\
     -DATSDK_BUILD_TESTS="func" \
     -DATSDK_MEMCHECK=OFF \
     -DATDIRECTORY_HOST="\"$ATDIRECTORY_HOST\"" \
@@ -152,7 +154,7 @@ configure-test-all:
     -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
     -DCMAKE_BUILD_TYPE=Debug \
     -DCMAKE_C_COMPILER=$C_COMPILER \
-    -DCMAKE_C_FLAGS="-std=c99 -Wno-error " \
+    -DCMAKE_C_FLAGS={{ c_flags }}\
     -DATSDK_BUILD_TESTS=ON \
     -DATSDK_MEMCHECK=OFF \
     -DATDIRECTORY_HOST="\"$ATDIRECTORY_HOST\"" \
@@ -170,12 +172,16 @@ configure-test-memcheck:
     -G "$GENERATOR" \
     -DCMAKE_BUILD_TYPE=Debug \
     -DCMAKE_C_COMPILER=$C_COMPILER \
-    -DCMAKE_C_FLAGS="-std=gnu99 -Wno-error" \
+    -DCMAKE_C_FLAGS={{ c_flags }}\
     -DATSDK_BUILD_TESTS=ON \
     -DBUILD_SHARED_LIBS=ON \
     -DATSDK_MEMCHECK=ON \
     -DFIRST_ATSIGN="\"$FIRST_ATSIGN\"" \
-    -DSECOND_ATSIGN="\"$SECOND_ATSIGN\""
+    -DSECOND_ATSIGN="\"$SECOND_ATSIGN\"" \
+    -DFIRST_ATSIGN_ATSERVER_HOST="\"$FIRST_ATSIGN_ATSERVER_HOST\"" \
+    -DFIRST_ATSIGN_ATSERVER_PORT=$FIRST_ATSIGN_ATSERVER_PORT \
+    -DSECOND_ATSIGN_ATSERVER_HOST="\"$SECOND_ATSIGN_ATSERVER_HOST\"" \
+    -DSECOND_ATSIGN_ATSERVER_PORT=$SECOND_ATSIGN_ATSERVER_PORT
 
 # DIAGNOSTIC COMMANDS
 

packages/atauth/tests/test_enroll_namespace.c

@@ -63,6 +63,7 @@ static int test_1a_parse_enroll_namespace_valid() {
                  ns.permissions[1]);
     return 1;
   }
+  enroll_namespace_free(&ns);
   return 0;
 }
 
@@ -162,6 +163,7 @@ static int test_3a_to_json() {
                  expected_json_3a, json);
   }
 
+  free(json);
   free(ns.namespaces[0]);
   free(ns.namespaces[1]);
   free(ns.namespaces);

packages/atclient/src/atclient_get_self_key.c

@@ -54,8 +54,6 @@ int atclient_get_self_key(atclient *atclient, atclient_atkey *atkey, char **valu
   char *value_raw = NULL;
   char *metadata_str = NULL;
 
-  atclient_atkey_metadata metadata;
-
   /*
    * 3. Build `llookup:` command
    */
@@ -115,17 +113,17 @@ int atclient_get_self_key(atclient *atclient, atclient_atkey *atkey, char **valu
 
   metadata_str = cJSON_Print(metadata_json);
 
-  if ((ret = atclient_atkey_metadata_from_json_str(&metadata, metadata_str)) != 0) {
+  if ((ret = atclient_atkey_metadata_from_json_str(&atkey->metadata, metadata_str)) != 0) {
     atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "atclient_atkey_metadata_from_json_str: %d\n", ret);
     goto exit;
   }
 
   /**
    * 6. Decrypt value
    */
-  if (atclient_atkey_metadata_is_iv_nonce_initialized(&metadata)) {
-    if ((ret = atchops_base64_decode(metadata.iv_nonce, strlen(metadata.iv_nonce), iv, ATCHOPS_IV_BUFFER_SIZE, NULL)) !=
-        0) {
+  if (atclient_atkey_metadata_is_iv_nonce_initialized(&atkey->metadata)) {
+    if ((ret = atchops_base64_decode(atkey->metadata.iv_nonce, strlen(atkey->metadata.iv_nonce), iv,
+                                     ATCHOPS_IV_BUFFER_SIZE, NULL)) != 0) {
       atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "atchops_base64_decode: %d\n", ret);
       goto exit;
     }

packages/atclient/src/atclient_get_shared_key.c

@@ -304,20 +304,23 @@ atclient_get_shared_key_shared_by_me_with_other(atclient *atclient, atclient_atk
   if ((ret = atchops_base64_decode(value_raw_encrypted_base64, value_raw_encrypted_base64_len, value_raw_encrypted,
                                    value_raw_encrypted_size, &value_raw_encrypted_len)) != 0) {
     atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "atchops_base64_decode: %d\n", ret);
+    free(value_raw_encrypted);
     goto exit;
   }
 
   const size_t value_raw_size = atchops_aes_ctr_plaintext_size(value_raw_encrypted_len);
   if ((value_raw = malloc(sizeof(unsigned char) * value_raw_size)) == NULL) {
     ret = 1;
     atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "Failed to allocate memory for value_raw\n");
+    free(value_raw_encrypted);
     goto exit;
   }
   memset(value_raw, 0, sizeof(unsigned char) * value_raw_size);
   size_t value_raw_len = 0;
-
-  if ((ret = atchops_aes_ctr_decrypt(shared_encryption_key_to_use, ATCHOPS_AES_256, iv, value_raw_encrypted,
-                                     value_raw_encrypted_len, value_raw, value_raw_size, &value_raw_len)) != 0) {
+  ret = atchops_aes_ctr_decrypt(shared_encryption_key_to_use, ATCHOPS_AES_256, iv, value_raw_encrypted,
+                                value_raw_encrypted_len, value_raw, value_raw_size, &value_raw_len);
+  free(value_raw_encrypted);
+  if (ret != 0) {
     atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "atchops_aes_ctr_decrypt: %d\n", ret);
     goto exit;
   }

packages/atclient/src/atkey.c

@@ -7,7 +7,15 @@
 #include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
+
+// TODO remove use of strtok_r so we can conform to C99 instead of POSIX 2001
+#if _POSIX_C_SOURCE < 200112L
+#undef _POSIX_C_SOURCE
+#define _POSIX_C_SOURCE 200112L
+#endif
 #include <string.h>
+extern char *strtok_r(char *, const char *, char **);
+// END (keep string.h include after removing this temp fix)
 
 #define TAG "atkey"
 

packages/atclient/src/encryption_key_helpers.c

@@ -5,6 +5,7 @@
 #include "atclient/atkeys.h"
 #include "atclient/constants.h"
 #include "atclient/string_utils.h"
+#include "atcommons/memory_util.h"
 #include "atlogger/atlogger.h"
 #include <atchops/platform.h>
 #include <stdlib.h>
@@ -39,8 +40,6 @@ int atclient_get_public_encryption_key(atclient *ctx, const char *atsign, char *
    */
 
   char *atsign_with_at = NULL;
-  char *atsign_without_at = NULL;
-
   char *command = NULL;
 
   const size_t recv_size = 1024; // sufficient buffer size to receive the public key
@@ -56,11 +55,6 @@ int atclient_get_public_encryption_key(atclient *ctx, const char *atsign, char *
     goto exit;
   }
 
-  if ((ret = atclient_string_utils_atsign_without_at(atsign_with_at, &atsign_without_at)) != 0) {
-    atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "atclient_string_utils_atsign_without_at: %d\n", ret);
-    goto exit;
-  }
-
   const size_t commandsize = strlen("plookup:publickey") + strlen(atsign_with_at) + strlen("\r\n") + 1;
   if ((command = (char *)malloc(sizeof(char) * commandsize)) == NULL) {
     ret = 1;
@@ -107,7 +101,15 @@ int atclient_get_public_encryption_key(atclient *ctx, const char *atsign, char *
   (*public_encryption_key)[public_encryption_key_len] = '\0';
 
   ret = 0;
-exit: { return ret; }
+exit: {
+  if (atsign_with_at != NULL) {
+    free(atsign_with_at);
+  }
+  if (command != NULL) {
+    free(command);
+  }
+  return ret;
+}
 }
 
 int atclient_get_shared_encryption_key_shared_by_me(atclient *ctx, const char *recipient_atsign,