Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup: Don't use VLAs for huge allocations. #2013

Merged
merged 1 commit into from
Feb 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions auto_tests/encryptsave_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -91,21 +94,25 @@ 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);
ck_assert_msg(key != nullptr, "pass key allocation failure");
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);
Expand All @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion auto_tests/save_friend_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -129,6 +129,7 @@ int main(void)
tox_kill(tox1);
tox_kill(tox2);
tox_kill(tox_to_compare);
free(savedata);

return 0;
}
3 changes: 2 additions & 1 deletion auto_tests/tox_one_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -126,6 +126,7 @@ static void test_one(void)
tox_options_free(options);
tox_kill(tox1);
tox_kill(tox2);
free(data);
}


Expand Down
2 changes: 1 addition & 1 deletion other/analysis/check_includes
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ for line in out.split("\n"):
# ccompat.h can include some things we don't really want elsewhere.
allowlist = ALLOWLIST
if filename == "toxcore/ccompat.h":
allowlist += ("alloca.h", "malloc.h", "stdlib.h")
allowlist += ("assert.h", "alloca.h", "malloc.h", "stdlib.h")
header = include[include.index("<") + 1:include.index(">")]
if header not in allowlist:
source = filename[:-2] + ".c"
Expand Down
2 changes: 1 addition & 1 deletion other/bootstrap_daemon/docker/tox-bootstrapd.sha256
Original file line number Diff line number Diff line change
@@ -1 +1 @@
7d31dd00dd4d8fefd21d375f2c9b69499f356867b364a386c562c15d6ac58d93 /usr/local/bin/tox-bootstrapd
fcf07ffe61e0db89f5e687c54457ae7c59a12f3200232a655e876105df2c0e3c /usr/local/bin/tox-bootstrapd
28 changes: 24 additions & 4 deletions toxcore/DHT.c
Original file line number Diff line number Diff line change
Expand Up @@ -477,14 +477,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;
}

Expand All @@ -493,6 +498,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;
}

Expand Down Expand Up @@ -970,7 +976,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;
Expand All @@ -983,6 +993,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()
Expand Down Expand Up @@ -1705,11 +1717,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];
Expand Down Expand Up @@ -1763,6 +1781,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;
}

Expand Down
13 changes: 11 additions & 2 deletions toxcore/ccompat.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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 <assert.h>
#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
Expand Down
8 changes: 7 additions & 1 deletion toxcore/onion_announce.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down
8 changes: 7 additions & 1 deletion toxcore/onion_client.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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()
Expand Down