From 6542ad442969cf5204ab1e5c10dca147ee755f7e Mon Sep 17 00:00:00 2001 From: iphydf Date: Wed, 9 Feb 2022 20:27:10 +0000 Subject: [PATCH] cleanup: Don't use VLAs for huge allocations. One of these was creating a single 262144 byte stack frame. We now have a way to check and limit the allocation size of a VLA. The `Cmp_Data` ones were also fairly large. Now, no allocation is larger than 2KiB (though rtp.c allocates close to that much). --- auto_tests/encryptsave_test.c | 29 ++++++++++++++----- auto_tests/save_friend_test.c | 3 +- auto_tests/tox_one_test.c | 3 +- .../docker/tox-bootstrapd.sha256 | 2 +- toxcore/DHT.c | 28 +++++++++++++++--- toxcore/ccompat.h | 13 +++++++-- toxcore/onion_announce.c | 8 ++++- toxcore/onion_client.c | 8 ++++- 8 files changed, 76 insertions(+), 18 deletions(-) diff --git a/auto_tests/encryptsave_test.c b/auto_tests/encryptsave_test.c index 54291ac7dba..30812875cc6 100644 --- a/auto_tests/encryptsave_test.c +++ b/auto_tests/encryptsave_test.c @@ -59,10 +59,12 @@ static void test_save_friend(void) ck_assert_msg(test != UINT32_MAX, "Failed to add friend"); size_t size = tox_get_savedata_size(tox1); - VLA(uint8_t, data, size); + uint8_t *data = (uint8_t *)malloc(size); + ck_assert(data != nullptr); tox_get_savedata(tox1, data); size_t size2 = size + TOX_PASS_ENCRYPTION_EXTRA_LENGTH; - VLA(uint8_t, enc_data, size2); + uint8_t *enc_data = (uint8_t *)malloc(size2); + ck_assert(enc_data != nullptr); Tox_Err_Encryption error1; bool ret = tox_pass_encrypt(data, size, (const uint8_t *)"correcthorsebatterystaple", 25, enc_data, &error1); ck_assert_msg(ret, "failed to encrypted save: %d", error1); @@ -78,7 +80,8 @@ static void test_save_friend(void) ck_assert_msg(err2 == TOX_ERR_NEW_LOAD_ENCRYPTED, "wrong error! %d. should fail with %d", err2, TOX_ERR_NEW_LOAD_ENCRYPTED); ck_assert_msg(tox3 == nullptr, "tox_new with error should return NULL"); - VLA(uint8_t, dec_data, size); + uint8_t *dec_data = (uint8_t *)malloc(size); + ck_assert(dec_data != nullptr); Tox_Err_Decryption err3; ret = tox_pass_decrypt(enc_data, size2, (const uint8_t *)"correcthorsebatterystaple", 25, dec_data, &err3); ck_assert_msg(ret, "failed to decrypt save: %d", err3); @@ -91,7 +94,8 @@ static void test_save_friend(void) ck_assert_msg(memcmp(address, address2, TOX_PUBLIC_KEY_SIZE) == 0, "addresses don't match!"); size = tox_get_savedata_size(tox3); - VLA(uint8_t, data2, size); + uint8_t *data2 = (uint8_t *)malloc(size); + ck_assert(data2 != nullptr); tox_get_savedata(tox3, data2); Tox_Err_Key_Derivation keyerr; Tox_Pass_Key *key = tox_pass_key_derive((const uint8_t *)"123qweasdzxc", 12, &keyerr); @@ -99,13 +103,16 @@ static void test_save_friend(void) memcpy((uint8_t *)key, test_salt, TOX_PASS_SALT_LENGTH); memcpy((uint8_t *)key + TOX_PASS_SALT_LENGTH, known_key2, TOX_PASS_KEY_LENGTH); size2 = size + TOX_PASS_ENCRYPTION_EXTRA_LENGTH; - VLA(uint8_t, encdata2, size2); + uint8_t *encdata2 = (uint8_t *)malloc(size2); + ck_assert(encdata2 != nullptr); ret = tox_pass_key_encrypt(key, data2, size, encdata2, &error1); ck_assert_msg(ret, "failed to key encrypt %d", error1); ck_assert_msg(tox_is_data_encrypted(encdata2), "magic number the second missing"); - VLA(uint8_t, out1, size); - VLA(uint8_t, out2, size); + uint8_t *out1 = (uint8_t *)malloc(size); + ck_assert(out1 != nullptr); + uint8_t *out2 = (uint8_t *)malloc(size); + ck_assert(out2 != nullptr); ret = tox_pass_decrypt(encdata2, size2, (const uint8_t *)pw, pwlen, out1, &err3); ck_assert_msg(ret, "failed to pw decrypt %d", err3); ret = tox_pass_key_decrypt(key, encdata2, size2, out2, &err3); @@ -129,6 +136,14 @@ static void test_save_friend(void) tox_kill(tox2); tox_kill(tox3); tox_kill(tox4); + + free(out2); + free(out1); + free(encdata2); + free(data2); + free(dec_data); + free(enc_data); + free(data); } static void test_keys(void) diff --git a/auto_tests/save_friend_test.c b/auto_tests/save_friend_test.c index 110ad2bf15e..6f50beda0b1 100644 --- a/auto_tests/save_friend_test.c +++ b/auto_tests/save_friend_test.c @@ -108,7 +108,7 @@ int main(void) } size_t save_size = tox_get_savedata_size(tox1); - VLA(uint8_t, savedata, save_size); + uint8_t *savedata = (uint8_t *)malloc(save_size); tox_get_savedata(tox1, savedata); struct Tox_Options *const options = tox_options_new(nullptr); @@ -129,6 +129,7 @@ int main(void) tox_kill(tox1); tox_kill(tox2); tox_kill(tox_to_compare); + free(savedata); return 0; } diff --git a/auto_tests/tox_one_test.c b/auto_tests/tox_one_test.c index ebfe46fe112..a44f5a5fd45 100644 --- a/auto_tests/tox_one_test.c +++ b/auto_tests/tox_one_test.c @@ -76,7 +76,7 @@ static void test_one(void) tox_self_get_address(tox1, address); size_t save_size = tox_get_savedata_size(tox1); - VLA(uint8_t, data, save_size); + uint8_t *data = (uint8_t *)malloc(save_size); tox_get_savedata(tox1, data); tox_kill(tox2); @@ -126,6 +126,7 @@ static void test_one(void) tox_options_free(options); tox_kill(tox1); tox_kill(tox2); + free(data); } diff --git a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 index 00089e35435..ac8aafbd479 100644 --- a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 +++ b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 @@ -1 +1 @@ -502cc22df74fa369b2c09d117176705a1e801726db6f8360c688aee90973fa22 /usr/local/bin/tox-bootstrapd +ffc13de36541eb75974c274f21c8474f396476122e63d3ce7f8cdf2c44bea095 /usr/local/bin/tox-bootstrapd diff --git a/toxcore/DHT.c b/toxcore/DHT.c index 1f4dba52288..6adfe2fb63f 100644 --- a/toxcore/DHT.c +++ b/toxcore/DHT.c @@ -475,14 +475,19 @@ static int dht_create_packet(const uint8_t public_key[CRYPTO_PUBLIC_KEY_SIZE], const uint8_t *shared_key, const uint8_t type, const uint8_t *plain, size_t plain_length, uint8_t *packet) { - VLA(uint8_t, encrypted, plain_length + CRYPTO_MAC_SIZE); + uint8_t *encrypted = (uint8_t *)malloc(plain_length + CRYPTO_MAC_SIZE); uint8_t nonce[CRYPTO_NONCE_SIZE]; + if (encrypted == nullptr) { + return -1; + } + random_nonce(nonce); const int encrypted_length = encrypt_data_symmetric(shared_key, nonce, plain, plain_length, encrypted); if (encrypted_length == -1) { + free(encrypted); return -1; } @@ -491,6 +496,7 @@ static int dht_create_packet(const uint8_t public_key[CRYPTO_PUBLIC_KEY_SIZE], memcpy(packet + 1 + CRYPTO_PUBLIC_KEY_SIZE, nonce, CRYPTO_NONCE_SIZE); memcpy(packet + 1 + CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_NONCE_SIZE, encrypted, encrypted_length); + free(encrypted); return 1 + CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_NONCE_SIZE + encrypted_length; } @@ -968,7 +974,11 @@ static void sort_client_list(Client_data *list, uint64_t cur_time, unsigned int { // Pass comp_public_key to qsort with each Client_data entry, so the // comparison function can use it as the base of comparison. - VLA(DHT_Cmp_Data, cmp_list, length); + DHT_Cmp_Data *cmp_list = (DHT_Cmp_Data *)calloc(length, sizeof(DHT_Cmp_Data)); + + if (cmp_list == nullptr) { + return; + } for (uint32_t i = 0; i < length; ++i) { cmp_list[i].cur_time = cur_time; @@ -981,6 +991,8 @@ static void sort_client_list(Client_data *list, uint64_t cur_time, unsigned int for (uint32_t i = 0; i < length; ++i) { list[i] = cmp_list[i].entry; } + + free(cmp_list); } non_null() @@ -1699,11 +1711,17 @@ static uint8_t do_ping_and_sendnode_requests(DHT *dht, uint64_t *lastgetnode, co const uint64_t temp_time = mono_time_get(dht->mono_time); uint32_t num_nodes = 0; - VLA(Client_data *, client_list, list_count * 2); - VLA(IPPTsPng *, assoc_list, list_count * 2); + Client_data **client_list = (Client_data **)calloc(list_count * 2, sizeof(Client_data *)); + IPPTsPng **assoc_list = (IPPTsPng **)calloc(list_count * 2, sizeof(IPPTsPng *)); unsigned int sort = 0; bool sort_ok = false; + if (client_list == nullptr || assoc_list == nullptr) { + free(assoc_list); + free(client_list); + return 0; + } + for (uint32_t i = 0; i < list_count; ++i) { /* If node is not dead. */ Client_data *client = &list[i]; @@ -1757,6 +1775,8 @@ static uint8_t do_ping_and_sendnode_requests(DHT *dht, uint64_t *lastgetnode, co ++*bootstrap_times; } + free(assoc_list); + free(client_list); return not_kill; } diff --git a/toxcore/ccompat.h b/toxcore/ccompat.h index 8aadc6c7469..8dee14a3f6e 100644 --- a/toxcore/ccompat.h +++ b/toxcore/ccompat.h @@ -27,7 +27,7 @@ // you may run out of stack space. #if !defined(DISABLE_VLA) && !defined(_MSC_VER) && defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L // C99 VLAs. -#define VLA(type, name, size) type name[size] +#define ALLOC_VLA(type, name, size) type name[size] #define SIZEOF_VLA sizeof #else @@ -48,13 +48,22 @@ #endif #endif -#define VLA(type, name, size) \ +#define ALLOC_VLA(type, name, size) \ const size_t name##_vla_size = (size) * sizeof(type); \ type *const name = (type *)alloca(name##_vla_size) #define SIZEOF_VLA(name) name##_vla_size #endif +#ifdef MAX_VLA_SIZE +#include +#define VLA(type, name, size) \ + ALLOC_VLA(type, name, size); \ + assert((size_t)(size) * sizeof(type) <= MAX_VLA_SIZE) +#else +#define VLA ALLOC_VLA +#endif + #if !defined(__cplusplus) || __cplusplus < 201103L #define nullptr NULL #ifndef static_assert diff --git a/toxcore/onion_announce.c b/toxcore/onion_announce.c index 90ebd99ce1c..0f519d785ba 100644 --- a/toxcore/onion_announce.c +++ b/toxcore/onion_announce.c @@ -310,7 +310,11 @@ static void sort_onion_announce_list(Onion_Announce_Entry *list, unsigned int le { // Pass comp_public_key to qsort with each Client_data entry, so the // comparison function can use it as the base of comparison. - VLA(Cmp_Data, cmp_list, length); + Cmp_Data *cmp_list = (Cmp_Data *)calloc(length, sizeof(Cmp_Data)); + + if (cmp_list == nullptr) { + return; + } for (uint32_t i = 0; i < length; ++i) { cmp_list[i].mono_time = mono_time; @@ -323,6 +327,8 @@ static void sort_onion_announce_list(Onion_Announce_Entry *list, unsigned int le for (uint32_t i = 0; i < length; ++i) { list[i] = cmp_list[i].entry; } + + free(cmp_list); } /** add entry to entries list diff --git a/toxcore/onion_client.c b/toxcore/onion_client.c index ea0bfcee11b..c220fac9286 100644 --- a/toxcore/onion_client.c +++ b/toxcore/onion_client.c @@ -660,7 +660,11 @@ static void sort_onion_node_list(Onion_Node *list, unsigned int length, const Mo { // Pass comp_public_key to qsort with each Client_data entry, so the // comparison function can use it as the base of comparison. - VLA(Onion_Client_Cmp_Data, cmp_list, length); + Onion_Client_Cmp_Data *cmp_list = (Onion_Client_Cmp_Data *)calloc(length, sizeof(Onion_Client_Cmp_Data)); + + if (cmp_list == nullptr) { + return; + } for (uint32_t i = 0; i < length; ++i) { cmp_list[i].mono_time = mono_time; @@ -673,6 +677,8 @@ static void sort_onion_node_list(Onion_Node *list, unsigned int length, const Mo for (uint32_t i = 0; i < length; ++i) { list[i] = cmp_list[i].entry; } + + free(cmp_list); } non_null()