| Message ID | 20191104102916.10554-1-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v2] alloc: Add memalign error checks | expand |
On 04.11.19 11:29, 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> > --- > lib/alloc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/lib/alloc.c b/lib/alloc.c > index ecdbbc4..b763c70 100644 > --- a/lib/alloc.c > +++ b/lib/alloc.c > @@ -47,6 +47,8 @@ void *memalign(size_t alignment, size_t size) > uintptr_t mem; > > assert(alloc_ops && alloc_ops->memalign); > + if (!size || !alignment) > + return NULL; > if (alignment <= sizeof(uintptr_t)) > alignment = sizeof(uintptr_t); BTW, memalign MAN page "EINVAL The alignment argument was not a power of two, or was not a multiple of sizeof(void *)." So we could also return NULL here (not sure if anybody relies on that) > else > @@ -55,6 +57,7 @@ void *memalign(size_t alignment, size_t size) > blkalign = MAX(alignment, alloc_ops->align_min); > size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min); > p = alloc_ops->memalign(blkalign, size); > + assert(p); > > /* Leave room for metadata before aligning the result. */ > mem = (uintptr_t)p + METADATA_EXTRA; > Reviewed-by: David Hildenbrand <david@redhat.com>
On Mon, Nov 04, 2019 at 11:33:28AM +0100, David Hildenbrand wrote: > On 04.11.19 11:29, 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> > > --- > > lib/alloc.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/lib/alloc.c b/lib/alloc.c > > index ecdbbc4..b763c70 100644 > > --- a/lib/alloc.c > > +++ b/lib/alloc.c > > @@ -47,6 +47,8 @@ void *memalign(size_t alignment, size_t size) > > uintptr_t mem; > > assert(alloc_ops && alloc_ops->memalign); > > + if (!size || !alignment) > > + return NULL; > > if (alignment <= sizeof(uintptr_t)) > > alignment = sizeof(uintptr_t); > > BTW, memalign MAN page > > "EINVAL The alignment argument was not a power of two, or was not a multiple > of sizeof(void *)." > Since we're not implementing the EINVAL part, then I'd assert when alignment isn't correct. > So we could also return NULL here (not sure if anybody relies on that) I made the following changes and tested with arm/arm64. No problems. Thanks, drew diff --git a/lib/alloc.c b/lib/alloc.c index ecdbbc44dbf9..ed8f5f94c9b0 100644 --- a/lib/alloc.c +++ b/lib/alloc.c @@ -46,15 +46,17 @@ void *memalign(size_t alignment, size_t size) uintptr_t blkalign; uintptr_t mem; + if (!size) + return NULL; + + assert(alignment >= sizeof(void *) && is_power_of_2(alignment)); assert(alloc_ops && alloc_ops->memalign); - if (alignment <= sizeof(uintptr_t)) - alignment = sizeof(uintptr_t); - else - size += alignment - 1; + size += alignment - 1; blkalign = MAX(alignment, alloc_ops->align_min); size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min); p = alloc_ops->memalign(blkalign, size); + assert(p); /* Leave room for metadata before aligning the result. */ mem = (uintptr_t)p + METADATA_EXTRA;
On 04/11/19 11:54, Andrew Jones wrote: > > diff --git a/lib/alloc.c b/lib/alloc.c > index ecdbbc44dbf9..ed8f5f94c9b0 100644 > --- a/lib/alloc.c > +++ b/lib/alloc.c > @@ -46,15 +46,17 @@ void *memalign(size_t alignment, size_t size) > uintptr_t blkalign; > uintptr_t mem; > > + if (!size) > + return NULL; > + > + assert(alignment >= sizeof(void *) && is_power_of_2(alignment)); > assert(alloc_ops && alloc_ops->memalign); > - if (alignment <= sizeof(uintptr_t)) > - alignment = sizeof(uintptr_t); > - else > - size += alignment - 1; > > + size += alignment - 1; > blkalign = MAX(alignment, alloc_ops->align_min); > size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min); > p = alloc_ops->memalign(blkalign, size); > + assert(p); > > /* Leave room for metadata before aligning the result. */ > mem = (uintptr_t)p + METADATA_EXTRA; Looks good, this is what I am queuing. Paolo
On 04.11.19 12:29, Paolo Bonzini wrote: > On 04/11/19 11:54, Andrew Jones wrote: >> >> diff --git a/lib/alloc.c b/lib/alloc.c >> index ecdbbc44dbf9..ed8f5f94c9b0 100644 >> --- a/lib/alloc.c >> +++ b/lib/alloc.c >> @@ -46,15 +46,17 @@ void *memalign(size_t alignment, size_t size) >> uintptr_t blkalign; >> uintptr_t mem; >> >> + if (!size) >> + return NULL; >> + >> + assert(alignment >= sizeof(void *) && is_power_of_2(alignment)); >> assert(alloc_ops && alloc_ops->memalign); >> - if (alignment <= sizeof(uintptr_t)) >> - alignment = sizeof(uintptr_t); >> - else >> - size += alignment - 1; >> >> + size += alignment - 1; >> blkalign = MAX(alignment, alloc_ops->align_min); >> size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min); >> p = alloc_ops->memalign(blkalign, size); >> + assert(p); >> >> /* Leave room for metadata before aligning the result. */ >> mem = (uintptr_t)p + METADATA_EXTRA; > > Looks good, this is what I am queuing. > > Paolo > Please add my Reviewed-by: David Hildenbrand <david@redhat.com>
On 11/4/19 12:29 PM, Paolo Bonzini wrote: > On 04/11/19 11:54, Andrew Jones wrote: >> >> diff --git a/lib/alloc.c b/lib/alloc.c >> index ecdbbc44dbf9..ed8f5f94c9b0 100644 >> --- a/lib/alloc.c >> +++ b/lib/alloc.c >> @@ -46,15 +46,17 @@ void *memalign(size_t alignment, size_t size) >> uintptr_t blkalign; >> uintptr_t mem; >> >> + if (!size) >> + return NULL; >> + >> + assert(alignment >= sizeof(void *) && is_power_of_2(alignment)); >> assert(alloc_ops && alloc_ops->memalign); >> - if (alignment <= sizeof(uintptr_t)) >> - alignment = sizeof(uintptr_t); >> - else >> - size += alignment - 1; >> >> + size += alignment - 1; >> blkalign = MAX(alignment, alloc_ops->align_min); >> size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min); >> p = alloc_ops->memalign(blkalign, size); >> + assert(p); I had some more time to think about and test this and I think returning NULL here is more useful. My usecase is a limit test where I allocate until I get a NULL and then free everything afterwards. >> >> /* Leave room for metadata before aligning the result. */ >> mem = (uintptr_t)p + METADATA_EXTRA; > > Looks good, this is what I am queuing. > > Paolo
On Mon, Nov 11, 2019 at 11:12:41AM +0100, Janosch Frank wrote: > On 11/4/19 12:29 PM, Paolo Bonzini wrote: > > On 04/11/19 11:54, Andrew Jones wrote: > >> > >> diff --git a/lib/alloc.c b/lib/alloc.c > >> index ecdbbc44dbf9..ed8f5f94c9b0 100644 > >> --- a/lib/alloc.c > >> +++ b/lib/alloc.c > >> @@ -46,15 +46,17 @@ void *memalign(size_t alignment, size_t size) > >> uintptr_t blkalign; > >> uintptr_t mem; > >> > >> + if (!size) > >> + return NULL; > >> + > >> + assert(alignment >= sizeof(void *) && is_power_of_2(alignment)); > >> assert(alloc_ops && alloc_ops->memalign); > >> - if (alignment <= sizeof(uintptr_t)) > >> - alignment = sizeof(uintptr_t); > >> - else > >> - size += alignment - 1; > >> > >> + size += alignment - 1; > >> blkalign = MAX(alignment, alloc_ops->align_min); > >> size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min); > >> p = alloc_ops->memalign(blkalign, size); > >> + assert(p); > > I had some more time to think about and test this and I think returning > NULL here is more useful. My usecase is a limit test where I allocate > until I get a NULL and then free everything afterwards. I think I prefer the assert here, since most users of this will not be expecting to run out of memory, and therefore not checking for it. I think you may want to just write your own allocator in your unit test that's based on alloc_pages(). Thanks, drew
On 11/11/19 13:31, Andrew Jones wrote: >> I had some more time to think about and test this and I think returning >> NULL here is more useful. My usecase is a limit test where I allocate >> until I get a NULL and then free everything afterwards. > I think I prefer the assert here, since most users of this will not be > expecting to run out of memory, and therefore not checking for it. Yeah, the main issue with returning NULL is that (void*) 0 is a valid address for kvm-unit-tests. Paolo
On 11/11/19 2:04 PM, Paolo Bonzini wrote: > On 11/11/19 13:31, Andrew Jones wrote: >>> I had some more time to think about and test this and I think returning >>> NULL here is more useful. My usecase is a limit test where I allocate >>> until I get a NULL and then free everything afterwards. >> I think I prefer the assert here, since most users of this will not be >> expecting to run out of memory, and therefore not checking for it. > > Yeah, the main issue with returning NULL is that (void*) 0 is a valid > address for kvm-unit-tests. > > Paolo So, images are not loaded to 0 for non s390x architectures?
On 11/11/19 14:13, Janosch Frank wrote: > On 11/11/19 2:04 PM, Paolo Bonzini wrote: >> On 11/11/19 13:31, Andrew Jones wrote: >>>> I had some more time to think about and test this and I think returning >>>> NULL here is more useful. My usecase is a limit test where I allocate >>>> until I get a NULL and then free everything afterwards. >>> I think I prefer the assert here, since most users of this will not be >>> expecting to run out of memory, and therefore not checking for it. >> >> Yeah, the main issue with returning NULL is that (void*) 0 is a valid >> address for kvm-unit-tests. > > So, images are not loaded to 0 for non s390x architectures? It depends on the architecture. Either way, if there is an out of memory error it would turn into a rogue memory write. That would be harder to debug than an assertion failure. If there is need, we can add try_malloc and try_memalign, too. Paolo
diff --git a/lib/alloc.c b/lib/alloc.c index ecdbbc4..b763c70 100644 --- a/lib/alloc.c +++ b/lib/alloc.c @@ -47,6 +47,8 @@ void *memalign(size_t alignment, size_t size) uintptr_t mem; assert(alloc_ops && alloc_ops->memalign); + if (!size || !alignment) + return NULL; if (alignment <= sizeof(uintptr_t)) alignment = sizeof(uintptr_t); else @@ -55,6 +57,7 @@ void *memalign(size_t alignment, size_t size) blkalign = MAX(alignment, alloc_ops->align_min); size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min); p = alloc_ops->memalign(blkalign, size); + assert(p); /* Leave room for metadata before aligning the result. */ mem = (uintptr_t)p + METADATA_EXTRA;
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> --- lib/alloc.c | 3 +++ 1 file changed, 3 insertions(+)