diff mbox series

[4/6] malloc: Improve free checks

Message ID PAWPR08MB89822D5B07AFE56F4E8C356E83AD2@PAWPR08MB8982.eurprd08.prod.outlook.com (mailing list archive)
State Accepted
Delegated to: Adhemerval Zanella Netto
Headers
Series [1/6] malloc: Move mmap code out of __libc_free hotpath |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed

Commit Message

Wilco Dijkstra March 31, 2025, 12:59 p.m. UTC 
  The checks on size can be merged and use __builtin_add_overflow.  Since
tcache only handles small sizes (and rejects sizes < MINSIZE), delay this
check until after tcache.

---

Comments

Adhemerval Zanella Netto April 14, 2025, 1:37 p.m. UTC | #1 
On 31/03/25 09:59, Wilco Dijkstra wrote:
> 
> The checks on size can be merged and use __builtin_add_overflow.  Since
> tcache only handles small sizes (and rejects sizes < MINSIZE), delay this
> check until after tcache.

LGTM, I see that you did change to use the __glibc_unlikely.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> 
> ---
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 24a2d50ec845d379e7eae0a2360cb669a06504ea..f8f35b337d91a0a4790d27163728fec53083f026 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3465,16 +3465,8 @@ __libc_free (void *mem)
>  
>    INTERNAL_SIZE_T size = chunksize (p);
>  
> -  /* Little security check which won't hurt performance: the
> -     allocator never wraps around at the end of the address space.
> -     Therefore we can exclude some size values which might appear
> -     here by accident or by "design" from some intruder.  */
> -  if (__builtin_expect ((uintptr_t) p > (uintptr_t) -size, 0)
> -      || __builtin_expect (misaligned_chunk (p), 0))
> +  if (__glibc_unlikely (misaligned_chunk (p)))
>      malloc_printerr ("free(): invalid pointer");
> -  /* We know that each chunk is at least MINSIZE bytes.  */
> -  if (__glibc_unlikely (size < MINSIZE))
> -    malloc_printerr ("free(): invalid size");
>  
>    check_inuse_chunk (arena_for_chunk (p), p);
>  
> @@ -3483,6 +3475,11 @@ __libc_free (void *mem)
>      return;
>  #endif
>  
> +  /* Check size >= MINSIZE and p + size does not overflow.  */
> +  if (__glibc_unlikely (__builtin_add_overflow_p ((uintptr_t) p, size - MINSIZE,
> +						  (uintptr_t) 0)))
> +    malloc_printerr ("free(): invalid size");
> +
>    _int_free_chunk (arena_for_chunk (p), p, size, 0);
>  }
>  libc_hidden_def (__libc_free)
>
diff mbox series

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 24a2d50ec845d379e7eae0a2360cb669a06504ea..f8f35b337d91a0a4790d27163728fec53083f026 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3465,16 +3465,8 @@  __libc_free (void *mem)
 
   INTERNAL_SIZE_T size = chunksize (p);
 
-  /* Little security check which won't hurt performance: the
-     allocator never wraps around at the end of the address space.
-     Therefore we can exclude some size values which might appear
-     here by accident or by "design" from some intruder.  */
-  if (__builtin_expect ((uintptr_t) p > (uintptr_t) -size, 0)
-      || __builtin_expect (misaligned_chunk (p), 0))
+  if (__glibc_unlikely (misaligned_chunk (p)))
     malloc_printerr ("free(): invalid pointer");
-  /* We know that each chunk is at least MINSIZE bytes.  */
-  if (__glibc_unlikely (size < MINSIZE))
-    malloc_printerr ("free(): invalid size");
 
   check_inuse_chunk (arena_for_chunk (p), p);
 
@@ -3483,6 +3475,11 @@  __libc_free (void *mem)
     return;
 #endif
 
+  /* Check size >= MINSIZE and p + size does not overflow.  */
+  if (__glibc_unlikely (__builtin_add_overflow_p ((uintptr_t) p, size - MINSIZE,
+						  (uintptr_t) 0)))
+    malloc_printerr ("free(): invalid size");
+
   _int_free_chunk (arena_for_chunk (p), p, size, 0);
 }
 libc_hidden_def (__libc_free)