Skip to content

Commit

Permalink
Use dummy allocator to make accesses defined as per standard
Browse files Browse the repository at this point in the history
systemd uses malloc_usable_size() everywhere to use memory blocks
obtained through malloc, but that is abuse since the
malloc_usable_size() interface isn't meant for this kind of use, it is
for diagnostics only.  This is also why systemd behaviour is flaky when
built with _FORTIFY_SOURCE.

One way to make this more standard (and hence safer) is to, at every
malloc_usable_size() call, also 'reallocate' the block so that the
compiler can see the larger size.  This is done through a dummy
reallocator whose only purpose is to tell the compiler about the larger
usable size, it doesn't do any actual reallocation.

Florian Weimer pointed out that this doesn't solve the problem of an
allocator potentially growing usable size at will, which will break the
implicit assumption in systemd use that the value returned remains
constant as long as the object is valid.  The safest way to fix that is
for systemd to step away from using malloc_usable_size() like this.

Resolves #22801.
  • Loading branch information
siddhesh authored and poettering committed Dec 14, 2022
1 parent cc137d5 commit 7929e18
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 10 deletions.
4 changes: 4 additions & 0 deletions src/basic/alloc-util.c
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,7 @@ void* greedy_realloc0(

return q;
}

void *expand_to_usable(void *ptr, size_t newsize _unused_) {
return ptr;
}
38 changes: 28 additions & 10 deletions src/basic/alloc-util.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#pragma once

#include <alloca.h>
#include <malloc.h>
#include <stddef.h>
#include <stdlib.h>
#include <string.h>
Expand Down Expand Up @@ -184,17 +185,34 @@ void* greedy_realloc0(void **p, size_t need, size_t size);
# define msan_unpoison(r, s)
#endif

/* 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
* malloc_usable_size() and __builtin_object_size() here, so that we definitely operate in safe territory by
* both the compiler's and libc's standards. Note that __builtin_object_size() evaluates to SIZE_MAX if the
* size cannot be determined, hence the MIN() expression should be safe with dynamically sized memory,
* 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. */
/* Dummy allocator to tell the compiler that the new size of p is newsize. The implementation returns the
* pointer as is; the only reason for its existence is as a conduit for the _alloc_ attribute. This cannot be
* a static inline because gcc then loses the attributes on the function.
* See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503 */
void *expand_to_usable(void *p, size_t newsize) _alloc_(2) _returns_nonnull_;

static inline size_t malloc_sizeof_safe(void **xp) {
if (_unlikely_(!xp || !*xp))
return 0;

size_t sz = malloc_usable_size(*xp);
*xp = expand_to_usable(*xp, sz);
/* GCC doesn't see the _returns_nonnull_ when built with ubsan, so yet another hint to make it doubly
* clear that expand_to_usable won't return NULL.
* See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79265 */
if (!*xp)
assert_not_reached();
return sz;
}

/* This returns the number of usable bytes in a malloc()ed region as per malloc_usable_size(), which may
* return a value larger than the size that was actually allocated. Access to that additional memory is
* discouraged because it violates the C standard; a compiler cannot see that this as valid. To help the
* compiler out, the MALLOC_SIZEOF_SAFE macro 'allocates' the usable size using a dummy allocator function
* expand_to_usable. There is a possibility of malloc_usable_size() returning different values during the
* lifetime of an object, which may cause problems, but the glibc allocator does not do that at the moment. */
#define MALLOC_SIZEOF_SAFE(x) \
MIN(malloc_usable_size(x), __builtin_object_size(x, 0))
malloc_sizeof_safe((void**) &__builtin_choose_expr(__builtin_constant_p(x), (void*) { NULL }, (x)))

/* Inspired by ELEMENTSOF() but operates on malloc()'ed memory areas: typesafely returns the number of items
* that fit into the specified memory block */
Expand Down

0 comments on commit 7929e18

Please sign in to comment.