[PATCH] malloc: skip allocation retry when encountering faulty requests (bug 29709)

[PATCH] malloc: skip allocation retry when encountering faulty requests (bug 29709)

[David Milo]

[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]

Steps to reproduce the (bug 29709):

    1. build glibc without tcache (--disable-experimental-malloc)
    2. create a multithreaded process
    3. make an invalid request (> PTRDIFF_MAX) via malloc/memalign

Step 3 then results in a second allocation attempt with a different arena. This
behavior does not make much sense, since retrying with another arena wont fix
the real issue - the invalid request size. Read (bug xx) for more information.

This patch should fix the issue. I see two possible solutions for this problem:

    1. let _int_malloc/_int_memalign set errno to EINVAL instead of ENOMEM
       when encountering a faulty request size. This way, we can check
       errno before starting a second allocation attempt. If errno is
       set to EINVAL, we know that there is an issue with the argument
       itself and a second attempt would not behave differently, even
       if we would switch arenas.

    2. add an extra condition for the second allocation attempt. Only
       start a second attempt if following conditions are met

         - returned pointer is NULL
         - requested size is less or equals PTRDIFF_MAX
         - current arena pointer is not NULL

In my opinion solution 1 is the better choice. It would make more sense
if we set errno to EINVAL instead of ENOMEM whenever we deal with
faulty requests, since EINVAL indicates that invalid arguments were
passed to library functions.

Nevertheless, I decided to go with solution 2 in order to avoid
bigger changes in exposed library functions (malloc/memalign
would then also be able to set EINVAL instead of only ENOMEM).

Signed-off-by: David Milosevic <milod2048@gmail.com>

[-- Attachment #2: bug29709.patch --]
[-- Type: application/octet-stream, Size: 3027 bytes --]

From e938867c3c3371524c3e8008169ba4483bc29364 Mon Sep 17 00:00:00 2001
From: David Milosevic <milod2048@gmail.com>
Date: Thu, 20 Oct 2022 19:20:39 +0200
Subject: [PATCH] malloc: skip allocation retry when encountering faulty requests (bug 29709)
To: libc-alpha@sourceware.org

Steps to reproduce the (bug 29709):

    1. build glibc without tcache (--disable-experimental-malloc)
    2. create a multithreaded process
    3. make an invalid request (> PTRDIFF_MAX) via malloc/memalign

Step 3 then results in a second allocation attempt with a different arena. This
behavior does not make much sense, since retrying with another arena wont fix
the real issue - the invalid request size. Read (bug xx) for more information.

This patch should fix the issue. I see two possible solutions for this problem:

    1. let _int_malloc/_int_memalign set errno to EINVAL instead of ENOMEM
       when encountering a faulty request size. This way, we can check
       errno before starting a second allocation attempt. If errno is
       set to EINVAL, we know that there is an issue with the argument
       itself and a second attempt would not behave differently, even
       if we would switch arenas.

    2. add an extra condition for the second allocation attempt. Only
       start a second attempt if following conditions are met

         - returned pointer is NULL
         - requested size is less or equals PTRDIFF_MAX
         - current arena pointer is not NULL

In my opinion solution 1 is the better choice. It would make more sense
if we set errno to EINVAL instead of ENOMEM whenever we deal with
faulty requests, since EINVAL indicates that invalid arguments were
passed to library functions.

Nevertheless, I decided to go with solution 2 in order to avoid
bigger changes in exposed library functions (malloc/memalign
would then also be able to set EINVAL instead of only ENOMEM).

Signed-off-by: David Milosevic <milod2048@gmail.com>
---
 malloc/malloc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 2a61c8b5ee..b91a82558e 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3299,11 +3299,12 @@ __libc_malloc (size_t bytes)
     }
 
   arena_get (ar_ptr, bytes);
-
   victim = _int_malloc (ar_ptr, bytes);
+
   /* Retry with another arena only if we were able to find a usable arena
      before.  */
-  if (!victim && ar_ptr != NULL)
+
+  if ((victim == NULL) && (bytes <= PTRDIFF_MAX) && (ar_ptr != NULL))
     {
       LIBC_PROBE (memory_malloc_retry, 1, bytes);
       ar_ptr = arena_get_retry (ar_ptr, bytes);
@@ -3541,9 +3542,9 @@ _mid_memalign (size_t alignment, size_t bytes, void *address)
     }
 
   arena_get (ar_ptr, bytes + alignment + MINSIZE);
-
   p = _int_memalign (ar_ptr, alignment, bytes);
-  if (!p && ar_ptr != NULL)
+
+  if ((p == NULL) && (bytes <= PTRDIFF_MAX) && (ar_ptr != NULL))
     {
       LIBC_PROBE (memory_memalign_retry, 2, bytes, alignment);
       ar_ptr = arena_get_retry (ar_ptr, bytes);
-- 
2.37.3