alloc: Add more memalign asserts

PATCH In review Janosch Frank 3 participants 5 messages Nov 4, 2019

Subsystems (2)

KVM Library code

lore

Share & tools

lore

Janosch Frank

author

2019-11-04 09:20 UTC #0 lore raw

PATCH 1/1 alloc: Add more memalign asserts

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 <[email protected]>

---

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

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

lib/alloc.c +3 -0

index ecdbbc4..eba9dd6 100644
@@ -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);

Signature · 1 line

2.20.1

David Hildenbrand

Reviewed-by on v1 1/1

2019-11-04 10:01 UTC #1 lore raw

Quoted 28 lines · 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 <[email protected]>
---
Tested only under s390, tests under other architectures are highly appreciated.
---
  lib/alloc.c | 3 +++
  1 file changed, 3 insertions(+)
lib/alloc.c +1 -0
index ecdbbc4..eba9dd6 100644
@@ -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);

lib/alloc.c void *memalign(size_t alignment, size_t size) · line 64

Quoted 8 lines

lib/alloc.c +2 -0

@@ -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 <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

Paolo Bonzini

maintainer

2019-11-04 10:07 UTC #2 lore raw

In reply to Janosch Frank

Quoted 19 lines · 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 <[email protected]>
---
Tested only under s390, tests under other architectures are highly appreciated.
---
 lib/alloc.c | 3 +++
 1 file changed, 3 insertions(+)
lib/alloc.c +0 -0
index ecdbbc4..eba9dd6 100644

lib/alloc.c void *memalign(size_t alignment, size_t size) · line 49

Quoted 4 lines

lib/alloc.c +1 -0

@@ -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

Quoted 13 lines

lib/alloc.c +2 -0

 	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

author

2019-11-04 10:12 UTC #3 lore raw

Quoted 23 lines · Paolo Bonzini

Quoted 22 lines · 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 <[email protected]>
---
Tested only under s390, tests under other architectures are highly appreciated.
---
 lib/alloc.c | 3 +++
 1 file changed, 3 insertions(+)
lib/alloc.c +0 -0
index ecdbbc4..eba9dd6 100644
@@ -46,6 +46,7 @@ void *memalign(size_t alignment, size_t size)
 	uintptr_t blkalign;
 	uintptr_t mem;
 

Quoted 6 lines

lib/alloc.c void *memalign(size_t alignment, size_t size) · line 49
Quoted 1 line
lib/alloc.c +1 -0
+	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.

Quoted 15 lines

Quoted 14 lines

lib/alloc.c +2 -0

 	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

maintainer

2019-11-04 10:16 UTC #4 lore raw

Quoted 29 lines · Janosch Frank

Quoted 28 lines · Paolo Bonzini
Quoted 19 lines · 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 <[email protected]>
---
Tested only under s390, tests under other architectures are highly appreciated.
---
 lib/alloc.c | 3 +++
 1 file changed, 3 insertions(+)
lib/alloc.c +0 -0
index ecdbbc4..eba9dd6 100644
lib/alloc.c void *memalign(size_t alignment, size_t size) · line 49
Quoted 4 lines
lib/alloc.c +1 -0
@@ -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.

Quoted 6 lines

Quoted 1 line
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

Quoted 17 lines

Quoted 15 lines

Quoted 14 lines

lib/alloc.c +2 -0

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