Add cupsArrayDup test and overflow protection in array allocation#151
Add cupsArrayDup test and overflow protection in array allocation#151tanjiroK-coder wants to merge 1 commit into
Conversation
michaelrsweet
left a comment
michaelrsweet
left a comment
There was a problem hiding this comment.
I appreciate the attention to detail, although it is quite likely that your process would be killed (or get a failed malloc/realloc) long before the overflow is reached.
My preference is to use a reasonable limit on the number of elements and hash size (2^30-1) that will work for 32-bit and 64-bit platforms. And any new tests need to be added to the existing "testarray.c" unit test program.
| static bool cups_array_add(cups_array_t *a, void *e, bool insert); | ||
| static size_t cups_array_find(cups_array_t *a, void *e, size_t prev, int *rdiff); | ||
|
|
||
| static bool cups_array_multiply(size_t count, size_t size, size_t *bytes); |
There was a problem hiding this comment.
No, let's just define a reasonable maximum for num_elements (maybe 2^30-1 which is ~1 billion elements) and limit it accordingly.
#define _CUPS_ARRAY_MAX_ELEM 0x1fffffff| // Allocate memory for the elements... | ||
| da->elements = malloc((size_t)a->num_elements * sizeof(void *)); | ||
| if (!da->elements) | ||
| size_t bytes; |
There was a problem hiding this comment.
Assuming that you are duplicating an array you've created using the normal APIs, it is unlikely that duplicating will overflow. But for completeness-sake we can verify that a->num_elements <= _CUPS_MAXELEM before trying to allocate that much memory.
| da->alloc_elements = a->num_elements; | ||
| } | ||
|
|
||
| /* Copy hash table if present so duplicated arrays preserve hashed lookups */ |
There was a problem hiding this comment.
Copying the hash is important, but again the size should already be limited.
|
|
||
| if (hsize > 0 && hf) | ||
| { | ||
| size_t bytes; |
There was a problem hiding this comment.
Add a _CUPS_MAXHASH limit and ignore the hashing if it exceeds the limit, e.g.:
#define _CUPS_MAXHASH 0x1fffffff
...
if (hsize > 0 && hsize <= _CUPS_MAXHASH && hf)
{
...
}| count = a->alloc_elements + 1024; | ||
|
|
||
| if ((temp = realloc(a->elements, count * sizeof(void *))) == NULL) | ||
| size_t bytes; |
There was a problem hiding this comment.
Limit count to _CUPS_MAXELEMS.
| @@ -0,0 +1,19 @@ | |||
| Test helper: cupsArrayDup | |||
There was a problem hiding this comment.
NO.
The "testarray.c" unit test program tests the cupsArray API. Add to that please.
| @@ -0,0 +1,100 @@ | |||
| #include <stdio.h> | |||
There was a problem hiding this comment.
This should be part of the existing "testarray.c".
2 participants
I fixed a bug in
cupsArrayDup()where the code could calculate a huge allocation size and accidentally overflow the multiplication when doingnum_elements * sizeof(void *)So now the patch checks that size safely before calling
malloc()and also uses the same safe check for hash allocations.I also added a regression test in
tests/test_array_dup.cto make sure the function rejects an overflowed size instead of trying to allocate it.This is a real hardening fix for a memory-safety issue, not just a small cleanup.