[PATCH] Handle NULL input to malloc_usable_size [BZ #28506]

[PATCH] Handle NULL input to malloc_usable_size [BZ #28506]

[Siddhesh Poyarekar]

Hoist the NULL check for malloc_usable_size into its entry points in
malloc-debug and malloc and assume non-NULL in all callees.  This fixes
BZ #28506

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 malloc/malloc-debug.c      | 12 ++++++------
 malloc/malloc.c            | 23 ++++++++++-------------
 malloc/tst-malloc-usable.c | 21 ++++++++-------------
 3 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/malloc/malloc-debug.c b/malloc/malloc-debug.c
index 9922ef5f25..5e954d7dc2 100644
--- a/malloc/malloc-debug.c
+++ b/malloc/malloc-debug.c
@@ -399,17 +399,17 @@ strong_alias (__debug_calloc, calloc)
 size_t
 malloc_usable_size (void *mem)
 {
+  if (mem == NULL)
+    return 0;
+
   if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK))
     return mcheck_usable_size (mem);
   if (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))
     return malloc_check_get_size (mem);
 
-  if (mem != NULL)
-    {
-      mchunkptr p = mem2chunk (mem);
-     if (DUMPED_MAIN_ARENA_CHUNK (p))
-       return chunksize (p) - SIZE_SZ;
-    }
+  mchunkptr p = mem2chunk (mem);
+  if (DUMPED_MAIN_ARENA_CHUNK (p))
+    return chunksize (p) - SIZE_SZ;
 
   return musable (mem);
 }
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 2ba1fee144..9a345572a1 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -5008,29 +5008,26 @@ static size_t
 musable (void *mem)
 {
   mchunkptr p;
-  if (mem != 0)
-    {
-      size_t result = 0;
+  size_t result = 0;
 
-      p = mem2chunk (mem);
+  p = mem2chunk (mem);
 
-      if (chunk_is_mmapped (p))
-	result = chunksize (p) - CHUNK_HDR_SZ;
-      else if (inuse (p))
-	result = memsize (p);
+  if (chunk_is_mmapped (p))
+    result = chunksize (p) - CHUNK_HDR_SZ;
+  else if (inuse (p))
+    result = memsize (p);
 
-      return result;
-    }
-  return 0;
+  return result;
 }
 
 #if IS_IN (libc)
 size_t
 __malloc_usable_size (void *m)
 {
-  size_t result;
+  size_t result = 0;
 
-  result = musable (m);
+  if (m != NULL)
+    result = musable (m);
   return result;
 }
 #endif
diff --git a/malloc/tst-malloc-usable.c b/malloc/tst-malloc-usable.c
index a1074b782a..cd5c27cfcd 100644
--- a/malloc/tst-malloc-usable.c
+++ b/malloc/tst-malloc-usable.c
@@ -21,29 +21,24 @@
 #include <malloc.h>
 #include <string.h>
 #include <stdio.h>
+#include <support/support.h>
+#include <support/check.h>
 
 static int
 do_test (void)
 {
   size_t usable_size;
   void *p = malloc (7);
-  if (!p)
-    {
-      printf ("memory allocation failed\n");
-      return 1;
-    }
 
+  TEST_VERIFY_EXIT (p != NULL);
   usable_size = malloc_usable_size (p);
-  if (usable_size != 7)
-    {
-      printf ("malloc_usable_size: expected 7 but got %zu\n", usable_size);
-      return 1;
-    }
-
+  TEST_VERIFY_EXIT (usable_size == 7);
   memset (p, 0, usable_size);
   free (p);
+
+  TEST_VERIFY_EXIT (malloc_usable_size (NULL) == 0);
+
   return 0;
 }
 
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include "support/test-driver.c"
-- 
2.31.1

Re: [PATCH] Handle NULL input to malloc_usable_size [BZ #28506]

[Florian Weimer]

* Siddhesh Poyarekar via Libc-alpha:

>  size_t
>  __malloc_usable_size (void *m)
>  {
> +  size_t result = 0;
>  
> +  if (m != NULL)
> +    result = musable (m);
>    return result;
>  }

Is there a reason for not writing it this way?

size_t
__malloc_usable_size (void *m)
{
  if (m == NULL)
    return 0;
  return musable (m);
}

The extra variable seems a bit … unnecessary?

> diff --git a/malloc/tst-malloc-usable.c b/malloc/tst-malloc-usable.c
> index a1074b782a..cd5c27cfcd 100644
> --- a/malloc/tst-malloc-usable.c
> +++ b/malloc/tst-malloc-usable.c
> @@ -21,29 +21,24 @@
>  #include <malloc.h>
>  #include <string.h>
>  #include <stdio.h>
> +#include <support/support.h>
> +#include <support/check.h>
>  
>  static int
>  do_test (void)
>  {
>    size_t usable_size;
>    void *p = malloc (7);
>  
> +  TEST_VERIFY_EXIT (p != NULL);
>    usable_size = malloc_usable_size (p);
> +  TEST_VERIFY_EXIT (usable_size == 7);

You can use TEST_COMPARE here.

>    memset (p, 0, usable_size);
>    free (p);
> +
> +  TEST_VERIFY_EXIT (malloc_usable_size (NULL) == 0);

And here.

Thanks,
Florian

[PATCH v2] Handle NULL input to malloc_usable_size [BZ #28506]

[Siddhesh Poyarekar]

Hoist the NULL check for malloc_usable_size into its entry points in
malloc-debug and malloc and assume non-NULL in all callees.  This fixes
BZ #28506

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 malloc/malloc-debug.c      | 12 ++++++------
 malloc/malloc.c            | 24 ++++++++----------------
 malloc/tst-malloc-usable.c | 21 ++++++++-------------
 3 files changed, 22 insertions(+), 35 deletions(-)

diff --git a/malloc/malloc-debug.c b/malloc/malloc-debug.c
index 9922ef5f25..5e954d7dc2 100644
--- a/malloc/malloc-debug.c
+++ b/malloc/malloc-debug.c
@@ -399,17 +399,17 @@ strong_alias (__debug_calloc, calloc)
 size_t
 malloc_usable_size (void *mem)
 {
+  if (mem == NULL)
+    return 0;
+
   if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK))
     return mcheck_usable_size (mem);
   if (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))
     return malloc_check_get_size (mem);
 
-  if (mem != NULL)
-    {
-      mchunkptr p = mem2chunk (mem);
-     if (DUMPED_MAIN_ARENA_CHUNK (p))
-       return chunksize (p) - SIZE_SZ;
-    }
+  mchunkptr p = mem2chunk (mem);
+  if (DUMPED_MAIN_ARENA_CHUNK (p))
+    return chunksize (p) - SIZE_SZ;
 
   return musable (mem);
 }
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 2ba1fee144..a9dfc82788 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -5007,20 +5007,13 @@ __malloc_trim (size_t s)
 static size_t
 musable (void *mem)
 {
-  mchunkptr p;
-  if (mem != 0)
-    {
-      size_t result = 0;
-
-      p = mem2chunk (mem);
+  mchunkptr p = mem2chunk (mem);
 
-      if (chunk_is_mmapped (p))
-	result = chunksize (p) - CHUNK_HDR_SZ;
-      else if (inuse (p))
-	result = memsize (p);
+  if (chunk_is_mmapped (p))
+    return chunksize (p) - CHUNK_HDR_SZ;
+  else if (inuse (p))
+    return memsize (p);
 
-      return result;
-    }
   return 0;
 }
 
@@ -5028,10 +5021,9 @@ musable (void *mem)
 size_t
 __malloc_usable_size (void *m)
 {
-  size_t result;
-
-  result = musable (m);
-  return result;
+  if (m == NULL)
+    return 0;
+  return musable (m);
 }
 #endif
 
diff --git a/malloc/tst-malloc-usable.c b/malloc/tst-malloc-usable.c
index a1074b782a..e50cadcf10 100644
--- a/malloc/tst-malloc-usable.c
+++ b/malloc/tst-malloc-usable.c
@@ -21,29 +21,24 @@
 #include <malloc.h>
 #include <string.h>
 #include <stdio.h>
+#include <support/support.h>
+#include <support/check.h>
 
 static int
 do_test (void)
 {
   size_t usable_size;
   void *p = malloc (7);
-  if (!p)
-    {
-      printf ("memory allocation failed\n");
-      return 1;
-    }
 
+  TEST_VERIFY_EXIT (p != NULL);
   usable_size = malloc_usable_size (p);
-  if (usable_size != 7)
-    {
-      printf ("malloc_usable_size: expected 7 but got %zu\n", usable_size);
-      return 1;
-    }
-
+  TEST_COMPARE (usable_size, 7);
   memset (p, 0, usable_size);
   free (p);
+
+  TEST_COMPARE (malloc_usable_size (NULL), 0);
+
   return 0;
 }
 
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include "support/test-driver.c"
-- 
2.31.1

Re: [PATCH v2] Handle NULL input to malloc_usable_size [BZ #28506]

[Richard W.M. Jones]

On Fri, Oct 29, 2021 at 02:25:21PM +0530, Siddhesh Poyarekar wrote:
> Hoist the NULL check for malloc_usable_size into its entry points in
> malloc-debug and malloc and assume non-NULL in all callees.  This fixes
> BZ #28506
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> ---
>  malloc/malloc-debug.c      | 12 ++++++------
>  malloc/malloc.c            | 24 ++++++++----------------
>  malloc/tst-malloc-usable.c | 21 ++++++++-------------
>  3 files changed, 22 insertions(+), 35 deletions(-)
> 
> diff --git a/malloc/malloc-debug.c b/malloc/malloc-debug.c
> index 9922ef5f25..5e954d7dc2 100644
> --- a/malloc/malloc-debug.c
> +++ b/malloc/malloc-debug.c
> @@ -399,17 +399,17 @@ strong_alias (__debug_calloc, calloc)
>  size_t
>  malloc_usable_size (void *mem)
>  {
> +  if (mem == NULL)
> +    return 0;
> +
>    if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK))
>      return mcheck_usable_size (mem);
>    if (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))
>      return malloc_check_get_size (mem);
>  
> -  if (mem != NULL)
> -    {
> -      mchunkptr p = mem2chunk (mem);
> -     if (DUMPED_MAIN_ARENA_CHUNK (p))
> -       return chunksize (p) - SIZE_SZ;
> -    }
> +  mchunkptr p = mem2chunk (mem);
> +  if (DUMPED_MAIN_ARENA_CHUNK (p))
> +    return chunksize (p) - SIZE_SZ;
>  
>    return musable (mem);
>  }
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 2ba1fee144..a9dfc82788 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -5007,20 +5007,13 @@ __malloc_trim (size_t s)
>  static size_t
>  musable (void *mem)
>  {
> -  mchunkptr p;
> -  if (mem != 0)
> -    {
> -      size_t result = 0;
> -
> -      p = mem2chunk (mem);
> +  mchunkptr p = mem2chunk (mem);
>  
> -      if (chunk_is_mmapped (p))
> -	result = chunksize (p) - CHUNK_HDR_SZ;
> -      else if (inuse (p))
> -	result = memsize (p);
> +  if (chunk_is_mmapped (p))
> +    return chunksize (p) - CHUNK_HDR_SZ;
> +  else if (inuse (p))
> +    return memsize (p);
>  
> -      return result;
> -    }
>    return 0;
>  }
>  
> @@ -5028,10 +5021,9 @@ musable (void *mem)
>  size_t
>  __malloc_usable_size (void *m)
>  {
> -  size_t result;
> -
> -  result = musable (m);
> -  return result;
> +  if (m == NULL)
> +    return 0;
> +  return musable (m);
>  }
>  #endif
>  
> diff --git a/malloc/tst-malloc-usable.c b/malloc/tst-malloc-usable.c
> index a1074b782a..e50cadcf10 100644
> --- a/malloc/tst-malloc-usable.c
> +++ b/malloc/tst-malloc-usable.c
> @@ -21,29 +21,24 @@
>  #include <malloc.h>
>  #include <string.h>
>  #include <stdio.h>
> +#include <support/support.h>
> +#include <support/check.h>
>  
>  static int
>  do_test (void)
>  {
>    size_t usable_size;
>    void *p = malloc (7);
> -  if (!p)
> -    {
> -      printf ("memory allocation failed\n");
> -      return 1;
> -    }
>  
> +  TEST_VERIFY_EXIT (p != NULL);
>    usable_size = malloc_usable_size (p);
> -  if (usable_size != 7)
> -    {
> -      printf ("malloc_usable_size: expected 7 but got %zu\n", usable_size);
> -      return 1;
> -    }
> -
> +  TEST_COMPARE (usable_size, 7);
>    memset (p, 0, usable_size);
>    free (p);
> +
> +  TEST_COMPARE (malloc_usable_size (NULL), 0);
> +
>    return 0;
>  }
>  
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> +#include "support/test-driver.c"

Also looks sensible, so:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [PATCH v2] Handle NULL input to malloc_usable_size [BZ #28506]

[Florian Weimer]

* Siddhesh Poyarekar:

> Hoist the NULL check for malloc_usable_size into its entry points in
> malloc-debug and malloc and assume non-NULL in all callees.  This fixes
> BZ #28506
>
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

I forgot: If you use Signed-off-by: for real (without FSF copyright
assignment), you need to adjust the copyright headers.

The rest of the patch looks okay to me.

Thanks,
Florian

[COMMITTED] Handle NULL input to malloc_usable_size [BZ #28506]

[Siddhesh Poyarekar]

Hoist the NULL check for malloc_usable_size into its entry points in
malloc-debug and malloc and assume non-NULL in all callees.  This fixes
BZ #28506

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
Reviewed-by: Florian Weimer <fweimer@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
---
 malloc/malloc-debug.c      | 13 +++++++------
 malloc/malloc.c            | 25 +++++++++----------------
 malloc/tst-malloc-usable.c | 22 +++++++++-------------
 3 files changed, 25 insertions(+), 35 deletions(-)

diff --git a/malloc/malloc-debug.c b/malloc/malloc-debug.c
index 9922ef5f25..3d7e6d44fd 100644
--- a/malloc/malloc-debug.c
+++ b/malloc/malloc-debug.c
@@ -1,5 +1,6 @@
 /* Malloc debug DSO.
    Copyright (C) 2021 Free Software Foundation, Inc.
+   Copyright The GNU Toolchain Authors.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -399,17 +400,17 @@ strong_alias (__debug_calloc, calloc)
 size_t
 malloc_usable_size (void *mem)
 {
+  if (mem == NULL)
+    return 0;
+
   if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK))
     return mcheck_usable_size (mem);
   if (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))
     return malloc_check_get_size (mem);
 
-  if (mem != NULL)
-    {
-      mchunkptr p = mem2chunk (mem);
-     if (DUMPED_MAIN_ARENA_CHUNK (p))
-       return chunksize (p) - SIZE_SZ;
-    }
+  mchunkptr p = mem2chunk (mem);
+  if (DUMPED_MAIN_ARENA_CHUNK (p))
+    return chunksize (p) - SIZE_SZ;
 
   return musable (mem);
 }
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 2ba1fee144..095d97a3be 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -1,5 +1,6 @@
 /* Malloc implementation for multiple threads without lock contention.
    Copyright (C) 1996-2021 Free Software Foundation, Inc.
+   Copyright The GNU Toolchain Authors.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -5007,20 +5008,13 @@ __malloc_trim (size_t s)
 static size_t
 musable (void *mem)
 {
-  mchunkptr p;
-  if (mem != 0)
-    {
-      size_t result = 0;
-
-      p = mem2chunk (mem);
+  mchunkptr p = mem2chunk (mem);
 
-      if (chunk_is_mmapped (p))
-	result = chunksize (p) - CHUNK_HDR_SZ;
-      else if (inuse (p))
-	result = memsize (p);
+  if (chunk_is_mmapped (p))
+    return chunksize (p) - CHUNK_HDR_SZ;
+  else if (inuse (p))
+    return memsize (p);
 
-      return result;
-    }
   return 0;
 }
 
@@ -5028,10 +5022,9 @@ musable (void *mem)
 size_t
 __malloc_usable_size (void *m)
 {
-  size_t result;
-
-  result = musable (m);
-  return result;
+  if (m == NULL)
+    return 0;
+  return musable (m);
 }
 #endif
 
diff --git a/malloc/tst-malloc-usable.c b/malloc/tst-malloc-usable.c
index a1074b782a..b0d702be10 100644
--- a/malloc/tst-malloc-usable.c
+++ b/malloc/tst-malloc-usable.c
@@ -2,6 +2,7 @@
    MALLOC_CHECK_ exported to a positive value.
 
    Copyright (C) 2012-2021 Free Software Foundation, Inc.
+   Copyright The GNU Toolchain Authors.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -21,29 +22,24 @@
 #include <malloc.h>
 #include <string.h>
 #include <stdio.h>
+#include <support/support.h>
+#include <support/check.h>
 
 static int
 do_test (void)
 {
   size_t usable_size;
   void *p = malloc (7);
-  if (!p)
-    {
-      printf ("memory allocation failed\n");
-      return 1;
-    }
 
+  TEST_VERIFY_EXIT (p != NULL);
   usable_size = malloc_usable_size (p);
-  if (usable_size != 7)
-    {
-      printf ("malloc_usable_size: expected 7 but got %zu\n", usable_size);
-      return 1;
-    }
-
+  TEST_COMPARE (usable_size, 7);
   memset (p, 0, usable_size);
   free (p);
+
+  TEST_COMPARE (malloc_usable_size (NULL), 0);
+
   return 0;
 }
 
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include "support/test-driver.c"
-- 
2.31.1

Re: [COMMITTED] Handle NULL input to malloc_usable_size [BZ #28506]

[Florian Weimer]

* Siddhesh Poyarekar:

> Hoist the NULL check for malloc_usable_size into its entry points in
> malloc-debug and malloc and assume non-NULL in all callees.  This fixes
> BZ #28506
>
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> Reviewed-by: Florian Weimer <fweimer@redhat.com>
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Looks good, thanks.

Florian