[PATCH] Allow direct access to dlmalloc

[Yaakov Selkowitz]

This allows programs to override malloc/free/etc. while still being able
to access the internally provided implementation.

* common.din (__libc_calloc): Export.
(__libc_free): Export.
(__libc_mallinfo): Export.
(__libc_malloc): Export.
(__libc_mallopt): Export.
(__libc_memalign): Export.
(__libc_realloc): Export.
(__libc_valloc): Export.
* malloc_wrapper.cc (__libc_free, free): Split out builtin dlmalloc
implementation into separate function.
(__libc_malloc, malloc): Ditto.
(__libc_realloc, realloc): Ditto.
(__libc_calloc, calloc): Ditto.
(__libc_memalign, posix_memalign, memalign): Ditto.
(__libc_valloc, valloc): Ditto.
(__libc_mallopt, mallopt): Ditto.
(__libc_mallinfo, mallinfo): Ditto.
---
This came to my attention due to:

https://git.gnome.org/browse/gdk-pixbuf/commit/?id=b07c3bf

Although this will likely be changed due to it being unable to link
on anything not using glibc.

The one question I have about this patch is that I see in a couple other
places that malloc-overriding code tries to override the __libc_* variants
as well on Linux (where they are just weak aliases to the main functions).
This particular method wouldn't allow that, but I'm not sure if we want or
need to allow that either.

winsup/cygwin/common.din | 8 +++
winsup/cygwin/malloc_wrapper.cc | 129 ++++++++++++++++++++++++++--------------
2 files changed, 94 insertions(+), 43 deletions(-)

diff --git a/winsup/cygwin/common.din b/winsup/cygwin/common.din
index 71a0c9b..3381c61 100644
--- a/winsup/cygwin/common.din
+++ b/winsup/cygwin/common.din
@@ -67,6 +67,14 @@ __isinfd NOSIGFE
__isinff NOSIGFE
__isnand NOSIGFE
__isnanf NOSIGFE
+__libc_calloc SIGFE
+__libc_free SIGFE
+__libc_mallinfo SIGFE
+__libc_malloc SIGFE
+__libc_mallopt SIGFE
+__libc_memalign SIGFE
+__libc_realloc SIGFE
+__libc_valloc SIGFE
__locale_mb_cur_max NOSIGFE
__main NOSIGFE
__mempcpy = mempcpy NOSIGFE
diff --git a/winsup/cygwin/malloc_wrapper.cc b/winsup/cygwin/malloc_wrapper.cc
index 43b8144..1375a46 100644
--- a/winsup/cygwin/malloc_wrapper.cc
+++ b/winsup/cygwin/malloc_wrapper.cc
@@ -36,17 +36,31 @@ static bool internal_malloc_determined;
doesn't provide its own malloc. */

extern "C" void
+__libc_free (void *p)
+{
+ __malloc_lock ();
+ dlfree (p);
+ __malloc_unlock ();
+}
+
+extern "C" void
free (void *p)
{
malloc_printf ("(%p), called by %p", p, caller_return_address ());
if (!use_internal)
user_data->free (p);
else
- {
- __malloc_lock ();
- dlfree (p);
- __malloc_unlock ();
- }
+ __libc_free (p);
+}
+
+extern "C" void *
+__libc_malloc (size_t size)
+{
+ void *res;
+ __malloc_lock ();
+ res = dlmalloc (size);
+ __malloc_unlock ();
+ return res;
}

extern "C" void *
@@ -56,28 +70,30 @@ malloc (size_t size)
if (!use_internal)
res = user_data->malloc (size);
else
- {
- __malloc_lock ();
- res = dlmalloc (size);
- __malloc_unlock ();
- }
+ res = __libc_malloc (size);
malloc_printf ("(%ld) = %p, called by %p", size, res,
caller_return_address ());
return res;
}

extern "C" void *
+__libc_realloc (void *p, size_t size)
+{
+ void *res;
+ __malloc_lock ();
+ res = dlrealloc (p, size);
+ __malloc_unlock ();
+ return res;
+}
+
+extern "C" void *
realloc (void *p, size_t size)
{
void *res;
if (!use_internal)
res = user_data->realloc (p, size);
else
- {
- __malloc_lock ();
- res = dlrealloc (p, size);
- __malloc_unlock ();
- }
+ res = __libc_realloc (p, size);
malloc_printf ("(%p, %ld) = %p, called by %p", p, size, res,
caller_return_address ());
return res;
@@ -95,22 +111,38 @@ reallocf (void *p, size_t size)
}

extern "C" void *
+__libc_calloc (size_t nmemb, size_t size)
+{
+ void *res;
+ __malloc_lock ();
+ res = dlcalloc (nmemb, size);
+ __malloc_unlock ();
+ return res;
+}
+
+extern "C" void *
calloc (size_t nmemb, size_t size)
{
void *res;
if (!use_internal)
res = user_data->calloc (nmemb, size);
else
- {
- __malloc_lock ();
- res = dlcalloc (nmemb, size);
- __malloc_unlock ();
- }
+ res = __libc_calloc (nmemb, size);
malloc_printf ("(%ld, %ld) = %p, called by %p", nmemb, size, res,
caller_return_address ());
return res;
}

+extern "C" void *
+__libc_memalign (size_t alignment, size_t bytes)
+{
+ void *res;
+ __malloc_lock ();
+ res = dlmemalign (alignment, bytes);
+ __malloc_unlock ();
+ return res;
+}
+
extern "C" int
posix_memalign (void **memptr, size_t alignment, size_t bytes)
{
@@ -121,9 +153,7 @@ posix_memalign (void **memptr, size_t alignment, size_t bytes)
return user_data->posix_memalign (memptr, alignment, bytes);
if ((alignment & (alignment - 1)) != 0)
return EINVAL;
- __malloc_lock ();
- res = dlmemalign (alignment, bytes);
- __malloc_unlock ();
+ res = __libc_memalign (alignment, bytes);
if (!res)
return ENOMEM;
if (memptr)
@@ -141,12 +171,17 @@ memalign (size_t alignment, size_t bytes)
res = NULL;
}
else
- {
- __malloc_lock ();
- res = dlmemalign (alignment, bytes);
- __malloc_unlock ();
- }
+ res = __libc_memalign (alignment, bytes);
+ return res;
+}

+extern "C" void *
+__libc_valloc (size_t bytes)
+{
+ void *res;
+ __malloc_lock ();
+ res = dlvalloc (bytes);
+ __malloc_unlock ();
return res;
}

@@ -160,11 +195,7 @@ valloc (size_t bytes)
res = NULL;
}
else
- {
- __malloc_lock ();
- res = dlvalloc (bytes);
- __malloc_unlock ();
- }
+ res = __libc_valloc (bytes);

return res;
}
@@ -208,6 +239,16 @@ malloc_trim (size_t pad)
}

extern "C" int
+__libc_mallopt (int p, int v)
+{
+ int res;
+ __malloc_lock ();
+ res = dlmallopt (p, v);
+ __malloc_unlock ();
+ return res;
+}
+
+extern "C" int
mallopt (int p, int v)
{
int res;
@@ -217,11 +258,7 @@ mallopt (int p, int v)
res = 0;
}
else
- {
- __malloc_lock ();
- res = dlmallopt (p, v);
- __malloc_unlock ();
- }
+ res = __libc_mallopt (p, v);

return res;
}
@@ -240,6 +277,16 @@ malloc_stats ()
}

extern "C" struct mallinfo
+__libc_mallinfo ()
+{
+ struct mallinfo m;
+ __malloc_lock ();
+ m = dlmallinfo ();
+ __malloc_unlock ();
+ return m;
+}
+
+extern "C" struct mallinfo
mallinfo ()
{
struct mallinfo m;
@@ -249,11 +296,7 @@ mallinfo ()
set_errno (ENOSYS);
}
else
- {
- __malloc_lock ();
- m = dlmallinfo ();
- __malloc_unlock ();
- }
+ m = __libc_mallinfo ();

return m;
}

[Corinna Vinschen]

Hi Yaakov,

Post by Yaakov Selkowitz
This allows programs to override malloc/free/etc. while still being able
to access the internally provided implementation.
* common.din (__libc_calloc): Export.
(__libc_free): Export.
(__libc_mallinfo): Export.
(__libc_malloc): Export.
(__libc_mallopt): Export.
(__libc_memalign): Export.
(__libc_realloc): Export.
(__libc_valloc): Export.
* malloc_wrapper.cc (__libc_free, free): Split out builtin dlmalloc
implementation into separate function.
(__libc_malloc, malloc): Ditto.
(__libc_realloc, realloc): Ditto.
(__libc_calloc, calloc): Ditto.
(__libc_memalign, posix_memalign, memalign): Ditto.
(__libc_valloc, valloc): Ditto.
(__libc_mallopt, mallopt): Ditto.
(__libc_mallinfo, mallinfo): Ditto.
---
https://git.gnome.org/browse/gdk-pixbuf/commit/?id=b07c3bf
Although this will likely be changed due to it being unable to link
on anything not using glibc.

Right. The patch looks ok, but do we really want to go there? These
__libc_* symbols are very much an internal and (probably?) undocumented
implementation detail of glibc. The aformentioned case is not even
production code but just a test and even that is eventually going to be
changed for portability reasons as you pointed out.

Unfortunately it seems not possible to make the __libc_* symbols simply
a set of aliases for existing functions. So, is it really worth the
additional function call per each malloc? Granted, you'll probably only
notice the slowdown on 32 bit, but still...?

Post by Yaakov Selkowitz
The one question I have about this patch is that I see in a couple other
places that malloc-overriding code tries to override the __libc_* variants
as well on Linux (where they are just weak aliases to the main functions).
This particular method wouldn't allow that, but I'm not sure if we want or
need to allow that either.

You'd need another set of malloc/calloc/realloc/free pointers in struct
per_process. We still have room for 9 pointers in per_process without
having to resize it. Doable, yes. Worth it? I'm not so sure.


Corinna