[kvm-unit-tests] alloc: Add more memalign asserts
diff mbox series

Message ID 20191104092055.5679-1-frankja@linux.ibm.com
State New
Headers show
Series
  • [kvm-unit-tests] alloc: Add more memalign asserts
Related show

Commit Message

Janosch Frank Nov. 4, 2019, 9:20 a.m. UTC
Let's test for size and alignment in memalign to catch invalid input
data. Also we need to test for NULL after calling the memalign
function of the registered alloc operations.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---

Tested only under s390, tests under other architectures are highly
appreciated.

---
 lib/alloc.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

David Hildenbrand Nov. 4, 2019, 10:01 a.m. UTC | #1
On 04.11.19 10:20, Janosch Frank wrote:
> Let's test for size and alignment in memalign to catch invalid input
> data. Also we need to test for NULL after calling the memalign
> function of the registered alloc operations.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> 
> Tested only under s390, tests under other architectures are highly
> appreciated.
> 
> ---
>   lib/alloc.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/lib/alloc.c b/lib/alloc.c
> index ecdbbc4..eba9dd6 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -46,6 +46,7 @@ void *memalign(size_t alignment, size_t size)
>   	uintptr_t blkalign;
>   	uintptr_t mem;
>   
> +	assert(size && alignment);
>   	assert(alloc_ops && alloc_ops->memalign);
>   	if (alignment <= sizeof(uintptr_t))
>   		alignment = sizeof(uintptr_t);
> @@ -56,6 +57,8 @@ void *memalign(size_t alignment, size_t size)
>   	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
>   	p = alloc_ops->memalign(blkalign, size);
>   
> +	assert(p != NULL);
> +
>   	/* Leave room for metadata before aligning the result.  */
>   	mem = (uintptr_t)p + METADATA_EXTRA;
>   	mem = ALIGN(mem, alignment);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>
Paolo Bonzini Nov. 4, 2019, 10:07 a.m. UTC | #2
On 04/11/19 10:20, Janosch Frank wrote:
> Let's test for size and alignment in memalign to catch invalid input
> data. Also we need to test for NULL after calling the memalign
> function of the registered alloc operations.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> 
> Tested only under s390, tests under other architectures are highly
> appreciated.
> 
> ---
>  lib/alloc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/alloc.c b/lib/alloc.c
> index ecdbbc4..eba9dd6 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -46,6 +46,7 @@ void *memalign(size_t alignment, size_t size)
>  	uintptr_t blkalign;
>  	uintptr_t mem;
>  
> +	assert(size && alignment);

Do we want to return NULL instead on !size?  This is how malloc(3) is
documented.

Paolo

>  	assert(alloc_ops && alloc_ops->memalign);
>  	if (alignment <= sizeof(uintptr_t))
>  		alignment = sizeof(uintptr_t);
> @@ -56,6 +57,8 @@ void *memalign(size_t alignment, size_t size)
>  	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
>  	p = alloc_ops->memalign(blkalign, size);
>  
> +	assert(p != NULL);
> +
>  	/* Leave room for metadata before aligning the result.  */
>  	mem = (uintptr_t)p + METADATA_EXTRA;
>  	mem = ALIGN(mem, alignment);
>
Janosch Frank Nov. 4, 2019, 10:12 a.m. UTC | #3
On 11/4/19 11:07 AM, Paolo Bonzini wrote:
> On 04/11/19 10:20, Janosch Frank wrote:
>> Let's test for size and alignment in memalign to catch invalid input
>> data. Also we need to test for NULL after calling the memalign
>> function of the registered alloc operations.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>
>> Tested only under s390, tests under other architectures are highly
>> appreciated.
>>
>> ---
>>  lib/alloc.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/lib/alloc.c b/lib/alloc.c
>> index ecdbbc4..eba9dd6 100644
>> --- a/lib/alloc.c
>> +++ b/lib/alloc.c
>> @@ -46,6 +46,7 @@ void *memalign(size_t alignment, size_t size)
>>  	uintptr_t blkalign;
>>  	uintptr_t mem;
>>  
>> +	assert(size && alignment);
> 
> Do we want to return NULL instead on !size?  This is how malloc(3) is
> documented.
> 
> Paolo

I myself never check for NULL on a unit test and therefore added the
asserts to have it fail visibly.

But sure, we can return NULL for both asserts.

> 
>>  	assert(alloc_ops && alloc_ops->memalign);
>>  	if (alignment <= sizeof(uintptr_t))
>>  		alignment = sizeof(uintptr_t);
>> @@ -56,6 +57,8 @@ void *memalign(size_t alignment, size_t size)
>>  	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
>>  	p = alloc_ops->memalign(blkalign, size);
>>  
>> +	assert(p != NULL);
>> +
>>  	/* Leave room for metadata before aligning the result.  */
>>  	mem = (uintptr_t)p + METADATA_EXTRA;
>>  	mem = ALIGN(mem, alignment);
>>
>
Paolo Bonzini Nov. 4, 2019, 10:16 a.m. UTC | #4
On 04/11/19 11:12, Janosch Frank wrote:
> On 11/4/19 11:07 AM, Paolo Bonzini wrote:
>> On 04/11/19 10:20, Janosch Frank wrote:
>>> Let's test for size and alignment in memalign to catch invalid input
>>> data. Also we need to test for NULL after calling the memalign
>>> function of the registered alloc operations.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>
>>> Tested only under s390, tests under other architectures are highly
>>> appreciated.
>>>
>>> ---
>>>  lib/alloc.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/lib/alloc.c b/lib/alloc.c
>>> index ecdbbc4..eba9dd6 100644
>>> --- a/lib/alloc.c
>>> +++ b/lib/alloc.c
>>> @@ -46,6 +46,7 @@ void *memalign(size_t alignment, size_t size)
>>>  	uintptr_t blkalign;
>>>  	uintptr_t mem;
>>>  
>>> +	assert(size && alignment);
>>
>> Do we want to return NULL instead on !size?  This is how malloc(3) is
>> documented.
>>
>> Paolo
> 
> I myself never check for NULL on a unit test and therefore added the
> asserts to have it fail visibly.
> 
> But sure, we can return NULL for both asserts.

Hmm yeah for ->memalign it makes sense since unit tests won't SIGSEGV.
For !size let's return NULL, it can simplify code a bit.

Paolo

>>
>>>  	assert(alloc_ops && alloc_ops->memalign);
>>>  	if (alignment <= sizeof(uintptr_t))
>>>  		alignment = sizeof(uintptr_t);
>>> @@ -56,6 +57,8 @@ void *memalign(size_t alignment, size_t size)
>>>  	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
>>>  	p = alloc_ops->memalign(blkalign, size);
>>>  
>>> +	assert(p != NULL);
>>> +
>>>  	/* Leave room for metadata before aligning the result.  */
>>>  	mem = (uintptr_t)p + METADATA_EXTRA;
>>>  	mem = ALIGN(mem, alignment);
>>>
>>
> 
>

Patch
diff mbox series

diff --git a/lib/alloc.c b/lib/alloc.c
index ecdbbc4..eba9dd6 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -46,6 +46,7 @@  void *memalign(size_t alignment, size_t size)
 	uintptr_t blkalign;
 	uintptr_t mem;
 
+	assert(size && alignment);
 	assert(alloc_ops && alloc_ops->memalign);
 	if (alignment <= sizeof(uintptr_t))
 		alignment = sizeof(uintptr_t);
@@ -56,6 +57,8 @@  void *memalign(size_t alignment, size_t size)
 	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
 	p = alloc_ops->memalign(blkalign, size);
 
+	assert(p != NULL);
+
 	/* Leave room for metadata before aligning the result.  */
 	mem = (uintptr_t)p + METADATA_EXTRA;
 	mem = ALIGN(mem, alignment);