Skip to content

Commit

Permalink
Fix errors on error paths found by oomer.
Browse files Browse the repository at this point in the history
* Use-after-free because we free network before dht in one case.
* Various unchecked allocs in tests (not so important).
* We used to not check whether ping arrays were actually allocated in DHT.
* `ping_kill` and `ping_array_kill` used to crash when passing NULL.

Also:
* Added an assert in all public API functions to ensure tox isn't NULL.
  The error message you get from that is a bit nicer than "Segmentation
  fault" when clients (or our tests) do things wrong.
* Decreased the sleep time in iterate_all_wait from 20ms to 5ms.
  Everything seems to still work with 5ms, and this greatly decreases
  the amount of time spent per test run, making oomer run much faster.
  • Loading branch information
iphydf committed May 2, 2020
1 parent e057bae commit 5e70ee3
Show file tree
Hide file tree
Showing 8 changed files with 131 additions and 9 deletions.
17 changes: 13 additions & 4 deletions auto_tests/run_auto_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ static void iterate_all_wait(uint32_t tox_count, Tox **toxes, State *state, uint
}

/* Also actually sleep a little, to allow for local network processing */
c_sleep(20);
c_sleep(5);
}

static uint64_t get_state_clock_callback(Mono_Time *mono_time, void *user_data)
Expand All @@ -63,6 +63,9 @@ static void run_auto_test(uint32_t tox_count, void test(Tox **toxes, State *stat
Tox **toxes = (Tox **)calloc(tox_count, sizeof(Tox *));
State *state = (State *)calloc(tox_count, sizeof(State));

ck_assert(toxes != nullptr);
ck_assert(state != nullptr);

for (uint32_t i = 0; i < tox_count; i++) {
state[i].index = i;
toxes[i] = tox_new_log(nullptr, nullptr, &state[i].index);
Expand All @@ -82,7 +85,9 @@ static void run_auto_test(uint32_t tox_count, void test(Tox **toxes, State *stat

uint8_t public_key[TOX_PUBLIC_KEY_SIZE];
tox_self_get_public_key(toxes[j], public_key);
tox_friend_add_norequest(toxes[i], public_key, nullptr);
Tox_Err_Friend_Add err;
tox_friend_add_norequest(toxes[i], public_key, &err);
ck_assert(err == TOX_ERR_FRIEND_ADD_OK);
}
}
} else {
Expand All @@ -93,7 +98,9 @@ static void run_auto_test(uint32_t tox_count, void test(Tox **toxes, State *stat
if (i != j) {
uint8_t public_key[TOX_PUBLIC_KEY_SIZE];
tox_self_get_public_key(toxes[j], public_key);
tox_friend_add_norequest(toxes[i], public_key, nullptr);
Tox_Err_Friend_Add err;
tox_friend_add_norequest(toxes[i], public_key, &err);
ck_assert(err == TOX_ERR_FRIEND_ADD_OK);
}
}
}
Expand All @@ -105,7 +112,9 @@ static void run_auto_test(uint32_t tox_count, void test(Tox **toxes, State *stat
const uint16_t dht_port = tox_self_get_udp_port(toxes[0], nullptr);

for (uint32_t i = 1; i < tox_count; i++) {
tox_bootstrap(toxes[i], "localhost", dht_port, dht_key, nullptr);
Tox_Err_Bootstrap err;
tox_bootstrap(toxes[i], "localhost", dht_port, dht_key, &err);
ck_assert(err == TOX_ERR_BOOTSTRAP_OK);
}

do {
Expand Down
5 changes: 4 additions & 1 deletion auto_tests/tox_one_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,10 @@ static void test_one(void)

uint32_t index[] = { 1, 2 };
Tox *tox1 = tox_new_log(nullptr, nullptr, &index[0]);
ck_assert(tox1 != nullptr);
set_random_name_and_status_message(tox1, name, status_message);
Tox *tox2 = tox_new_log(nullptr, nullptr, &index[1]);
ck_assert(tox2 != nullptr);
set_random_name_and_status_message(tox2, name2, status_message2);

uint8_t address[TOX_ADDRESS_SIZE];
Expand All @@ -49,7 +51,7 @@ static void test_one(void)
ck_assert_msg(ret == UINT32_MAX && error == TOX_ERR_FRIEND_ADD_OWN_KEY, "Adding own address worked.");

tox_self_get_address(tox2, address);
uint8_t message[TOX_MAX_FRIEND_REQUEST_LENGTH + 1];
uint8_t message[TOX_MAX_FRIEND_REQUEST_LENGTH + 1] = {0};
ret = tox_friend_add(tox1, address, nullptr, 0, &error);
ck_assert_msg(ret == UINT32_MAX && error == TOX_ERR_FRIEND_ADD_NULL, "Sending request with no message worked.");
ret = tox_friend_add(tox1, address, message, 0, &error);
Expand Down Expand Up @@ -85,6 +87,7 @@ static void test_one(void)
Tox_Err_New err_n;

struct Tox_Options *options = tox_options_new(nullptr);
ck_assert(options != nullptr);
tox_options_set_savedata_type(options, TOX_SAVEDATA_TYPE_TOX_SAVE);
tox_options_set_savedata_data(options, data, save_size);
tox2 = tox_new_log(options, &err_n, &index[1]);
Expand Down
5 changes: 5 additions & 0 deletions toxcore/DHT.c
Original file line number Diff line number Diff line change
Expand Up @@ -2721,6 +2721,11 @@ DHT *new_dht(const Logger *log, Mono_Time *mono_time, Networking_Core *net, bool
dht->dht_ping_array = ping_array_new(DHT_PING_ARRAY_SIZE, PING_TIMEOUT);
dht->dht_harden_ping_array = ping_array_new(DHT_PING_ARRAY_SIZE, PING_TIMEOUT);

if (dht->dht_ping_array == nullptr || dht->dht_harden_ping_array == nullptr) {
kill_dht(dht);
return nullptr;
}

for (uint32_t i = 0; i < DHT_FAKE_FRIEND_NUMBER; ++i) {
uint8_t random_public_key_bytes[CRYPTO_PUBLIC_KEY_SIZE];
uint8_t random_secret_key_bytes[CRYPTO_SECRET_KEY_SIZE];
Expand Down
4 changes: 2 additions & 2 deletions toxcore/Messenger.c
Original file line number Diff line number Diff line change
Expand Up @@ -1999,8 +1999,8 @@ Messenger *new_messenger(Mono_Time *mono_time, Messenger_Options *options, unsig
m->net_crypto = new_net_crypto(m->log, m->mono_time, m->dht, &options->proxy_info);

if (m->net_crypto == nullptr) {
kill_networking(m->net);
kill_dht(m->dht);
kill_networking(m->net);
friendreq_kill(m->fr);
logger_kill(m->log);
free(m);
Expand All @@ -2012,7 +2012,7 @@ Messenger *new_messenger(Mono_Time *mono_time, Messenger_Options *options, unsig
m->onion_c = new_onion_client(m->mono_time, m->net_crypto);
m->fr_c = new_friend_connections(m->mono_time, m->onion_c, options->local_discovery_enabled);

if (!(m->onion && m->onion_a && m->onion_c)) {
if (!(m->onion && m->onion_a && m->onion_c && m->fr_c)) {
kill_friend_connections(m->fr_c);
kill_onion(m->onion);
kill_onion_announce(m->onion_a);
Expand Down
2 changes: 0 additions & 2 deletions toxcore/crypto_core_mem.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@
#if defined(_WIN32) || defined(__WIN32__) || defined(WIN32)
#include <windows.h>
#include <wincrypt.h>
#elif defined(HAVE_EXPLICIT_BZERO)
#include <string.h>
#endif
#endif

Expand Down
4 changes: 4 additions & 0 deletions toxcore/ping.c
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,10 @@ Ping *ping_new(const Mono_Time *mono_time, DHT *dht)

void ping_kill(Ping *ping)
{
if (ping == nullptr) {
return;
}

networking_registerhandler(dht_get_net(ping->dht), NET_PACKET_PING_REQUEST, nullptr, nullptr);
networking_registerhandler(dht_get_net(ping->dht), NET_PACKET_PING_RESPONSE, nullptr, nullptr);
ping_array_kill(ping->ping_array);
Expand Down
4 changes: 4 additions & 0 deletions toxcore/ping_array.c
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ static void clear_entry(Ping_Array *array, uint32_t index)

void ping_array_kill(Ping_Array *array)
{
if (array == nullptr) {
return;
}

while (array->last_deleted != array->last_added) {
const uint32_t index = array->last_deleted % array->total_size;
clear_entry(array, index);
Expand Down
Loading

0 comments on commit 5e70ee3

Please sign in to comment.