From a1035cf81494c3ab22c49b01788ca8ad933c5eb0 Mon Sep 17 00:00:00 2001 From: iphydf Date: Sun, 26 Aug 2018 09:42:08 +0000 Subject: [PATCH] Add some tests for `ping_array`. No timeout test here yet, because we don't yet have the ability to manipulate time at will, so we would have to actually sleep. --- CMakeLists.txt | 1 + other/astyle/format-source | 16 ++++- testing/random_testing.cc | 25 +++++--- toxcore/BUILD.bazel | 9 +++ toxcore/crypto_core_test.cc | 4 +- toxcore/ping_array.api.h | 29 +++++---- toxcore/ping_array.c | 59 +++++++----------- toxcore/ping_array.h | 29 +++++---- toxcore/ping_array_test.cc | 119 ++++++++++++++++++++++++++++++++++++ 9 files changed, 218 insertions(+), 73 deletions(-) create mode 100644 toxcore/ping_array_test.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index a867e1dd..95b12ce0 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -362,6 +362,7 @@ unit_test(toxav ring_buffer) unit_test(toxav rtp) unit_test(toxcore crypto_core) unit_test(toxcore mono_time) +unit_test(toxcore ping_array) unit_test(toxcore util) ################################################################################ diff --git a/other/astyle/format-source b/other/astyle/format-source index 5e139d89..550bcf07 100755 --- a/other/astyle/format-source +++ b/other/astyle/format-source @@ -49,15 +49,25 @@ if grep '' */*.h; then exit 1 fi +CC_SOURCES=`find . '(' -name '*.cc' ')'` +CC_SOURCES="$CC_SOURCES toxcore/ping_array.c" + +for bin in clang-format-6.0 clang-format-5.0 clang-format; do + if which "$bin"; then + "$bin" -i -style='{BasedOnStyle: Google, ColumnLimit: 100}' $CC_SOURCES + break + fi +done + FIND="find ." -FIND="$FIND '(' -name '*.[ch]' -or -name '*.cpp' ')'" +FIND="$FIND '(' -name '*.[ch]' ')'" FIND="$FIND -and -not -name '*.api.h'" FIND="$FIND -and -not -wholename './super_donators/*'" FIND="$FIND -and -not -wholename './third_party/*'" FIND="$FIND -and -not -wholename './toxencryptsave/crypto_pwhash*'" -SOURCES=`eval "$FIND"` +C_SOURCES=`eval "$FIND"` -$ASTYLE -n --options=other/astyle/astylerc $SOURCES +$ASTYLE -n --options=other/astyle/astylerc $C_SOURCES git diff --exit-code diff --git a/testing/random_testing.cc b/testing/random_testing.cc index 5ea0bc9d..6211630e 100644 --- a/testing/random_testing.cc +++ b/testing/random_testing.cc @@ -283,7 +283,8 @@ bool attempt_action(Global_State *toxes, std::mt19937 *rng) { int main() { std::vector const actions = { { - 10, "creates a new conference", + 10, + "creates a new conference", [](Local_State const &state) { return tox_conference_get_chatlist_size(state.tox()) < MAX_CONFERENCES_PER_USER; }, @@ -294,7 +295,8 @@ int main() { }, }, { - 10, "invites a random friend to a conference", + 10, + "invites a random friend to a conference", [](Local_State const &state) { return tox_conference_get_chatlist_size(state.tox()) != 0; }, @@ -302,15 +304,15 @@ int main() { size_t chat_count = tox_conference_get_chatlist_size(state->tox()); assert(chat_count != 0); // Condition above. TOX_ERR_CONFERENCE_INVITE err; - tox_conference_invite( - state->tox(), rnd->friend_selector(*rng), - state->next_invite % chat_count, &err); + tox_conference_invite(state->tox(), rnd->friend_selector(*rng), + state->next_invite % chat_count, &err); state->next_invite++; assert(err == TOX_ERR_CONFERENCE_INVITE_OK); }, }, { - 10, "deletes the last conference", + 10, + "deletes the last conference", [](Local_State const &state) { return tox_conference_get_chatlist_size(state.tox()) != 0; }, @@ -322,7 +324,8 @@ int main() { }, }, { - 10, "sends a message to the last conference", + 10, + "sends a message to the last conference", [](Local_State const &state) { return tox_conference_get_chatlist_size(state.tox()) != 0; }, @@ -344,7 +347,9 @@ int main() { }, }, { - 10, "changes their name", [](Local_State const &state) { return true; }, + 10, + "changes their name", + [](Local_State const &state) { return true; }, [](Local_State *state, Random *rnd, std::mt19937 *rng) { std::vector name(rnd->name_length_selector(*rng)); for (uint8_t &byte : name) { @@ -359,7 +364,9 @@ int main() { }, }, { - 10, "sets their name to empty", [](Local_State const &state) { return true; }, + 10, + "sets their name to empty", + [](Local_State const &state) { return true; }, [](Local_State *state, Random *rnd, std::mt19937 *rng) { TOX_ERR_SET_INFO err; tox_self_set_name(state->tox(), nullptr, 0, &err); diff --git a/toxcore/BUILD.bazel b/toxcore/BUILD.bazel index 6fbe016e..a34c328c 100644 --- a/toxcore/BUILD.bazel +++ b/toxcore/BUILD.bazel @@ -113,6 +113,15 @@ cc_library( deps = [":network"], ) +cc_test( + name = "ping_array_test", + srcs = ["ping_array_test.cc"], + deps = [ + ":ping_array", + "@com_google_googletest//:gtest_main", + ], +) + cc_library( name = "DHT", srcs = [ diff --git a/toxcore/crypto_core_test.cc b/toxcore/crypto_core_test.cc index cdc315b2..d6888b19 100644 --- a/toxcore/crypto_core_test.cc +++ b/toxcore/crypto_core_test.cc @@ -10,8 +10,10 @@ enum { /** * The size of the arrays to compare. This was chosen to take around 2000 * CPU clocks on x86_64. + * + * This is 1MiB. */ - CRYPTO_TEST_MEMCMP_SIZE = 1024 * 1024, // 1 MiB + CRYPTO_TEST_MEMCMP_SIZE = 1024 * 1024, /** * The number of times we run memcmp in the test. * diff --git a/toxcore/ping_array.api.h b/toxcore/ping_array.api.h index 61021e24..74e2b002 100644 --- a/toxcore/ping_array.api.h +++ b/toxcore/ping_array.api.h @@ -22,10 +22,15 @@ * You should have received a copy of the GNU General Public License * along with Tox. If not, see . */ -#ifndef PING_ARRAY_H -#define PING_ARRAY_H +#ifndef C_TOXCORE_TOXCORE_PING_ARRAY_H +#define C_TOXCORE_TOXCORE_PING_ARRAY_H -#include "network.h" +#include +#include + +#ifdef __cplusplus +extern "C" { +#endif %} class mono_Time { struct this; } @@ -36,11 +41,11 @@ struct this; /** * Initialize a Ping_Array. - * size represents the total size of the array and should be a power of 2. - * timeout represents the maximum timeout in seconds for the entry. * - * return 0 on success. - * return -1 on failure. + * @param size represents the total size of the array and should be a power of 2. + * @param timeout represents the maximum timeout in seconds for the entry. + * + * @return 0 on success, -1 on failure. */ static this new(uint32_t size, uint32_t timeout); @@ -52,8 +57,7 @@ void kill(); /** * Add a data with length to the Ping_Array list and return a ping_id. * - * return ping_id on success. - * return 0 on failure. + * @return ping_id on success, 0 on failure. */ uint64_t add(const mono_Time::this *mono_time, const uint8_t *data, uint32_t length); @@ -62,13 +66,16 @@ uint64_t add(const mono_Time::this *mono_time, const uint8_t *data, uint32_t len * * On success, copies the data into data of length, * - * return length of data copied on success. - * return -1 on failure. + * @return length of data copied on success, -1 on failure. */ int32_t check(const mono_Time::this *mono_time, uint8_t[length] data, uint64_t ping_id); } %{ +#ifdef __cplusplus +} // extern "C" #endif + +#endif // C_TOXCORE_TOXCORE_PING_ARRAY_H %} diff --git a/toxcore/ping_array.c b/toxcore/ping_array.c index 9130254e..6669b4bd 100644 --- a/toxcore/ping_array.c +++ b/toxcore/ping_array.c @@ -34,7 +34,6 @@ #include "mono_time.h" #include "util.h" - typedef struct Ping_Array_Entry { void *data; uint32_t length; @@ -46,25 +45,23 @@ struct Ping_Array { Ping_Array_Entry *entries; uint32_t last_deleted; /* number representing the next entry to be deleted. */ - uint32_t last_added; /* number representing the last entry to be added. */ - uint32_t total_size; /* The length of entries */ - uint32_t timeout; /* The timeout after which entries are cleared. */ + uint32_t last_added; /* number representing the last entry to be added. */ + uint32_t total_size; /* The length of entries */ + uint32_t timeout; /* The timeout after which entries are cleared. */ }; -/* Initialize a Ping_Array. - * size represents the total size of the array and should be a power of 2. - * timeout represents the maximum timeout in seconds for the entry. - * - * return 0 on success. - * return -1 on failure. - */ Ping_Array *ping_array_new(uint32_t size, uint32_t timeout) { if (size == 0 || timeout == 0) { return nullptr; } - Ping_Array *empty_array = (Ping_Array *)calloc(1, sizeof(Ping_Array)); + if ((size & (size - 1)) != 0) { + // Not a power of 2. + return nullptr; + } + + Ping_Array *const empty_array = (Ping_Array *)calloc(1, sizeof(Ping_Array)); if (empty_array == nullptr) { return nullptr; @@ -86,19 +83,15 @@ Ping_Array *ping_array_new(uint32_t size, uint32_t timeout) static void clear_entry(Ping_Array *array, uint32_t index) { + const Ping_Array_Entry empty = {nullptr}; free(array->entries[index].data); - array->entries[index].data = nullptr; - array->entries[index].length = 0; - array->entries[index].time = 0; - array->entries[index].ping_id = 0; + array->entries[index] = empty; } -/* Free all the allocated memory in a Ping_Array. - */ void ping_array_kill(Ping_Array *array) { while (array->last_deleted != array->last_added) { - uint32_t index = array->last_deleted % array->total_size; + const uint32_t index = array->last_deleted % array->total_size; clear_entry(array, index); ++array->last_deleted; } @@ -112,7 +105,7 @@ void ping_array_kill(Ping_Array *array) static void ping_array_clear_timedout(Ping_Array *array, const Mono_Time *mono_time) { while (array->last_deleted != array->last_added) { - uint32_t index = array->last_deleted % array->total_size; + const uint32_t index = array->last_deleted % array->total_size; if (!mono_time_is_timeout(mono_time, array->entries[index].time, array->timeout)) { break; @@ -123,15 +116,11 @@ static void ping_array_clear_timedout(Ping_Array *array, const Mono_Time *mono_t } } -/* Add a data with length to the Ping_Array list and return a ping_id. - * - * return ping_id on success. - * return 0 on failure. - */ -uint64_t ping_array_add(Ping_Array *array, const Mono_Time *mono_time, const uint8_t *data, uint32_t length) +uint64_t ping_array_add(Ping_Array *array, const Mono_Time *mono_time, const uint8_t *data, + uint32_t length) { ping_array_clear_timedout(array, mono_time); - uint32_t index = array->last_added % array->total_size; + const uint32_t index = array->last_added % array->total_size; if (array->entries[index].data != nullptr) { array->last_deleted = array->last_added - array->total_size; @@ -161,21 +150,14 @@ uint64_t ping_array_add(Ping_Array *array, const Mono_Time *mono_time, const uin return ping_id; } - -/* Check if ping_id is valid and not timed out. - * - * On success, copies the data into data of length, - * - * return length of data copied on success. - * return -1 on failure. - */ -int32_t ping_array_check(Ping_Array *array, const Mono_Time *mono_time, uint8_t *data, size_t length, uint64_t ping_id) +int32_t ping_array_check(Ping_Array *array, const Mono_Time *mono_time, uint8_t *data, + size_t length, uint64_t ping_id) { if (ping_id == 0) { return -1; } - uint32_t index = ping_id % array->total_size; + const uint32_t index = ping_id % array->total_size; if (array->entries[index].ping_id != ping_id) { return -1; @@ -189,12 +171,13 @@ int32_t ping_array_check(Ping_Array *array, const Mono_Time *mono_time, uint8_t return -1; } + // TODO(iphydf): This can't happen? If it indeed can't, turn it into an assert. if (array->entries[index].data == nullptr) { return -1; } memcpy(data, array->entries[index].data, array->entries[index].length); - uint32_t len = array->entries[index].length; + const uint32_t len = array->entries[index].length; clear_entry(array, index); return len; } diff --git a/toxcore/ping_array.h b/toxcore/ping_array.h index cfa9bfcc..7bc95a42 100644 --- a/toxcore/ping_array.h +++ b/toxcore/ping_array.h @@ -21,10 +21,15 @@ * You should have received a copy of the GNU General Public License * along with Tox. If not, see . */ -#ifndef PING_ARRAY_H -#define PING_ARRAY_H +#ifndef C_TOXCORE_TOXCORE_PING_ARRAY_H +#define C_TOXCORE_TOXCORE_PING_ARRAY_H -#include "network.h" +#include +#include + +#ifdef __cplusplus +extern "C" { +#endif #ifndef MONO_TIME_DEFINED #define MONO_TIME_DEFINED @@ -38,11 +43,11 @@ typedef struct Ping_Array Ping_Array; /** * Initialize a Ping_Array. - * size represents the total size of the array and should be a power of 2. - * timeout represents the maximum timeout in seconds for the entry. * - * return 0 on success. - * return -1 on failure. + * @param size represents the total size of the array and should be a power of 2. + * @param timeout represents the maximum timeout in seconds for the entry. + * + * @return 0 on success, -1 on failure. */ struct Ping_Array *ping_array_new(uint32_t size, uint32_t timeout); @@ -54,8 +59,7 @@ void ping_array_kill(struct Ping_Array *_array); /** * Add a data with length to the Ping_Array list and return a ping_id. * - * return ping_id on success. - * return 0 on failure. + * @return ping_id on success, 0 on failure. */ uint64_t ping_array_add(struct Ping_Array *_array, const struct Mono_Time *mono_time, const uint8_t *data, uint32_t length); @@ -65,10 +69,13 @@ uint64_t ping_array_add(struct Ping_Array *_array, const struct Mono_Time *mono_ * * On success, copies the data into data of length, * - * return length of data copied on success. - * return -1 on failure. + * @return length of data copied on success, -1 on failure. */ int32_t ping_array_check(struct Ping_Array *_array, const struct Mono_Time *mono_time, uint8_t *data, size_t length, uint64_t ping_id); +#ifdef __cplusplus +} // extern "C" #endif + +#endif // C_TOXCORE_TOXCORE_PING_ARRAY_H diff --git a/toxcore/ping_array_test.cc b/toxcore/ping_array_test.cc new file mode 100644 index 00000000..fca5009a --- /dev/null +++ b/toxcore/ping_array_test.cc @@ -0,0 +1,119 @@ +#include "ping_array.h" + +#include + +#include +#include "mono_time.h" + +namespace { + +struct Ping_Array_Deleter { + void operator()(Ping_Array *arr) { ping_array_kill(arr); } +}; + +using Ping_Array_Ptr = std::unique_ptr; + +struct Mono_Time_Deleter { + void operator()(Mono_Time *arr) { mono_time_free(arr); } +}; + +using Mono_Time_Ptr = std::unique_ptr; + +TEST(PingArray, MinimumTimeoutIsOne) { + EXPECT_EQ(ping_array_new(1, 0), nullptr); + EXPECT_NE(Ping_Array_Ptr(ping_array_new(1, 1)), nullptr); +} + +TEST(PingArray, MinimumArraySizeIsOne) { + EXPECT_EQ(ping_array_new(0, 1), nullptr); + EXPECT_NE(Ping_Array_Ptr(ping_array_new(1, 1)), nullptr); +} + +TEST(PingArray, ArraySizeMustBePowerOfTwo) { + Ping_Array_Ptr arr; + arr.reset(ping_array_new(2, 1)); + EXPECT_NE(arr, nullptr); + arr.reset(ping_array_new(4, 1)); + EXPECT_NE(arr, nullptr); + arr.reset(ping_array_new(1024, 1)); + EXPECT_NE(arr, nullptr); + + EXPECT_EQ(ping_array_new(1023, 1), nullptr); + EXPECT_EQ(ping_array_new(1234, 1), nullptr); +} + +TEST(PingArray, StoredDataCanBeRetrieved) { + Ping_Array_Ptr const arr(ping_array_new(2, 1)); + Mono_Time_Ptr const mono_time(mono_time_new()); + + uint64_t const ping_id = + ping_array_add(arr.get(), mono_time.get(), std::vector{1, 2, 3, 4}.data(), 4); + EXPECT_NE(ping_id, 0); + + std::vector data(4); + EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), data.data(), data.size(), ping_id), 4); + EXPECT_EQ(data, std::vector({1, 2, 3, 4})); +} + +TEST(PingArray, RetrievingDataWithTooSmallOutputBufferHasNoEffect) { + Ping_Array_Ptr const arr(ping_array_new(2, 1)); + Mono_Time_Ptr const mono_time(mono_time_new()); + + uint64_t const ping_id = + ping_array_add(arr.get(), mono_time.get(), (std::vector{1, 2, 3, 4}).data(), 4); + EXPECT_NE(ping_id, 0); + + std::vector data(4); + EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), data.data(), 3, ping_id), -1); + // It doesn't write anything to the data array. + EXPECT_EQ(data, std::vector({0, 0, 0, 0})); + // Afterwards, we can still read it. + EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), data.data(), 4, ping_id), 4); + EXPECT_EQ(data, std::vector({1, 2, 3, 4})); +} + +TEST(PingArray, ZeroLengthDataCanBeAdded) { + Ping_Array_Ptr const arr(ping_array_new(2, 1)); + Mono_Time_Ptr const mono_time(mono_time_new()); + + uint64_t const ping_id = ping_array_add(arr.get(), mono_time.get(), nullptr, 0); + EXPECT_NE(ping_id, 0); + + EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, ping_id), 0); +} + +TEST(PingArray, PingId0IsInvalid) { + Ping_Array_Ptr const arr(ping_array_new(2, 1)); + Mono_Time_Ptr const mono_time(mono_time_new()); + + EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, 0), -1); +} + +// Protection against replay attacks. +TEST(PingArray, DataCanOnlyBeRetrievedOnce) { + Ping_Array_Ptr const arr(ping_array_new(2, 1)); + Mono_Time_Ptr const mono_time(mono_time_new()); + + uint64_t const ping_id = ping_array_add(arr.get(), mono_time.get(), nullptr, 0); + EXPECT_NE(ping_id, 0); + + EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, ping_id), 0); + EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, ping_id), -1); +} + +TEST(PingArray, PingIdMustMatchOnCheck) { + Ping_Array_Ptr const arr(ping_array_new(1, 1)); + Mono_Time_Ptr const mono_time(mono_time_new()); + + uint64_t const ping_id = ping_array_add(arr.get(), mono_time.get(), nullptr, 0); + EXPECT_NE(ping_id, 0); + + uint64_t const bad_ping_id = ping_id == 1 ? 2 : 1; + + // bad_ping_id will also be pointing at the same element, but won't match the + // actual ping_id. + EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, bad_ping_id), -1); + EXPECT_EQ(ping_array_check(arr.get(), mono_time.get(), nullptr, 0, ping_id), 0); +} + +} // namespace