From fd91bbdd7bab5c0a6a5116c193a632f646639747 Mon Sep 17 00:00:00 2001 From: iphydf Date: Wed, 8 Dec 2021 23:01:13 +0000 Subject: [PATCH] test: Add a simple test for `ip_ntoa`. Just to demonstrate that it will never exceed its input buffer. --- .circleci/config.yml | 24 +++++++- .github/settings.yml | 1 + .github/workflows/clang-tidy-review.yml | 28 ---------- build/Makefile.am | 2 + other/analysis/run-clang-tidy | 62 +++++++++++++++++++++ other/bootstrap_daemon/src/tox-bootstrapd.c | 2 +- toxcore/BUILD.bazel | 10 ++++ toxcore/ccompat.h | 2 +- toxcore/network.c | 6 +- toxcore/network_test.cc | 20 +++++++ 10 files changed, 123 insertions(+), 34 deletions(-) delete mode 100644 .github/workflows/clang-tidy-review.yml create mode 100755 other/analysis/run-clang-tidy create mode 100644 toxcore/network_test.cc diff --git a/.circleci/config.yml b/.circleci/config.yml index 26551c2b..c3f588e7 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -9,8 +9,9 @@ workflows: - asan - tsan # Static analysis - - static-analysis + - clang-tidy - infer + - static-analysis jobs: asan: @@ -89,3 +90,24 @@ jobs: - run: other/analysis/run-clang-analyze - run: other/analysis/run-cppcheck - run: other/analysis/run-gcc + + clang-tidy: + working_directory: ~/work + docker: + - image: ubuntu + + steps: + - 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: other/analysis/run-clang-tidy diff --git a/.github/settings.yml b/.github/settings.yml index b7828228..3555e5b6 100644 --- a/.github/settings.yml +++ b/.github/settings.yml @@ -15,6 +15,7 @@ branches: - Codacy Static Code Analysis - CodeFactor - "ci/circleci: asan" + - "ci/circleci: clang-tidy" - "ci/circleci: infer" - "ci/circleci: static-analysis" - "ci/circleci: tsan" diff --git a/.github/workflows/clang-tidy-review.yml b/.github/workflows/clang-tidy-review.yml deleted file mode 100644 index f2ecde97..00000000 --- a/.github/workflows/clang-tidy-review.yml +++ /dev/null @@ -1,28 +0,0 @@ -name: clang-tidy-review - -on: - pull_request: - branches: [master] - -jobs: - build: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v2 - - name: Install compiledb - run: pip install compiledb - - name: Install dependencies - run: - sudo apt-get install -y --no-install-recommends - cmake - libconfig-dev - libopus-dev - libsodium-dev - libvpx-dev - pkg-config - - name: Generate compile_commands.json - run: - cmake . && compiledb make -j4 - - - uses: ZedThree/clang-tidy-review@v0.7.0 - id: review diff --git a/build/Makefile.am b/build/Makefile.am index d9d2911e..cbaca765 100644 --- a/build/Makefile.am +++ b/build/Makefile.am @@ -24,3 +24,5 @@ include ../other/Makefile.inc include ../testing/Makefile.inc include ../other/bootstrap_daemon/src/Makefile.inc include ../auto_tests/Makefile.inc + +build-tests: $(check_PROGRAMS) diff --git a/other/analysis/run-clang-tidy b/other/analysis/run-clang-tidy new file mode 100755 index 00000000..e4afb111 --- /dev/null +++ b/other/analysis/run-clang-tidy @@ -0,0 +1,62 @@ +#!/bin/sh + +CHECKS="*" +CHECKS="$CHECKS,-bugprone-not-null-terminated-result" +CHECKS="$CHECKS,-bugprone-reserved-identifier" +CHECKS="$CHECKS,-bugprone-sizeof-expression" +CHECKS="$CHECKS,-cert-dcl37-c" +CHECKS="$CHECKS,-cert-dcl51-cpp" +CHECKS="$CHECKS,-clang-analyzer-optin.performance.Padding" +CHECKS="$CHECKS,-cppcoreguidelines-avoid-magic-numbers" +CHECKS="$CHECKS,-cppcoreguidelines-init-variables" +CHECKS="$CHECKS,-hicpp-multiway-paths-covered" +CHECKS="$CHECKS,-hicpp-signed-bitwise" +CHECKS="$CHECKS,-llvm-else-after-return" +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-inconsistent-declaration-parameter-name" +CHECKS="$CHECKS,-readability-magic-numbers" + +ERRORS="*" +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-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" +ERRORS="$ERRORS,-readability-redundant-control-flow" + +# TODO(iphydf): Fix these. +ERRORS="$ERRORS,-android-cloexec-accept" +ERRORS="$ERRORS,-android-cloexec-fopen" +ERRORS="$ERRORS,-bugprone-macro-parentheses" +ERRORS="$ERRORS,-bugprone-posix-return" +ERRORS="$ERRORS,-bugprone-signed-char-misuse" +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-non-const-parameter" +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 \ + "$@" diff --git a/other/bootstrap_daemon/src/tox-bootstrapd.c b/other/bootstrap_daemon/src/tox-bootstrapd.c index a64ebb30..5afe2776 100644 --- a/other/bootstrap_daemon/src/tox-bootstrapd.c +++ b/other/bootstrap_daemon/src/tox-bootstrapd.c @@ -12,9 +12,9 @@ #endif // system provided +#include // system header, rather than C, because we need it for POSIX sigaction(2) #include #include -#include // system header, rather than C, because we need it for POSIX sigaction(2) #include // C diff --git a/toxcore/BUILD.bazel b/toxcore/BUILD.bazel index e6ac3901..de696f67 100644 --- a/toxcore/BUILD.bazel +++ b/toxcore/BUILD.bazel @@ -115,6 +115,16 @@ cc_library( ], ) +cc_test( + name = "network_test", + size = "small", + srcs = ["network_test.cc"], + deps = [ + ":network", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "util_test", size = "small", diff --git a/toxcore/ccompat.h b/toxcore/ccompat.h index 4340ac6a..6f870de9 100644 --- a/toxcore/ccompat.h +++ b/toxcore/ccompat.h @@ -43,7 +43,7 @@ #if !defined(__cplusplus) || __cplusplus < 201103L #define nullptr NULL #ifndef static_assert -#define static_assert(cond, msg) extern int unused_for_static_assert +#define static_assert(cond, msg) extern const int unused_for_static_assert #endif #endif diff --git a/toxcore/network.c b/toxcore/network.c index f2bbbf09..a949ffb7 100644 --- a/toxcore/network.c +++ b/toxcore/network.c @@ -1129,7 +1129,7 @@ const char *ip_ntoa(const IP *ip, char *ip_str, size_t length) struct in_addr addr; fill_addr4(ip->ip.v4, &addr); - ip_str[0] = 0; + ip_str[0] = '\0'; assert(make_family(ip->family) == AF_INET); inet_ntop4(&addr, ip_str, length); } else if (net_family_is_ipv6(ip->family)) { @@ -1142,7 +1142,7 @@ const char *ip_ntoa(const IP *ip, char *ip_str, size_t length) inet_ntop6(&addr, &ip_str[1], length - 3); const size_t len = strlen(ip_str); ip_str[len] = ']'; - ip_str[len + 1] = 0; + ip_str[len + 1] = '\0'; } else { snprintf(ip_str, length, "(IP invalid, family %u)", ip->family.value); } @@ -1151,7 +1151,7 @@ const char *ip_ntoa(const IP *ip, char *ip_str, size_t length) } /* brute force protection against lacking termination */ - ip_str[length - 1] = 0; + ip_str[length - 1] = '\0'; return ip_str; } diff --git a/toxcore/network_test.cc b/toxcore/network_test.cc new file mode 100644 index 00000000..c2f1d221 --- /dev/null +++ b/toxcore/network_test.cc @@ -0,0 +1,20 @@ +#include "network.h" + +#include + +namespace { + +TEST(IpNtoa, DoesntWriteOutOfBounds) { + char ip_str[IP_NTOA_LEN]; + IP ip; + ip.family = net_family_ipv6; + ip.ip.v6.uint64[0] = -1; + ip.ip.v6.uint64[1] = -1; + + ip_ntoa(&ip, ip_str, sizeof(ip_str)); + + EXPECT_EQ(std::string(ip_str), "[ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff]"); + EXPECT_LT(std::string(ip_str).length(), IP_NTOA_LEN); +} + +} // namespace