Skip to content

Commit e02272b

Browse files
ericwolzewertons
ericwolz
and
ewertons
authored
added AMQP malloc checks (#453)
* mollc size checks * merge with main * Update src/amqpvalue.c Co-authored-by: Ewerton Scaboro da Silva <ewertons@microsoft.com> * Update src/header_detect_io.c Co-authored-by: Ewerton Scaboro da Silva <ewertons@microsoft.com> * Update src/session.c Co-authored-by: Ewerton Scaboro da Silva <ewertons@microsoft.com> * PR update * update cutil --------- Co-authored-by: Ewerton Scaboro da Silva <ewertons@microsoft.com>
1 parent 35b5780 commit e02272b

9 files changed

Lines changed: 115 additions & 53 deletions

File tree

‎src/amqp_frame_codec.c‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "azure_macro_utils/macro_utils.h"
99
#include "azure_c_shared_utility/gballoc.h"
1010
#include "azure_c_shared_utility/xlogging.h"
11+
#include "azure_c_shared_utility/safe_math.h"
1112
#include "azure_uamqp_c/amqp_frame_codec.h"
1213
#include "azure_uamqp_c/frame_codec.h"
1314
#include "azure_uamqp_c/amqpvalue.h"
@@ -274,10 +275,13 @@ int amqp_frame_codec_encode_frame(AMQP_FRAME_CODEC_HANDLE amqp_frame_codec, uint
274275
}
275276
else
276277
{
277-
PAYLOAD* new_payloads = (PAYLOAD*)calloc(1, (sizeof(PAYLOAD) * (payload_count + 1)));
278-
if (new_payloads == NULL)
278+
PAYLOAD* new_payloads;
279+
size_t calloc_size = safe_add_size_t(payload_count, 1);
280+
calloc_size = safe_multiply_size_t(calloc_size, sizeof(PAYLOAD));
281+
if (calloc_size == SIZE_MAX ||
282+
(new_payloads = (PAYLOAD*)calloc(1, calloc_size)) == NULL)
279283
{
280-
LogError("Could not allocate frame payloads");
284+
LogError("Could not allocate frame payloads, size:%zu", calloc_size);
281285
result = MU_FAILURE;
282286
}
283287
else

‎src/amqpvalue.c‎

Lines changed: 34 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1278,11 +1278,12 @@ int amqpvalue_set_list_item_count(AMQP_VALUE value, uint32_t list_size)
12781278
AMQP_VALUE* new_list;
12791279

12801280
/* Codes_SRS_AMQPVALUE_01_152: [amqpvalue_set_list_item_count shall resize an AMQP list.] */
1281-
new_list = (AMQP_VALUE*)realloc(value_data->value.list_value.items, list_size * sizeof(AMQP_VALUE));
1282-
if (new_list == NULL)
1281+
size_t realloc_size = safe_multiply_size_t(list_size, sizeof(AMQP_VALUE));
1282+
if (realloc_size == SIZE_MAX ||
1283+
(new_list = (AMQP_VALUE*)realloc(value_data->value.list_value.items, realloc_size)) == NULL)
12831284
{
12841285
/* Codes_SRS_AMQPVALUE_01_154: [If allocating memory for the list according to the new size fails, then amqpvalue_set_list_item_count shall return a non-zero value, while preserving the existing list contents.] */
1285-
LogError("Could not reallocate list memory");
1286+
LogError("Could not reallocate list memory, size:%zu", realloc_size);
12861287
result = MU_FAILURE;
12871288
}
12881289
else
@@ -1415,11 +1416,14 @@ int amqpvalue_set_list_item(AMQP_VALUE value, uint32_t index, AMQP_VALUE list_it
14151416
{
14161417
if (index >= value_data->value.list_value.count)
14171418
{
1418-
AMQP_VALUE* new_list = (AMQP_VALUE*)realloc(value_data->value.list_value.items, ((size_t)index + 1) * sizeof(AMQP_VALUE));
1419-
if (new_list == NULL)
1419+
AMQP_VALUE* new_list;
1420+
size_t realloc_size = safe_add_size_t((size_t)index, 1);
1421+
realloc_size = safe_multiply_size_t(realloc_size, sizeof(AMQP_VALUE));
1422+
if (realloc_size == SIZE_MAX ||
1423+
(new_list = (AMQP_VALUE*)realloc(value_data->value.list_value.items, realloc_size)) == NULL)
14201424
{
14211425
/* Codes_SRS_AMQPVALUE_01_170: [When amqpvalue_set_list_item fails due to not being able to clone the item or grow the list, the list shall not be altered.] */
1422-
LogError("Could not reallocate list storage");
1426+
LogError("Could not reallocate list storage, size:%zu", realloc_size);
14231427
amqpvalue_destroy(cloned_item);
14241428
result = MU_FAILURE;
14251429
}
@@ -1613,13 +1617,16 @@ int amqpvalue_set_map_value(AMQP_VALUE map, AMQP_VALUE key, AMQP_VALUE value)
16131617
}
16141618
else
16151619
{
1616-
AMQP_MAP_KEY_VALUE_PAIR* new_pairs = (AMQP_MAP_KEY_VALUE_PAIR*)realloc(value_data->value.map_value.pairs, ((size_t)value_data->value.map_value.pair_count + 1) * sizeof(AMQP_MAP_KEY_VALUE_PAIR));
1617-
if (new_pairs == NULL)
1620+
AMQP_MAP_KEY_VALUE_PAIR* new_pairs;
1621+
size_t realloc_size = safe_add_size_t((size_t)value_data->value.map_value.pair_count, 1);
1622+
realloc_size = safe_multiply_size_t(realloc_size, sizeof(AMQP_MAP_KEY_VALUE_PAIR));
1623+
if (realloc_size == SIZE_MAX ||
1624+
(new_pairs = (AMQP_MAP_KEY_VALUE_PAIR*)realloc(value_data->value.map_value.pairs, realloc_size)) == NULL)
16181625
{
16191626
/* Codes_SRS_AMQPVALUE_01_186: [If allocating memory to hold a new key/value pair fails, amqpvalue_set_map_value shall fail and return a non-zero value.] */
16201627
amqpvalue_destroy(cloned_key);
16211628
amqpvalue_destroy(cloned_value);
1622-
LogError("Could not reallocate memory for map");
1629+
LogError("Could not reallocate memory for new_pairs map, size:%zu", realloc_size);
16231630
result = MU_FAILURE;
16241631
}
16251632
else
@@ -1918,13 +1925,16 @@ int amqpvalue_add_array_item(AMQP_VALUE value, AMQP_VALUE array_item_value)
19181925
}
19191926
else
19201927
{
1921-
AMQP_VALUE* new_array = (AMQP_VALUE*)realloc(value_data->value.array_value.items, ((size_t)value_data->value.array_value.count + 1) * sizeof(AMQP_VALUE));
1922-
if (new_array == NULL)
1928+
AMQP_VALUE* new_array;
1929+
size_t realloc_size = safe_add_size_t((size_t)value_data->value.array_value.count, 1);
1930+
realloc_size = safe_multiply_size_t(realloc_size, sizeof(AMQP_VALUE));
1931+
if (realloc_size == SIZE_MAX ||
1932+
(new_array = (AMQP_VALUE*)realloc(value_data->value.array_value.items, realloc_size)) == NULL)
19231933
{
19241934
/* Codes_SRS_AMQPVALUE_01_423: [ When `amqpvalue_add_array_item` fails due to not being able to clone the item or grow the array, the array shall not be altered. ] */
19251935
/* Codes_SRS_AMQPVALUE_01_424: [ If growing the array fails, then `amqpvalue_add_array_item` shall fail and return a non-zero value. ] */
19261936
amqpvalue_destroy(cloned_item);
1927-
LogError("Cannot resize array");
1937+
LogError("Cannot resize array, size:%zu", realloc_size);
19281938
result = MU_FAILURE;
19291939
}
19301940
else
@@ -6360,10 +6370,11 @@ static int internal_decoder_decode_bytes(INTERNAL_DECODER_DATA* internal_decoder
63606370
else
63616371
{
63626372
uint32_t i;
6363-
internal_decoder_data->decode_to_value->value.list_value.items = (AMQP_VALUE*)calloc(1, (sizeof(AMQP_VALUE) * internal_decoder_data->decode_to_value->value.list_value.count));
6364-
if (internal_decoder_data->decode_to_value->value.list_value.items == NULL)
6373+
size_t calloc_size = safe_multiply_size_t(sizeof(AMQP_VALUE), internal_decoder_data->decode_to_value->value.list_value.count);
6374+
if (calloc_size == SIZE_MAX ||
6375+
(internal_decoder_data->decode_to_value->value.list_value.items = (AMQP_VALUE*)calloc(1, calloc_size)) == NULL)
63656376
{
6366-
LogError("Could not allocate memory for decoded list value");
6377+
LogError("Could not allocate memory for decoded list value, size:%zu", calloc_size);
63676378
result = MU_FAILURE;
63686379
}
63696380
else
@@ -6826,10 +6837,11 @@ static int internal_decoder_decode_bytes(INTERNAL_DECODER_DATA* internal_decoder
68266837
else
68276838
{
68286839
uint32_t i;
6829-
internal_decoder_data->decode_to_value->value.array_value.items = (AMQP_VALUE*)calloc(1, (sizeof(AMQP_VALUE) * internal_decoder_data->decode_to_value->value.array_value.count));
6830-
if (internal_decoder_data->decode_to_value->value.array_value.items == NULL)
6840+
size_t calloc_size = safe_multiply_size_t(sizeof(AMQP_VALUE), internal_decoder_data->decode_to_value->value.array_value.count);
6841+
if (calloc_size == SIZE_MAX ||
6842+
(internal_decoder_data->decode_to_value->value.array_value.items = (AMQP_VALUE*)calloc(1, calloc_size)) == NULL)
68316843
{
6832-
LogError("Could not allocate memory for array items");
6844+
LogError("Could not allocate memory for array items, size:%zu", calloc_size);
68336845
result = MU_FAILURE;
68346846
}
68356847
else
@@ -6860,10 +6872,11 @@ static int internal_decoder_decode_bytes(INTERNAL_DECODER_DATA* internal_decoder
68606872
else
68616873
{
68626874
uint32_t i;
6863-
internal_decoder_data->decode_to_value->value.array_value.items = (AMQP_VALUE*)calloc(1, (sizeof(AMQP_VALUE) * internal_decoder_data->decode_to_value->value.array_value.count));
6864-
if (internal_decoder_data->decode_to_value->value.array_value.items == NULL)
6875+
size_t calloc_size = safe_multiply_size_t(sizeof(AMQP_VALUE), internal_decoder_data->decode_to_value->value.array_value.count);
6876+
if (calloc_size == SIZE_MAX ||
6877+
(internal_decoder_data->decode_to_value->value.array_value.items = (AMQP_VALUE*)calloc(1, calloc_size)) == NULL)
68656878
{
6866-
LogError("Could not allocate memory for array items");
6879+
LogError("Could not allocate memory for array items, size:%zu", calloc_size);
68676880
result = MU_FAILURE;
68686881
}
68696882
else

‎src/amqpvalue_to_string.c‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include "azure_c_shared_utility/gballoc.h"
1212
#include "azure_c_shared_utility/xlogging.h"
1313
#include "azure_c_shared_utility/uuid.h"
14+
#include "azure_c_shared_utility/safe_math.h"
1415
#include "azure_uamqp_c/amqpvalue_to_string.h"
1516
#include "azure_uamqp_c/amqpvalue.h"
1617

@@ -35,10 +36,11 @@ static int string_concat(char** string, const char* to_concat)
3536
src_length = 0;
3637
}
3738

38-
new_string = (char*)realloc(*string, src_length + length);
39-
if (new_string == NULL)
39+
size_t realloc_size = safe_add_size_t(src_length, length);
40+
if (realloc_size == SIZE_MAX ||
41+
(new_string = (char*)realloc(*string, realloc_size)) == NULL)
4042
{
41-
LogError("Cannot allocate memory for the new string");
43+
LogError("Cannot allocate memory for the new string, size:%zu", realloc_size);
4244
result = MU_FAILURE;
4345
}
4446
else

‎src/connection.c‎

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "azure_c_shared_utility/xio.h"
1010
#include "azure_c_shared_utility/xlogging.h"
1111
#include "azure_c_shared_utility/tickcounter.h"
12+
#include "azure_c_shared_utility/safe_math.h"
1213

1314
#include "azure_uamqp_c/connection.h"
1415
#include "azure_uamqp_c/frame_codec.h"
@@ -1897,11 +1898,13 @@ ENDPOINT_HANDLE connection_create_endpoint(CONNECTION_HANDLE connection)
18971898
result->connection = connection;
18981899

18991900
/* Codes_S_R_S_CONNECTION_01_197: [The newly created endpoint shall be added to the endpoints list, so that it can be tracked.] */
1900-
new_endpoints = (ENDPOINT_HANDLE*)realloc(connection->endpoints, sizeof(ENDPOINT_HANDLE) * ((size_t)connection->endpoint_count + 1));
1901-
if (new_endpoints == NULL)
1901+
size_t realloc_size = safe_add_size_t((size_t)connection->endpoint_count, 1);
1902+
realloc_size = safe_multiply_size_t(realloc_size, sizeof(ENDPOINT_HANDLE));
1903+
if (realloc_size == SIZE_MAX ||
1904+
(new_endpoints = (ENDPOINT_HANDLE*)realloc(connection->endpoints, realloc_size)) == NULL)
19021905
{
19031906
/* Tests_S_R_S_CONNECTION_01_198: [If adding the endpoint to the endpoints list tracked by the connection fails, connection_create_endpoint shall fail and return NULL.] */
1904-
LogError("Cannot reallocate memory for connection endpoints");
1907+
LogError("Cannot reallocate memory for connection endpoints, size:%zu", realloc_size);
19051908
free(result);
19061909
result = NULL;
19071910
}
@@ -1911,7 +1914,15 @@ ENDPOINT_HANDLE connection_create_endpoint(CONNECTION_HANDLE connection)
19111914

19121915
if (i < connection->endpoint_count)
19131916
{
1914-
(void)memmove(&connection->endpoints[i + 1], &connection->endpoints[i], sizeof(ENDPOINT_INSTANCE*) * (connection->endpoint_count - i));
1917+
size_t memmove_size = safe_multiply_size_t(safe_subtract_size_t(connection->endpoint_count, i), sizeof(ENDPOINT_INSTANCE*));
1918+
if (memmove_size != SIZE_MAX)
1919+
{
1920+
(void)memmove(&connection->endpoints[i + 1], &connection->endpoints[i], memmove_size);
1921+
}
1922+
else
1923+
{
1924+
LogError("Cannot memmove endpoints, size:%zu", memmove_size);
1925+
}
19151926
}
19161927

19171928
connection->endpoints[i] = result;
@@ -2004,14 +2015,28 @@ void connection_destroy_endpoint(ENDPOINT_HANDLE endpoint)
20042015
else
20052016
{
20062017
ENDPOINT_HANDLE* new_endpoints;
2007-
2008-
if ((connection->endpoint_count - i - 1) > 0)
2018+
size_t new_count = safe_subtract_size_t(safe_subtract_size_t(connection->endpoint_count, i), 1);
2019+
if (new_count > 0)
20092020
{
2010-
(void)memmove(connection->endpoints + i, connection->endpoints + i + 1, sizeof(ENDPOINT_HANDLE) * (connection->endpoint_count - i - 1));
2021+
size_t memmove_size = safe_multiply_size_t(safe_subtract_size_t(safe_subtract_size_t(connection->endpoint_count, i), 1), sizeof(ENDPOINT_HANDLE));
2022+
if (memmove_size != SIZE_MAX)
2023+
{
2024+
(void)memmove(connection->endpoints + i, connection->endpoints + i + 1, memmove_size);
2025+
}
2026+
else
2027+
{
2028+
LogError("Cannot memmove endpoints, size:%zu", memmove_size);
2029+
}
20112030
}
20122031

2013-
new_endpoints = (ENDPOINT_HANDLE*)realloc(connection->endpoints, (connection->endpoint_count - 1) * sizeof(ENDPOINT_HANDLE));
2014-
if (new_endpoints != NULL)
2032+
size_t realloc_size = safe_subtract_size_t(connection->endpoint_count, 1);
2033+
realloc_size = safe_multiply_size_t(realloc_size, sizeof(ENDPOINT_HANDLE));
2034+
if (realloc_size == SIZE_MAX ||
2035+
(new_endpoints = (ENDPOINT_HANDLE*)realloc(connection->endpoints, realloc_size)) == NULL)
2036+
{
2037+
LogError("Memory realloc failed, size:%zu", realloc_size);
2038+
}
2039+
else
20152040
{
20162041
connection->endpoints = new_endpoints;
20172042
}

‎src/header_detect_io.c‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "azure_c_shared_utility/gballoc.h"
99
#include "azure_c_shared_utility/xlogging.h"
1010
#include "azure_c_shared_utility/singlylinkedlist.h"
11+
#include "azure_c_shared_utility/safe_math.h"
1112
#include "azure_uamqp_c/header_detect_io.h"
1213
#include "azure_uamqp_c/server_protocol_io.h"
1314

@@ -529,9 +530,11 @@ static CONCRETE_IO_HANDLE header_detect_io_create(void* io_create_parameters)
529530
else
530531
{
531532
/* Codes_SRS_HEADER_DETECT_IO_01_009: [ The `header_detect_entries` array shall be copied so that it can be later used when detecting which header was received. ]*/
532-
result->header_detect_entries = (INTERNAL_HEADER_DETECT_ENTRY*)calloc(1, (header_detect_io_config->header_detect_entry_count * sizeof(INTERNAL_HEADER_DETECT_ENTRY)));
533-
if (result->header_detect_entries == NULL)
533+
size_t calloc_size = safe_multiply_size_t(header_detect_io_config->header_detect_entry_count, sizeof(INTERNAL_HEADER_DETECT_ENTRY));
534+
if (calloc_size == SIZE_MAX ||
535+
(result->header_detect_entries = (INTERNAL_HEADER_DETECT_ENTRY*)calloc(1, calloc_size)) == NULL)
534536
{
537+
LogError("Could not allocate header_detect_entries, size:%zu", calloc_size);
535538
free(result);
536539
result = NULL;
537540
}

‎src/link.c‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include "azure_c_shared_utility/xlogging.h"
1111
#include "azure_c_shared_utility/singlylinkedlist.h"
1212
#include "azure_c_shared_utility/tickcounter.h"
13+
#include "azure_c_shared_utility/safe_math.h"
1314
#include "azure_uamqp_c/link.h"
1415
#include "azure_uamqp_c/session.h"
1516
#include "azure_uamqp_c/amqpvalue.h"
@@ -448,10 +449,12 @@ static void link_frame_received(void* context, AMQP_VALUE performative, uint32_t
448449
/* If this is a continuation transfer or if this is the first chunk of a multi frame transfer */
449450
if ((link_instance->received_payload_size > 0) || more)
450451
{
451-
unsigned char* new_received_payload = (unsigned char*)realloc(link_instance->received_payload, (size_t)link_instance->received_payload_size + payload_size);
452-
if (new_received_payload == NULL)
452+
unsigned char* new_received_payload;;
453+
size_t realloc_size = safe_add_size_t((size_t)link_instance->received_payload_size, payload_size);
454+
if (realloc_size == SIZE_MAX ||
455+
(new_received_payload = (unsigned char*)realloc(link_instance->received_payload, realloc_size)) == NULL)
453456
{
454-
LogError("Could not allocate memory for the received payload");
457+
LogError("Could not allocate memory for the received payload, size:%zu", realloc_size);
455458
}
456459
else
457460
{

‎src/message.c‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,7 @@ MESSAGE_HANDLE message_clone(MESSAGE_HANDLE source_message)
238238
if (calloc_size == SIZE_MAX)
239239
{
240240
LogError("Invalid size for body_amqp_data_items");
241+
message_destroy(result);
241242
result = NULL;
242243
}
243244
else
@@ -1022,7 +1023,7 @@ int message_add_body_amqp_data(MESSAGE_HANDLE message, BINARY_DATA amqp_data)
10221023
if ((message == NULL) ||
10231024
/* Tests_SRS_MESSAGE_01_089: [ If the `bytes` member of `amqp_data` is NULL and the `size` member is non-zero, `message_add_body_amqp_data` shall fail and return a non-zero value. ]*/
10241025
((amqp_data.bytes == NULL) &&
1025-
(amqp_data.length != 0)))
1026+
(amqp_data.length != 0)))
10261027
{
10271028
LogError("Bad arguments: message = %p, bytes = %p, length = %u",
10281029
message, amqp_data.bytes, (unsigned int)amqp_data.length);
@@ -1471,7 +1472,7 @@ int message_set_message_format(MESSAGE_HANDLE message, uint32_t message_format)
14711472
return result;
14721473
}
14731474

1474-
int message_get_message_format(MESSAGE_HANDLE message, uint32_t *message_format)
1475+
int message_get_message_format(MESSAGE_HANDLE message, uint32_t* message_format)
14751476
{
14761477
int result;
14771478

0 commit comments

Comments
 (0)