Fix memory hook installation #1269

Baraa-Hasheesh · 2025-04-28T08:48:41Z

Description

async-profiler patches dynamic references to memory allocation calls to monitor these calls to help detect possible memory leaks.
However it's possible to create custom memory allocation functions to replace the existing ones.

#include <dlfcn.h>
#include "stdlib.h"
#include "string.h"

void* malloc(size_t size) {
    void* ptr = calloc(1, size);
    memset(ptr, 0xFF, size);
    return ptr;
}

If this wrapper is defined within the scope of the main executable no problems will occur as the async-profiler only patches dynamic references as such the malloc won't be patched while the internal calloc would be patched, so everything will work as expected.

However if the above wrapper was preloaded into the executable via LD_PRELOAD then the dynamic reference of the main will originally resolve to the custom malloc, however when async-profiler is started it will resolve the malloc via a dlsym call which returns the original reference of malloc rather than the custom declaration.

This in turn means the async profiler will potentially change the behaviour (which methods are called) of a running program if it's used to memory profile the executable which is risky & could potentially lead to crashes & undefined behaviour.

Related issues

#1266

Motivation and context

Avoid changing the behaviour of running processes when attaching the profiler to them.

How has this been tested?

make test
=> A new test case has been added to check possible combination of the async-profiler loading with the library

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

apangin · 2025-04-28T13:25:14Z

test/native/libs/mallocwrapper.c

+     memset(ptr, 0xFF, size);
+     return ptr;
+ }
+


There should be no empty lines in the end, but the last line should terminate with EOL character (check other files too).

handled here 22e8737

apangin · 2025-04-28T13:28:20Z

test/test/nativemem/malloc_wrapper.c

+        // On Linux ARMx64 the char is dereferenced differently (-1 = 0xFF = 255 unsigned)
+        if (*ptr != -1 && *ptr != 255) {


Use unsigned char then.

handled here 421d617

apangin · 2025-04-28T13:29:50Z

src/mallocTracer.cpp

        CodeCache* cc = (*native_libs)[_patched_libs++];

+        if (cc->contains((const void*)MallocTracer::initialize)) {
+            // Let libasyncProfiler always use original allocation methods


Async-profiler will not be able to trace its own allocations. That's unfortunate, although acceptable.

apangin · 2025-04-28T13:32:27Z

src/mallocTracer.cpp

-
 extern "C" void* malloc_hook(size_t size) {
-    void* ret = _orig_malloc(size);
+    void* ret = malloc(size);


Have you tested your solution on Alpine Linux and on macOS?

The fix is working on alpine, however there are some problems in the created test
I will update the test to work on alpine

For MacOs it's not relevant as nativememory profiling isn't supported on MacOs

tested & fixed here 22e8737

Using calloc inside malloc was causing a stack overflow in alpine as calloc internally call malloc :)
(malloc -> calloc -> malloc -> calloc .....)

nativememory profiling isn't supported on MacOs

It is supported to some extent. At least, allocations in JDK should be handled correctly.
My ask here is to verify that it did not get worse with this PR.

I will need to double check on this

@apangin I've confirmed that the java libs are still being patched

Just as a note here, the patching logic is still the same as before, the only real change we created is that we used to call malloc using it's evaluated address via dlsym & now we just let the compiler & linker do that for us which less error prone

Baraa-Hasheesh · 2025-04-28T16:06:35Z

@apangin code updated & tests pass locally on alpine

apangin · 2025-04-28T16:31:18Z

src/mallocTracer.cpp

-
 extern "C" void* malloc_hook(size_t size) {
-    void* ret = _orig_malloc(size);
+    void* ret = malloc(size);


nativememory profiling isn't supported on MacOs

It is supported to some extent. At least, allocations in JDK should be handled correctly.
My ask here is to verify that it did not get worse with this PR.

apangin · 2025-04-28T16:33:39Z

test/native/libs/mallocwrapper.c

+__attribute__((visibility("default")))
+void* malloc(size_t size) {
+    if (_orig_malloc == NULL) {
+        _orig_malloc = (malloc_t) dlsym(((void *) -1l), "malloc");


What is (void *) -1l? Why not a named constant?

It's the value of RLTD_NEXT

The compile command used to compile the test isn't making the definitions needed to bundle it

I will check & update the compile command as needed

Yes, _GNU_SOURCE must be defined to use this constant.

solved here fb003af

apangin · 2025-04-29T12:28:42Z

test/test/nativemem/malloc_wrapper.c

+    unsigned char* ptr = (unsigned char*)malloc(1999993);
+
+    for (int i = 0; i < 1999993; i++) {
+        if (*ptr != 255) {


You check the same byte every time.

There is also a slight inconsistency between what you set (0xFF) and what you test (255). Try keeping the same style (async-profiler typically uses lowercase for hex numbers).

updated f270377

Thanks for the catch about pointer value check :)

apangin · 2025-04-29T12:36:35Z

test/test/nativemem/malloc_wrapper.c

+    initAsyncProfiler();
+
+    char start_cmd[2048] = {0};
+    snprintf(start_cmd, sizeof(start_cmd), "start,nativemem,cstack=dwarf,file=%s", filename);


Is cstack=dwarf essential here? Async-profiler does not supported it on all platforms (e.g. PPC64), so it's preferable not to use this option unnecessarily in order to avoid excluding test on these platforms.

remvoed f270377

apangin · 2025-04-29T12:41:15Z

test/test/nativemem/malloc_wrapper.c

@@ -0,0 +1,95 @@
+/*


Two similarly named files: malloc_wrapper.c and mallocwrapper.c
This is confusing, not to say that "wrapper" is a weasel word.

renamed here f270377

apangin · 2025-04-29T14:47:45Z

Makefile

 	$(CC) -shared -fPIC -o $(TEST_LIB_DIR)/libreladyn.$(SOEXT) test/native/libs/reladyn.c
 	$(CC) -shared -fPIC -o $(TEST_LIB_DIR)/libcallsmalloc.$(SOEXT) test/native/libs/callsmalloc.c
 	$(CC) -shared -fPIC $(INCLUDES) -Isrc -o $(TEST_LIB_DIR)/libjnimalloc.$(SOEXT) test/native/libs/jnimalloc.c
+	$(CXX) -shared -fPIC -o $(TEST_LIB_DIR)/libmalloc.$(SOEXT) test/native/libs/malloc.cpp


This simple file does not use any of C++ features, let's compile it as plain C.
To make RTLD_NEXT declaration available, simply add #define _GNU_SOURCE before includes.

applied c8f2638

apangin · 2025-04-29T14:50:24Z

test/test/nativemem/NativememTests.java

+    @Test(os = Os.LINUX, sh = "LD_PRELOAD=\"%testlib/libmalloc.so %lib\" ASPROF_COMMAND=start,nativemem,file=%f.jfr %testbin/malloc_reference preload %f.jfr",
+            output = true, env = {"LD_LIBRARY_PATH=build/lib"}, nameSuffix="LD_PRELOAD+profiler_second")
+    @Test(os = Os.LINUX, sh = "LD_PRELOAD=%testlib/libmalloc.so %testbin/malloc_reference api %f.jfr", output = true, env = {"LD_LIBRARY_PATH=build/lib"}, nameSuffix="api_test")
+    public void mallocWrapper(TestProcess p) throws Exception {


Let's call it preloadMalloc as it will better reflect the essence of the test. Similarly, malloc_reference.c can be renamed to preload_malloc.c

applied c8f2638

apangin · 2025-04-29T14:53:22Z

test/test/nativemem/NativememTests.java

    }
+
+    @Test(os = Os.LINUX, sh = "LD_PRELOAD=\"%lib %testlib/libmalloc.so\" ASPROF_COMMAND=start,nativemem,file=%f.jfr %testbin/malloc_reference preload %f.jfr",
+            output = true, env = {"LD_LIBRARY_PATH=build/lib"}, nameSuffix="LD_PRELOAD+profiler_first")


output = true is typically set when a test consumes output of the target program, which does not seem to be the case here.

removed c8f2638

apangin · 2025-04-29T14:54:11Z

test/test/nativemem/NativememTests.java

+    @Test(os = Os.LINUX, sh = "LD_PRELOAD=\"%lib %testlib/libmalloc.so\" ASPROF_COMMAND=start,nativemem,file=%f.jfr %testbin/malloc_reference preload %f.jfr",
+            output = true, env = {"LD_LIBRARY_PATH=build/lib"}, nameSuffix="LD_PRELOAD+profiler_first")
+    @Test(os = Os.LINUX, sh = "LD_PRELOAD=\"%testlib/libmalloc.so %lib\" ASPROF_COMMAND=start,nativemem,file=%f.jfr %testbin/malloc_reference preload %f.jfr",
+            output = true, env = {"LD_LIBRARY_PATH=build/lib"}, nameSuffix="LD_PRELOAD+profiler_second")


Spaces are missing: nameSuffix_=_

applied c8f2638

apangin · 2025-04-29T14:58:13Z

test/test/nativemem/malloc_reference.c

+
+void preloadOrderTest() {
+    unsigned char* ptr = (unsigned char*)malloc(1999993);
+    int size = 1999993 / sizeof(unsigned char);


Please don't. Size of unsigned char is known to be 1.

applied c8f2638

apangin · 2025-04-29T16:18:42Z

Thanks! Merged.

Baraa-Hasheesh added 2 commits April 28, 2025 08:32

async-profiler#1266: Skip patching memory allocation for the profiler

67525c8

cleanup & include test

8a767f2

Baraa-Hasheesh marked this pull request as draft April 28, 2025 08:54

Baraa-Hasheesh added 2 commits April 28, 2025 08:58

Fix ARMx64 linux failure

1fe1cf1

fix doc & condition

c11c87a

Baraa-Hasheesh force-pushed the fix-memory-hook-installation branch from 9b4545d to c11c87a Compare April 28, 2025 09:10

Baraa-Hasheesh marked this pull request as ready for review April 28, 2025 09:15

apangin reviewed Apr 28, 2025

View reviewed changes

Baraa-Hasheesh added 2 commits April 28, 2025 15:48

Fix test for Alpine

22e8737

use unsigned char

421d617

apangin reviewed Apr 28, 2025

View reviewed changes

Apply changes to use RLTD_NEXT constant

fb003af

apangin reviewed Apr 29, 2025

View reviewed changes

Apply feedback

f270377

apangin reviewed Apr 29, 2025

View reviewed changes

Apply feedback

c8f2638

apangin merged commit 8c15cba into async-profiler:master Apr 29, 2025
15 checks passed

Baraa-Hasheesh mentioned this pull request Apr 29, 2025

async-profiler incorrect installation of hooks #1266

Closed

krk pushed a commit to krk/async-profiler that referenced this pull request May 1, 2025

Fix memory hook installation (async-profiler#1269)

4552771

apangin mentioned this pull request May 27, 2025

Native leak profiling support for Jemalloc #1318

Closed

2 tasks

Baraa-Hasheesh mentioned this pull request Sep 19, 2025

Refactor Preload mode to use patching #1386

Open

		// On Linux ARMx64 the char is dereferenced differently (-1 = 0xFF = 255 unsigned)
		if (ptr != -1 && ptr != 255) {

Fix memory hook installation #1269

Fix memory hook installation #1269

Uh oh!

Conversation

Baraa-Hasheesh commented Apr 28, 2025

Description

Related issues

Motivation and context

How has this been tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Baraa-Hasheesh commented Apr 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!