chore: Run static analysers in multiple variants.

Currently: 1) libsodium and 2) nacl. Note that the "nacl" variant is actually libsodium. We just want to make sure the static analysers see the `VANILLA_NACL` code paths.
2024-03-22 13:30:51 +08:00 · 2022-01-14 00:28:06 +00:00 · 2022-01-14 00:28:06 +00:00 · d23222c92f
commit d23222c92f
parent dfa7a0183f
15 changed files with 221 additions and 159 deletions
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@ -10,6 +10,7 @@ workflows:
      - tsan
      - msan
      # Static analysis
+      - clang-analyze
      - clang-tidy
      - infer
      - static-analysis
@ -21,21 +22,21 @@ jobs:
      - image: ubuntu

    steps:
-      - checkout
      - run: &apt_install
          apt-get update &&
          DEBIAN_FRONTEND=noninteractive
          apt-get install -y --no-install-recommends
            clang
            cmake
+            git
            libconfig-dev
            libgtest-dev
            libopus-dev
            libsodium-dev
            libvpx-dev
-            llvm-dev
            ninja-build
            pkg-config
+      - checkout
      - run: CC=clang .circleci/cmake-asan

  tsan:
@ -44,8 +45,8 @@ jobs:
      - image: ubuntu

    steps:
-      - checkout
      - run: *apt_install
+      - checkout
      - run: CC=clang .circleci/cmake-tsan

  msan:
@ -65,17 +66,16 @@ jobs:
      - image: toxchat/infer

    steps:
-      - run:
-          apt-get update &&
-          DEBIAN_FRONTEND=noninteractive
-          apt-get install -y --no-install-recommends
-            git
-            libopus-dev
-            libsodium-dev
-            libvpx-dev
-            pkg-config
+      - run: *apt_install
      - checkout
-      - run: infer --no-progress-bar -- cc toxav/*.c toxcore/*.c $(pkg-config --cflags opus vpx)
+      - run: infer --no-progress-bar -- cc
+          auto_tests/lossless_packet_test.c
+          testing/misc_tools.c
+          toxav/*.c
+          toxcore/*.c
+          toxencryptsave/*.c
+          -lpthread
+          $(pkg-config --cflags --libs libsodium opus vpx)

  static-analysis:
    working_directory: ~/work
@ -83,46 +83,35 @@ jobs:
      - image: ubuntu

    steps:
+      - run: *apt_install
+      - run: apt-get install -y --no-install-recommends cppcheck g++ llvm-dev
      - checkout
-      - run:
-          apt-get update &&
-          DEBIAN_FRONTEND=noninteractive
-          apt-get install -y --no-install-recommends
-            clang
-            cppcheck
-            g++
-            libconfig-dev
-            libgtest-dev
-            libopus-dev
-            libsodium-dev
-            libvpx-dev
-            llvm
      - run: other/analysis/check_logger_levels
      - run: other/analysis/run-check-recursion
      - run: other/analysis/run-clang
-      - run: other/analysis/run-clang-analyze
      - run: other/analysis/run-cppcheck
      - run: other/analysis/run-gcc

+  clang-analyze:
+    working_directory: ~/work
+    docker:
+      - image: ubuntu
+
+    steps:
+      - run: *apt_install
+      - checkout
+      - run: other/analysis/run-clang-analyze
+
  clang-tidy:
    working_directory: ~/work
    docker:
      - image: ubuntu

    steps:
+      - run: *apt_install
+      - run: apt-get install -y --no-install-recommends clang-tidy-12
      - checkout
-      - run:
-          apt-get update &&
-          DEBIAN_FRONTEND=noninteractive
-          apt-get install -y --no-install-recommends
-            build-essential
-            clang-tidy-11
-            cmake
-            libconfig-dev
-            libopus-dev
-            libsodium-dev
-            libvpx-dev
-      - run: cmake . -B_build -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
+      - run: cmake . -B_build -GNinja -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
      - run:
          other/analysis/run-clang-tidy ||
          other/analysis/run-clang-tidy ||
--- a/.cirrus.yml
+++ b/.cirrus.yml
@ -50,6 +50,7 @@ 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): Remove "|| true" once this works properly.
 bazel-msan_task:
  container:
    image: toxchat/toktok-stack:0.0.31-msan
--- a/.github/settings.yml
+++ b/.github/settings.yml
@ -14,6 +14,7 @@ branches:
        contexts:
          - "bazel-asan"
          - "bazel-debug"
+          - "bazel-msan"
          - "bazel-release"
          - "bazel-tsan"
          - "build-bootstrapd-docker"
@ -26,6 +27,7 @@ branches:
          - "CodeFactor"
          - "coverage-linux"
          - "ci/circleci: asan"
+          - "ci/circleci: clang-analyze"
          - "ci/circleci: clang-tidy"
          - "ci/circleci: infer"
          - "ci/circleci: static-analysis"
--- a/.restyled.yaml
+++ b/.restyled.yaml
@ -1,6 +1,8 @@
 ---
 exclude:
  - "**/*.api.h"
+    # shfmt doesn't support this file
+  - "other/analysis/run-clang-tidy"

 restylers:
  - astyle:
--- a/auto_tests/encryptsave_test.c
+++ b/auto_tests/encryptsave_test.c
@ -3,11 +3,8 @@
 #include <string.h>
 #include <sys/types.h>

-#ifdef VANILLA_NACL
-#include "../toxencryptsave/crypto_pwhash_scryptsalsa208sha256/crypto_pwhash_scryptsalsa208sha256.h"
-#else
+#ifndef VANILLA_NACL
 #include <sodium.h>
-#endif

 #include "../testing/misc_tools.h"
 #include "../toxcore/ccompat.h"
@ -217,3 +214,9 @@ int main(void)

    return 0;
 }
+#else  // VANILLA_NACL
+int main(void)
+{
+    return 0;
+}
+#endif
--- a/other/analysis/gen-file.sh
+++ b/other/analysis/gen-file.sh
@ -58,6 +58,10 @@ FIND_QUERY="$FIND_QUERY -and -not -name trace.cc"
 FIND_QUERY="$FIND_QUERY -and -not -name version_test.c"
 FIND_QUERY="$FIND_QUERY -and -not -wholename './testing/fuzzing/*'"

+if [ "$SKIP_GTEST" == 1 ]; then
+  FIND_QUERY="$FIND_QUERY -and -not -name '*_test.cc'"
+fi
+
 readarray -t FILES <<<"$(eval "$FIND_QUERY")"

 (for i in "${FILES[@]}"; do
--- a/other/analysis/run-check-recursion
+++ b/other/analysis/run-check-recursion
@ -1,5 +1,7 @@
 #!/bin/sh

+set -e
+
 cat toxav/*.c toxcore/*.c toxencryptsave/*.c |
  clang "$(pkg-config --cflags libsodium opus vpx)" \
    -Itoxav -Itoxcore -Itoxencryptsave -S -emit-llvm -xc - -o- |
--- a/other/analysis/run-clang
+++ b/other/analysis/run-clang
@ -2,27 +2,34 @@

 . other/analysis/gen-file.sh

-echo "Running Clang compiler"
-clang++ -o /dev/null amalgamation.cc \
-  "${CPPFLAGS[@]}" \
-  "${LDFLAGS[@]}" \
-  -std=c++11 \
-  -Werror \
-  -Weverything \
-  -Wno-alloca \
-  -Wno-c++98-compat-pedantic \
-  -Wno-c99-extensions \
-  -Wno-conversion \
-  -Wno-covered-switch-default \
-  -Wno-disabled-macro-expansion \
-  -Wno-documentation-deprecated-sync \
-  -Wno-global-constructors \
-  -Wno-missing-braces \
-  -Wno-missing-field-initializers \
-  -Wno-old-style-cast \
-  -Wno-padded \
-  -Wno-sign-compare \
-  -Wno-unreachable-code-return \
-  -Wno-unused-parameter \
-  -Wno-used-but-marked-unused \
-  -Wno-source-uses-openmp
+set -e
+
+run() {
+  echo "Running Clang compiler in variant '$*'"
+  clang++ -o /dev/null amalgamation.cc \
+    "${CPPFLAGS[@]}" \
+    "${LDFLAGS[@]}" \
+    "$@" \
+    -std=c++11 \
+    -Werror \
+    -Weverything \
+    -Wno-alloca \
+    -Wno-c++98-compat-pedantic \
+    -Wno-c99-extensions \
+    -Wno-conversion \
+    -Wno-covered-switch-default \
+    -Wno-disabled-macro-expansion \
+    -Wno-documentation-deprecated-sync \
+    -Wno-global-constructors \
+    -Wno-missing-braces \
+    -Wno-missing-field-initializers \
+    -Wno-old-style-cast \
+    -Wno-padded \
+    -Wno-sign-compare \
+    -Wno-unreachable-code-return \
+    -Wno-unused-parameter \
+    -Wno-used-but-marked-unused \
+    -Wno-source-uses-openmp
+}
+
+. other/analysis/variants.sh
--- a/other/analysis/run-clang-analyze
+++ b/other/analysis/run-clang-analyze
@ -2,7 +2,14 @@

 . other/analysis/gen-file.sh

-echo "Running Clang static analyzer"
-clang++ --analyze amalgamation.cc \
-  "${CPPFLAGS[@]}" \
-  -std=c++11
+set -e
+
+run() {
+  echo "Running Clang static analyzer in variant '$*'"
+  clang++ --analyze amalgamation.cc \
+    "${CPPFLAGS[@]}" \
+    "$@" \
+    -std=c++11
+}
+
+. other/analysis/variants.sh
--- a/other/analysis/run-clang-tidy
+++ b/other/analysis/run-clang-tidy
@ -1,6 +1,9 @@
-#!/bin/sh
+#!/bin/bash

+# TODO(iphydf): We might want some of these. For the ones we don't want, add a
+# comment explaining why not.
 CHECKS="*"
+CHECKS="$CHECKS,-altera-unroll-loops"
 CHECKS="$CHECKS,-android-cloexec-accept"
 CHECKS="$CHECKS,-android-cloexec-fopen"
 CHECKS="$CHECKS,-bugprone-not-null-terminated-result"
@ -18,24 +21,31 @@ CHECKS="$CHECKS,-llvmlibc-restrict-system-libc-headers"
 CHECKS="$CHECKS,-misc-redundant-expression"
 CHECKS="$CHECKS,-misc-unused-parameters"
 CHECKS="$CHECKS,-readability-else-after-return"
+CHECKS="$CHECKS,-readability-function-cognitive-complexity"
 CHECKS="$CHECKS,-readability-inconsistent-declaration-parameter-name"
 CHECKS="$CHECKS,-readability-magic-numbers"
 CHECKS="$CHECKS,-readability-redundant-control-flow"

+# TODO(iphydf): Maybe fix these?
+CHECKS="$CHECKS,-altera-id-dependent-backward-branch"
+CHECKS="$CHECKS,-altera-struct-pack-align"
+CHECKS="$CHECKS,-bugprone-branch-clone"
+CHECKS="$CHECKS,-bugprone-easily-swappable-parameters"
+CHECKS="$CHECKS,-bugprone-implicit-widening-of-multiplication-result"
+CHECKS="$CHECKS,-bugprone-integer-division"
+CHECKS="$CHECKS,-bugprone-narrowing-conversions"
+CHECKS="$CHECKS,-clang-analyzer-core.NonNullParamChecker"
+CHECKS="$CHECKS,-clang-analyzer-core.NullDereference"
+CHECKS="$CHECKS,-clang-analyzer-optin.portability.UnixAPI"
+CHECKS="$CHECKS,-clang-analyzer-unix.Malloc"
+CHECKS="$CHECKS,-clang-analyzer-valist.Uninitialized"
+CHECKS="$CHECKS,-concurrency-mt-unsafe"
+CHECKS="$CHECKS,-cppcoreguidelines-avoid-non-const-global-variables"
+CHECKS="$CHECKS,-cppcoreguidelines-narrowing-conversions"
+CHECKS="$CHECKS,-google-readability-casting"
+CHECKS="$CHECKS,-misc-no-recursion"
+
 ERRORS="*"
-# TODO(iphydf): Maybe fix these? Otherwise don't show them, if they are useless.
-ERRORS="$ERRORS,-bugprone-branch-clone"
-ERRORS="$ERRORS,-bugprone-integer-division"
-ERRORS="$ERRORS,-bugprone-narrowing-conversions"
-ERRORS="$ERRORS,-clang-analyzer-core.NonNullParamChecker"
-ERRORS="$ERRORS,-clang-analyzer-core.NullDereference"
-ERRORS="$ERRORS,-clang-analyzer-optin.portability.UnixAPI"
-ERRORS="$ERRORS,-clang-analyzer-unix.Malloc"
-ERRORS="$ERRORS,-clang-analyzer-valist.Uninitialized"
-ERRORS="$ERRORS,-cppcoreguidelines-avoid-non-const-global-variables"
-ERRORS="$ERRORS,-cppcoreguidelines-narrowing-conversions"
-ERRORS="$ERRORS,-google-readability-casting"
-ERRORS="$ERRORS,-misc-no-recursion"

 # TODO(iphydf): Fix these.
 ERRORS="$ERRORS,-bugprone-macro-parentheses"
@ -45,19 +55,29 @@ ERRORS="$ERRORS,-cert-err34-c"
 ERRORS="$ERRORS,-cert-str34-c"
 ERRORS="$ERRORS,-clang-analyzer-security.insecureAPI.strcpy"
 ERRORS="$ERRORS,-hicpp-uppercase-literal-suffix"
+ERRORS="$ERRORS,-readability-suspicious-call-argument"
 ERRORS="$ERRORS,-readability-uppercase-literal-suffix"

 set -eux

-clang-tidy-11 \
-  -p=_build \
-  --extra-arg=-DMIN_LOGGER_LEVEL=LOGGER_LEVEL_TRACE \
-  --checks="$CHECKS" \
-  --warnings-as-errors="$ERRORS" \
-  --use-color \
-  other/bootstrap_daemon/src/*.c \
-  other/*.c \
-  toxav/*.c \
-  toxcore/*.c \
-  toxencryptsave/*.c \
-  "$@"
+run() {
+  echo "Running clang-tidy in variant '$*'"
+  EXTRA_ARGS=("$@")
+  for i in "${!EXTRA_ARGS[@]}"; do
+    EXTRA_ARGS[$i]="--extra-arg=${EXTRA_ARGS[$i]}"
+  done
+  clang-tidy-12 \
+    -p=_build \
+    --extra-arg=-DMIN_LOGGER_LEVEL=LOGGER_LEVEL_TRACE \
+    "${EXTRA_ARGS[@]}" \
+    --checks="$CHECKS" \
+    --warnings-as-errors="$ERRORS" \
+    --use-color \
+    other/bootstrap_daemon/src/*.c \
+    other/*.c \
+    toxav/*.c \
+    toxcore/*.c \
+    toxencryptsave/*.c
+}
+
+. other/analysis/variants.sh
--- a/other/analysis/run-cppcheck
+++ b/other/analysis/run-cppcheck
@ -1,5 +1,14 @@
 #!/bin/bash

+SKIP_GTEST=1
+
 . other/analysis/gen-file.sh

-cppcheck amalgamation.cc "${CPPFLAGS[@]}"
+set -e
+
+run() {
+  echo "Running cppcheck in variant '$*'"
+  cppcheck amalgamation.cc "${CPPFLAGS[@]}" "$@"
+}
+
+. other/analysis/variants.sh
--- a/other/analysis/run-gcc
+++ b/other/analysis/run-gcc
@ -2,64 +2,71 @@

 . other/analysis/gen-file.sh

-echo "Running GCC"
-# TODO(iphydf): Get rid of all VLAs, then enable -fstack-protector -Wstack-protector
-g++ -O3 -o /dev/null amalgamation.cc \
-  "${CPPFLAGS[@]}" \
-  "${LDFLAGS[@]}" \
-  -std=c++11 \
-  -fdiagnostics-color=always \
-  -Wall \
-  -Wextra \
-  -Werror \
-  -Wno-aggressive-loop-optimizations \
-  -Wno-error=null-dereference \
-  -Wno-error=type-limits \
-  -Wno-float-conversion \
-  -Wno-format-signedness \
-  -Wno-missing-field-initializers \
-  -Wno-padded \
-  -Wno-sign-compare \
-  -Wno-sign-conversion \
-  -Wno-switch-default \
-  -Wno-unused-parameter \
-  -Wstrict-aliasing=0 \
-  -Wstrict-overflow=1 \
-  \
-  -Wmissing-declarations \
-  -Wbool-compare \
-  -Wcast-align \
-  -Wcast-qual \
-  -Wchar-subscripts \
-  -Wdouble-promotion \
-  -Wduplicated-cond \
-  -Wempty-body \
-  -Wenum-compare \
-  -Wfloat-equal \
-  -Wformat=2 \
-  -Wframe-address \
-  -Wframe-larger-than=133168 \
-  -Wignored-qualifiers \
-  -Wignored-attributes \
-  -Winit-self \
-  -Winline \
-  -Wlarger-than=133120 \
-  -Wmaybe-uninitialized \
-  -Wmemset-transposed-args \
-  -Wmisleading-indentation \
-  -Wnonnull \
-  -Wnonnull-compare \
-  -Wnull-dereference \
-  -Wodr \
-  -Wredundant-decls \
-  -Wreturn-type \
-  -Wshadow \
-  -Wsuggest-attribute=format \
-  -Wundef \
-  -Wunsafe-loop-optimizations \
-  -Wunused-label \
-  -Wunused-local-typedefs \
-  -Wunused-value \
-  -Wunused-but-set-parameter \
-  -Wunused-but-set-variable \
-  -fopenmp
+set -e
+
+run() {
+  echo "Running GCC in variant '$*'"
+  # TODO(iphydf): Get rid of all VLAs, then enable -fstack-protector -Wstack-protector
+  g++ -O3 -o /dev/null amalgamation.cc \
+    "${CPPFLAGS[@]}" \
+    "${LDFLAGS[@]}" \
+    "$@" \
+    -std=c++11 \
+    -fdiagnostics-color=always \
+    -Wall \
+    -Wextra \
+    -Werror \
+    -Wno-error=null-dereference \
+    -Wno-error=type-limits \
+    -Wno-aggressive-loop-optimizations \
+    -Wno-float-conversion \
+    -Wno-format-signedness \
+    -Wno-missing-field-initializers \
+    -Wno-padded \
+    -Wno-sign-compare \
+    -Wno-sign-conversion \
+    -Wno-switch-default \
+    -Wno-unused-parameter \
+    -Wstrict-aliasing=0 \
+    -Wstrict-overflow=1 \
+    \
+    -Wmissing-declarations \
+    -Wbool-compare \
+    -Wcast-align \
+    -Wcast-qual \
+    -Wchar-subscripts \
+    -Wdouble-promotion \
+    -Wduplicated-cond \
+    -Wempty-body \
+    -Wenum-compare \
+    -Wfloat-equal \
+    -Wformat=2 \
+    -Wframe-address \
+    -Wframe-larger-than=133168 \
+    -Wignored-qualifiers \
+    -Wignored-attributes \
+    -Winit-self \
+    -Winline \
+    -Wlarger-than=133120 \
+    -Wmaybe-uninitialized \
+    -Wmemset-transposed-args \
+    -Wmisleading-indentation \
+    -Wnonnull \
+    -Wnonnull-compare \
+    -Wnull-dereference \
+    -Wodr \
+    -Wredundant-decls \
+    -Wreturn-type \
+    -Wshadow \
+    -Wsuggest-attribute=format \
+    -Wundef \
+    -Wunsafe-loop-optimizations \
+    -Wunused-label \
+    -Wunused-local-typedefs \
+    -Wunused-value \
+    -Wunused-but-set-parameter \
+    -Wunused-but-set-variable \
+    -fopenmp
+}
+
+. other/analysis/variants.sh
--- a/other/analysis/run-infer
+++ b/other/analysis/run-infer
@ -3,6 +3,8 @@
 # Infer ignores everything that's not in the "current file".
 SKIP_LINES=1

+set -e
+
 . other/analysis/gen-file.sh

 infer --no-progress-bar -- clang++ -fsyntax-only amalgamation.cc "${CPPFLAGS[@]}"
--- a/other/analysis/variants.sh
+++ b/other/analysis/variants.sh
@ -0,0 +1,4 @@
+#!/bin/bash
+
+run
+run -DVANILLA_NACL -I/usr/include/sodium
--- a/toxcore/crypto_core.c
+++ b/toxcore/crypto_core.c
@ -26,6 +26,9 @@
 #include <crypto_verify_16.h>
 #include <crypto_verify_32.h>
 #include <randombytes.h>
+#endif
+
+#ifndef crypto_box_MACBYTES
 #define crypto_box_MACBYTES (crypto_box_ZEROBYTES - crypto_box_BOXZEROBYTES)
 #endif