[kvm-unit-tests PATCH] alloc: Add more memalign asserts

[kvm-unit-tests PATCH] alloc: Add more memalign asserts

[Janosch Frank]

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);
-- 
2.20.1

Re: [kvm-unit-tests PATCH] alloc: Add more memalign asserts

[David Hildenbrand]

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>

-- 

Thanks,

David / dhildenb

Re: [kvm-unit-tests PATCH] alloc: Add more memalign asserts

[Paolo Bonzini]

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);
> 

Re: [kvm-unit-tests PATCH] alloc: Add more memalign asserts

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1556 bytes --]

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);
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH] alloc: Add more memalign asserts

[Paolo Bonzini]

[-- Attachment #1.1: Type: text/plain, Size: 1798 bytes --]

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);
>>>
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]