malloc/tst-posix_memalign.c: Tidy up code.

Message ID 524D6209.8080005@linaro.org
State Accepted
Headers show

Commit Message

Will Newton Oct. 3, 2013, 12:24 p.m.
Add some comments and call free on all potentially allocated pointers.

ChangeLog:

2013-10-03  Will Newton  <will.newton@linaro.org>

	* malloc/tst-posix_memalign.c: Add comments.
	(do_test): Add comments and call free on all potentially
	allocated pointers. Add space after cast.
---
 malloc/tst-posix_memalign.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Carlos O'Donell Oct. 3, 2013, 8:57 p.m. | #1
On 10/03/2013 08:24 AM, Will Newton wrote:
> 
> Add some comments and call free on all potentially allocated pointers.
> 
> ChangeLog:
> 
> 2013-10-03  Will Newton  <will.newton@linaro.org>
> 
> 	* malloc/tst-posix_memalign.c: Add comments.
> 	(do_test): Add comments and call free on all potentially
> 	allocated pointers. Add space after cast.

Looks good to me. Thanks for the cleanup.

> ---
>  malloc/tst-posix_memalign.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/malloc/tst-posix_memalign.c b/malloc/tst-posix_memalign.c
> index 9d9d1bf..276757b 100644
> --- a/malloc/tst-posix_memalign.c
> +++ b/malloc/tst-posix_memalign.c
> @@ -1,4 +1,5 @@
> -/* Copyright (C) 2013 Free Software Foundation, Inc.
> +/* Test for posix_memalign.
> +   Copyright (C) 2013 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
> 
>     The GNU C Library is free software; you can redistribute it and/or
> @@ -40,6 +41,8 @@ do_test (void)
> 
>    p = NULL;
> 
> +  /* An attempt to allocate a huge value should return ENOMEM and
> +     p should remain NULL.  */
>    ret = posix_memalign (&p, sizeof (void *), -1);
> 
>    if (ret != ENOMEM)
> @@ -48,15 +51,22 @@ do_test (void)
>    if (ret == ENOMEM && p != NULL)
>      merror ("returned an error but pointer was modified");
> 
> +  free (p);
> +
>    p = NULL;
> 
> +  /* Test to expose integer overflow in malloc internals from BZ #15857.  */
>    ret = posix_memalign (&p, pagesize, -pagesize);
> 
>    if (ret != ENOMEM)
>      merror ("posix_memalign (&p, pagesize, -pagesize) succeeded.");
> 
> +  free (p);
> +
>    p = NULL;
> 
> +  /* A zero-sized allocation should succeed with glibc, returning zero
> +     and setting p to a non-NULL value.  */
>    ret = posix_memalign (&p, sizeof (void *), 0);
> 
>    if (ret != 0 || p == NULL)
> @@ -84,9 +94,9 @@ do_test (void)
>    if (ret == 0 && p == NULL)
>      merror ("returned success but pointer is NULL");
> 
> -  ptrval = (unsigned long)p;
> +  ptrval = (unsigned long) p;
> 
> -  if (ret == 0 && (ptrval & 0xff))
> +  if (ret == 0 && (ptrval & 0xff) != 0)
>      merror ("pointer is not aligned to 0x100");
> 
>    free (p);
>
Andreas Jaeger Oct. 9, 2013, 11:11 a.m. | #2
On 10/03/2013 02:24 PM, Will Newton wrote:
> 
> Add some comments and call free on all potentially allocated pointers.

Will, does the test work for you?

I get:

*** Error in `/home/aj/build/glibc/x86-64/malloc/tst-posix_memalign':
free(): invalid next size (normal): 0x00007f7c3ba94010 ***

Note that I use MALLOC_CHECK_=3 in my environment.

Could you fix it the test, please?

Andreas
Will Newton Oct. 9, 2013, 1:48 p.m. | #3
On 9 October 2013 12:11, Andreas Jaeger <aj@suse.com> wrote:
> On 10/03/2013 02:24 PM, Will Newton wrote:
>>
>> Add some comments and call free on all potentially allocated pointers.
>
> Will, does the test work for you?

Yes, it works in my testing so far.

> I get:
>
> *** Error in `/home/aj/build/glibc/x86-64/malloc/tst-posix_memalign':
> free(): invalid next size (normal): 0x00007f7c3ba94010 ***
>
> Note that I use MALLOC_CHECK_=3 in my environment.
>
> Could you fix it the test, please?

The test is working as expected, the problem is caused by a bug in the
malloc check implementation. I have submitted a patch, thanks for
picking this up!

Patch

diff --git a/malloc/tst-posix_memalign.c b/malloc/tst-posix_memalign.c
index 9d9d1bf..276757b 100644
--- a/malloc/tst-posix_memalign.c
+++ b/malloc/tst-posix_memalign.c
@@ -1,4 +1,5 @@ 
-/* Copyright (C) 2013 Free Software Foundation, Inc.
+/* Test for posix_memalign.
+   Copyright (C) 2013 Free Software Foundation, Inc.
    This file is part of the GNU C Library.

    The GNU C Library is free software; you can redistribute it and/or
@@ -40,6 +41,8 @@  do_test (void)

   p = NULL;

+  /* An attempt to allocate a huge value should return ENOMEM and
+     p should remain NULL.  */
   ret = posix_memalign (&p, sizeof (void *), -1);

   if (ret != ENOMEM)
@@ -48,15 +51,22 @@  do_test (void)
   if (ret == ENOMEM && p != NULL)
     merror ("returned an error but pointer was modified");

+  free (p);
+
   p = NULL;

+  /* Test to expose integer overflow in malloc internals from BZ #15857.  */
   ret = posix_memalign (&p, pagesize, -pagesize);

   if (ret != ENOMEM)
     merror ("posix_memalign (&p, pagesize, -pagesize) succeeded.");

+  free (p);
+
   p = NULL;

+  /* A zero-sized allocation should succeed with glibc, returning zero
+     and setting p to a non-NULL value.  */
   ret = posix_memalign (&p, sizeof (void *), 0);

   if (ret != 0 || p == NULL)
@@ -84,9 +94,9 @@  do_test (void)
   if (ret == 0 && p == NULL)
     merror ("returned success but pointer is NULL");

-  ptrval = (unsigned long)p;
+  ptrval = (unsigned long) p;

-  if (ret == 0 && (ptrval & 0xff))
+  if (ret == 0 && (ptrval & 0xff) != 0)
     merror ("pointer is not aligned to 0x100");

   free (p);