From 835b9fbdc933878a744cd6ba1c77942610f4fe06 Mon Sep 17 00:00:00 2001 From: iphydf Date: Sat, 3 Feb 2018 16:13:22 +0000 Subject: [PATCH] Improve stability of crypto_memcmp test. Also reduce number of people in conference to 5, because on Circle CI the test times out trying to connect more than 6 or 7 people. The persistent conferences PR will improve this so we can set it much higher then. --- .travis.yml | 1 + CMakeLists.txt | 44 ++++++++++++++++- auto_tests/conference_test.c | 2 +- auto_tests/crypto_test.c | 78 ----------------------------- circle.yml | 2 +- other/travis/env.sh | 1 + other/travis/toxcore-script | 1 + toxcore/BUILD.bazel | 9 ++++ toxcore/crypto_core_test.cpp | 96 ++++++++++++++++++++++++++++++++++++ 9 files changed, 153 insertions(+), 81 deletions(-) create mode 100644 toxcore/crypto_core_test.cpp diff --git a/.travis.yml b/.travis.yml index 48f5ce3a..1b1a1606 100644 --- a/.travis.yml +++ b/.travis.yml @@ -24,6 +24,7 @@ matrix: - libopencv-contrib-dev # For av_test. - libopus-dev # For toxav. - libsndfile1-dev # For av_test. + - libgtest-dev # For unit tests. - libvpx-dev # For toxav. - opam # For apidsl and Frama-C. - portaudio19-dev # For av_test. diff --git a/CMakeLists.txt b/CMakeLists.txt index 542f151d..8cd5bae6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -429,7 +429,49 @@ install_module(toxcore DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/tox) ################################################################################ # -# :: Automated regression tests +# :: Unit tests: no networking, just pure function calls. +# +################################################################################ + +# Compile the GTest library. +# +if(EXISTS "/usr/src/gtest/src/gtest-all.cc") + add_library(gtest + /usr/src/gtest/src/gtest-all.cc + /usr/src/gtest/src/gtest_main.cc) + target_include_directories(gtest PRIVATE /usr/src/gtest) + check_cxx_compiler_flag("-w" HAVE_CXX_W QUIET) + check_cxx_compiler_flag("-Wno-global-constructors" HAVE_CXX_W_NO_GLOBAL_CONSTRUCTORS QUIET) + check_cxx_compiler_flag("-Wno-zero-as-null-pointer-constant" HAVE_CXX_W_NO_ZERO_AS_NULL_POINTER_CONSTANT QUIET) + if(HAVE_CXX_W) + set_target_properties(gtest PROPERTIES COMPILE_FLAGS "-w") + endif() + set(HAVE_GTEST TRUE) +endif() + +function(unit_test subdir target) + if(HAVE_GTEST) + add_executable(unit_${target}_test ${subdir}/${target}_test.cpp) + target_link_modules(unit_${target}_test ${toxcore_SUBLIBS} gtest) + set(gtest_CFLAGS "") + if(HAVE_CXX_W_NO_GLOBAL_CONSTRUCTORS) + set(gtest_CFLAGS "${gtest_CFLAGS} -Wno-global-constructors") + endif() + if(HAVE_CXX_W_NO_ZERO_AS_NULL_POINTER_CONSTANT) + set(gtest_CFLAGS "${gtest_CFLAGS} -Wno-zero-as-null-pointer-constant") + endif() + set_target_properties(unit_${target}_test PROPERTIES COMPILE_FLAGS "${gtest_CFLAGS}") + add_test(NAME ${target} COMMAND ${CROSSCOMPILING_EMULATOR} unit_${target}_test) + endif() +endfunction() + +# The actual unit tests follow. +# +unit_test(toxcore crypto_core) + +################################################################################ +# +# :: Automated regression tests: create a tox network and run integration tests # ################################################################################ diff --git a/auto_tests/conference_test.c b/auto_tests/conference_test.c index 2fbb9c98..bd3ea8d0 100644 --- a/auto_tests/conference_test.c +++ b/auto_tests/conference_test.c @@ -20,7 +20,7 @@ #include "helpers.h" -#define NUM_GROUP_TOX 8 +#define NUM_GROUP_TOX 5 static void handle_self_connection_status(Tox *tox, TOX_CONNECTION connection_status, void *user_data) { diff --git a/auto_tests/crypto_test.c b/auto_tests/crypto_test.c index a1b7fd30..045cd7ab 100644 --- a/auto_tests/crypto_test.c +++ b/auto_tests/crypto_test.c @@ -339,83 +339,6 @@ START_TEST(test_memzero) } END_TEST -#define CRYPTO_TEST_MEMCMP_SIZE 1024*1024 // 1MiB -#define CRYPTO_TEST_MEMCMP_COUNT 2000 -#define CRYPTO_TEST_MEMCMP_EPS 10 - -static int cmp(const void *a, const void *b) -{ - const clock_t *first = (const clock_t *) a; - const clock_t *second = (const clock_t *) b; - - if (*first < *second) { - return -1; - } - - if (*first > *second) { - return 1; - } - - return 0; -} - -static clock_t memcmp_time(void *a, void *b, size_t len) -{ - clock_t start = clock(); - crypto_memcmp(a, b, len); - return clock() - start; -} - -static clock_t memcmp_median(void *a, void *b, size_t len) -{ - size_t i; - clock_t results[CRYPTO_TEST_MEMCMP_COUNT]; - - for (i = 0; i < CRYPTO_TEST_MEMCMP_COUNT; i++) { - results[i] = memcmp_time(a, b, len); - } - - qsort(results, CRYPTO_TEST_MEMCMP_COUNT, sizeof(*results), cmp); - return results[CRYPTO_TEST_MEMCMP_COUNT / 2]; -} - -START_TEST(test_memcmp) -{ - uint8_t *src = (uint8_t *)malloc(CRYPTO_TEST_MEMCMP_SIZE); - rand_bytes(src, CRYPTO_TEST_MEMCMP_SIZE); - - uint8_t *same = (uint8_t *)malloc(CRYPTO_TEST_MEMCMP_SIZE); - memcpy(same, src, CRYPTO_TEST_MEMCMP_SIZE); - - uint8_t *not_same = (uint8_t *)malloc(CRYPTO_TEST_MEMCMP_SIZE); - rand_bytes(not_same, CRYPTO_TEST_MEMCMP_SIZE); - - printf("timing memcmp (equal arrays)\n"); - clock_t same_median = memcmp_median(src, same, CRYPTO_TEST_MEMCMP_SIZE); - printf("timing memcmp (non-equal arrays)\n"); - clock_t not_same_median = memcmp_median(src, not_same, CRYPTO_TEST_MEMCMP_SIZE); - printf("comparing times\n"); - - free(not_same); - free(same); - free(src); - - clock_t delta; - - if (same_median > not_same_median) { - delta = same_median - not_same_median; - } else { - delta = not_same_median - same_median; - } - - ck_assert_msg(delta < CRYPTO_TEST_MEMCMP_EPS, - "Delta time is too long (%d >= %d)\n" - "Time of the same data comparation: %d\n" - "Time of the different data comparation: %d", - delta, CRYPTO_TEST_MEMCMP_EPS, same_median, not_same_median); -} -END_TEST - static Suite *crypto_suite(void) { Suite *s = suite_create("Crypto"); @@ -427,7 +350,6 @@ static Suite *crypto_suite(void) DEFTESTCASE(large_data_symmetric); DEFTESTCASE_SLOW(increment_nonce, 20); DEFTESTCASE(memzero); - DEFTESTCASE_SLOW(memcmp, 30); return s; } diff --git a/circle.yml b/circle.yml index d0d2836c..44bf8c7f 100644 --- a/circle.yml +++ b/circle.yml @@ -137,4 +137,4 @@ compile: test: override: - - make VERBOSE=1 test || make VERBOSE=1 ARGS="--rerun-failed" test || make VERBOSE=1 ARGS="--rerun-failed" test || make VERBOSE=1 ARGS="--rerun-failed" test + - make test ARGS="--rerun-failed" CTEST_OUTPUT_ON_FAILURE=1 || make test ARGS="--rerun-failed" CTEST_OUTPUT_ON_FAILURE=1 || make test ARGS="--rerun-failed" CTEST_OUTPUT_ON_FAILURE=1 || make test ARGS="--rerun-failed" CTEST_OUTPUT_ON_FAILURE=1 || make test ARGS="--rerun-failed" CTEST_OUTPUT_ON_FAILURE=1 diff --git a/other/travis/env.sh b/other/travis/env.sh index f4ef8ef3..7d90d395 100644 --- a/other/travis/env.sh +++ b/other/travis/env.sh @@ -7,6 +7,7 @@ export LD_LIBRARY_PATH=$CACHE_DIR/lib export PKG_CONFIG_PATH=$CACHE_DIR/lib/pkgconfig export ASTYLE=$CACHE_DIR/astyle/build/gcc/bin/astyle export CFLAGS="-O3 -DTRAVIS_ENV=1" +export CXXFLAGS="-O3 -DTRAVIS_ENV=1" export CMAKE_EXTRA_FLAGS="-DERROR_ON_WARNING=ON" export MAKE=make diff --git a/other/travis/toxcore-script b/other/travis/toxcore-script index 763be2e4..82c39ce9 100755 --- a/other/travis/toxcore-script +++ b/other/travis/toxcore-script @@ -2,6 +2,7 @@ # Enable test coverage recording. export CFLAGS="$CFLAGS -fprofile-arcs -ftest-coverage" +export CXXFLAGS="$CXXFLAGS -fprofile-arcs -ftest-coverage" set_opt() { opts="$opts -D$1="`expr ON \& \( $i % 2 \) \| OFF` diff --git a/toxcore/BUILD.bazel b/toxcore/BUILD.bazel index e9ffdbd2..3b96dd6d 100644 --- a/toxcore/BUILD.bazel +++ b/toxcore/BUILD.bazel @@ -28,6 +28,15 @@ cc_library( ], ) +cc_test( + name = "crypto_core_test", + srcs = ["crypto_core_test.cpp"], + deps = [ + ":crypto_core", + "@gtest", + ], +) + cc_library( name = "list", srcs = ["list.c"], diff --git a/toxcore/crypto_core_test.cpp b/toxcore/crypto_core_test.cpp new file mode 100644 index 00000000..8f91dce8 --- /dev/null +++ b/toxcore/crypto_core_test.cpp @@ -0,0 +1,96 @@ +#include "crypto_core.h" + +#include + +#include + +namespace +{ + +enum { + /** + * The size of the arrays to compare. This was chosen to take around 2000 + * CPU clocks on x86_64. + */ + CRYPTO_TEST_MEMCMP_SIZE = 1024 * 1024, // 1 MiB + /** + * The number of times we run memcmp in the test. + * + * We compute the median time taken to reduce error margins. + */ + CRYPTO_TEST_MEMCMP_ITERATIONS = 500, + /** + * The margin of error (in clocks) we allow for this test. + * + * Should be within 0.5% of ~2000 CPU clocks. In reality, the code is much + * more precise and is usually within 1 CPU clock. + */ + CRYPTO_TEST_MEMCMP_EPS = 10, +}; + +clock_t memcmp_time(void *a, void *b, size_t len) +{ + clock_t start = clock(); + crypto_memcmp(a, b, len); + return clock() - start; +} + +/** + * This function performs the actual timing. It interleaves comparison of + * equal and non-equal arrays to reduce the influence of external effects + * such as the machine being a little more busy 1 second later. + */ +void memcmp_median(void *src, void *same, void *not_same, size_t len, + clock_t *same_median, clock_t *not_same_median) +{ + clock_t same_results[CRYPTO_TEST_MEMCMP_ITERATIONS]; + clock_t not_same_results[CRYPTO_TEST_MEMCMP_ITERATIONS]; + + for (size_t i = 0; i < CRYPTO_TEST_MEMCMP_ITERATIONS; i++) { + same_results[i] = memcmp_time(src, same, len); + not_same_results[i] = memcmp_time(src, not_same, len); + } + + std::sort(same_results, same_results + CRYPTO_TEST_MEMCMP_ITERATIONS); + *same_median = same_results[CRYPTO_TEST_MEMCMP_ITERATIONS / 2]; + std::sort(not_same_results, not_same_results + CRYPTO_TEST_MEMCMP_ITERATIONS); + *not_same_median = not_same_results[CRYPTO_TEST_MEMCMP_ITERATIONS / 2]; +} + +/** + * This test checks whether crypto_memcmp takes the same time for equal and + * non-equal chunks of memory. + */ +TEST(CryptoCore, MemcmpTimingIsDataIndependent) +{ + // A random piece of memory. + uint8_t *src = new uint8_t[CRYPTO_TEST_MEMCMP_SIZE]; + random_bytes(src, CRYPTO_TEST_MEMCMP_SIZE); + + // A separate piece of memory containing the same data. + uint8_t *same = new uint8_t[CRYPTO_TEST_MEMCMP_SIZE]; + memcpy(same, src, CRYPTO_TEST_MEMCMP_SIZE); + + // Another piece of memory containing different data. + uint8_t *not_same = new uint8_t[CRYPTO_TEST_MEMCMP_SIZE]; + random_bytes(not_same, CRYPTO_TEST_MEMCMP_SIZE); + + clock_t same_median; + clock_t not_same_median; + memcmp_median(src, same, not_same, CRYPTO_TEST_MEMCMP_SIZE, &same_median, ¬_same_median); + + delete[] not_same; + delete[] same; + delete[] src; + + clock_t const delta = same_median > not_same_median + ? same_median - not_same_median + : not_same_median - same_median; + + EXPECT_LT(delta, CRYPTO_TEST_MEMCMP_EPS) + << "Delta time is too long (" << delta << " >= " << CRYPTO_TEST_MEMCMP_EPS << ")\n" + << "Time of the same data comparation: " << same_median << " clocks\n" + << "Time of the different data comparation: " << not_same_median << " clocks"; +} + +} // namespace