[RFC] Supporting malloc_usable_size

[Siddhesh Poyarekar]

Hello,

This is in context of this systemd issue:

https://github.com/systemd/systemd/issues/22801

through which I had discovered that systemd was (ab)using
malloc_usable_size to use spare space in an allocated object.  This was
discovered when _FORTIFY_SOURCE=3 flagged this as a buffer overflow,
since the compiler is unable to see that the space beyond the allocation
was safe to use.

At that time, I thought this use was to avoid reallocating[1] when
there's enough space in the usable size and that is not entirely false.
I had then proposed during my Cauldron talk (and in discussions with
some of you) that maybe providing a fast path for cases where the new
size was within the current chunk size was the solution to the problem
systemd was trying to solve with malloc_usable_size.

However I was wrong, it looks like the malloc_usable_size usage goes far
beyond simply optimizing allocation, it is used as a way to query sizes
of objects and hence eliminating the need to pass their corresponding
sizes.  This is probably suboptimal (given that there's a call into
libc.so at every point instead of what could have been just an
additional parameter alongside the object) but it's the design pattern
they've chosen.

I have an idea to support this abuse and at the same time, satisfy the
compiler by giving it a hint of the new size.  This could be done within
systemd by calling a dummy allocator (e.g. expand_to_usable) with the
result of malloc_usable_size(), thus telling the compiler that the
object has been expanded to the usable size, thus making accesses into
the spare space well defined.  I have a patch too (see below) to systemd
that demonstrates how this could be achieved, it's been tested with gcc
as well as clang and with _FORTIFY_SOURCE=3.

The larger consequence of this patch though is that we further support
the usage of malloc_usable_size for cases beyond diagnostics.  Do we
want to do that?  If we do, should we also then make clear what kind of
usage we support as a library, say, in the manual?  We don't have a
manual blurb for malloc_usable_size, so maybe this is a good opportunity
to do that.

Any other thoughts on malloc_usable_size?

Thanks,
Sid


[1] https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-level#the_gains_of_improved_security_coverage_outweigh_the_cost

---
 src/basic/alloc-util.c     |  5 +++++
 src/basic/alloc-util.h     | 16 ++++++++++++++--
 src/test/test-alloc-util.c |  5 +++--
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/src/basic/alloc-util.c b/src/basic/alloc-util.c
index b030f454b2..58cf7c7433 100644
--- a/src/basic/alloc-util.c
+++ b/src/basic/alloc-util.c
@@ -102,3 +102,8 @@ void* greedy_realloc0(
 
         return q;
 }
+
+void *expand_to_usable (void *ptr, size_t newsize _unused_)
+{
+        return ptr;
+}
diff --git a/src/basic/alloc-util.h b/src/basic/alloc-util.h
index b38db7d473..0c0b4851f5 100644
--- a/src/basic/alloc-util.h
+++ b/src/basic/alloc-util.h
@@ -4,6 +4,7 @@
 #include <alloca.h>
 #include <stddef.h>
 #include <stdlib.h>
+#include <malloc.h>
 #include <string.h>
 
 #include "macro.h"
@@ -184,6 +185,18 @@ void* greedy_realloc0(void **p, size_t need, size_t size);
 #  define msan_unpoison(r, s)
 #endif
 
+void *expand_to_usable (void *ptr, size_t newsize) _alloc_ (2);
+
+static inline size_t
+_malloc_usable_size(void **ptr)
+{
+        if (*ptr == NULL)
+                return 0;
+        size_t full_size = malloc_usable_size (*ptr);
+        *ptr = expand_to_usable (*ptr, full_size);
+        return full_size;
+}
+
 /* This returns the number of usable bytes in a malloc()ed region as per malloc_usable_size(), in a way that
  * is compatible with _FORTIFY_SOURCES. If _FORTIFY_SOURCES is used many memory operations will take the
  * object size as returned by __builtin_object_size() into account. Hence, let's return the smaller size of
@@ -193,8 +206,7 @@ void* greedy_realloc0(void **p, size_t need, size_t size);
  * too. Moreover, when NULL is passed malloc_usable_size() is documented to return zero, and
  * __builtin_object_size() returns SIZE_MAX too, hence we also return a sensible value of 0 in this corner
  * case. */
-#define MALLOC_SIZEOF_SAFE(x) \
-        MIN(malloc_usable_size(x), __builtin_object_size(x, 0))
+#define MALLOC_SIZEOF_SAFE(x) _malloc_usable_size((void **) &(x))
 
 /* Inspired by ELEMENTSOF() but operates on malloc()'ed memory areas: typesafely returns the number of items
  * that fit into the specified memory block */
diff --git a/src/test/test-alloc-util.c b/src/test/test-alloc-util.c
index df6139005f..f9fd192576 100644
--- a/src/test/test-alloc-util.c
+++ b/src/test/test-alloc-util.c
@@ -170,8 +170,9 @@ TEST(malloc_size_safe) {
         size_t n = 4711;
 
         /* Let's check the macros and built-ins work on NULL and return the expected values */
-        assert_se(MALLOC_ELEMENTSOF((float*) NULL) == 0);
-        assert_se(MALLOC_SIZEOF_SAFE((float*) NULL) == 0);
+        float *nullptr = NULL;
+        assert_se(MALLOC_ELEMENTSOF(nullptr) == 0);
+        assert_se(MALLOC_SIZEOF_SAFE(nullptr) == 0);
         assert_se(malloc_usable_size(NULL) == 0); /* as per man page, this is safe and defined */
         assert_se(__builtin_object_size(NULL, 0) == SIZE_MAX); /* as per docs SIZE_MAX is returned for pointers where the size isn't known */
 
-- 
2.38.1