Add nativemem test with dlopen after profiler start. #1243

krk · 2025-04-17T18:25:14Z

Description

Parse symbols and hook functions of the main executable, when profiler is not loaded via LD_PRELOAD.

Without this fix, profiler is unable to hook non-java executables if dlopen is called after starting the profiler.

Related issues

Issue was introduced in #1220 as dlerror may not return NULL for the main executable.

Thanks for the repro in #1241 @Baraa-Hasheesh and for discussions @fandreuz and @Baraa-Hasheesh.

Fixes #1233 and #1241.

How has this been tested?

make test-java TESTS=nativemem

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

fandreuz · 2025-04-17T19:20:16Z

src/symbols_linux.cpp

+            // Main executable will return NULL from handle, we need to parse it anyway.
+            // dlopen is not required in this case as the main exe cannot be unloaded.
+            bool isMainExe = handle == NULL && exePath != NULL && strcmp(lib.file, exePath) == 0;
+            bool dlopenSuccess = handle != NULL || dlerror_str == NULL || OS::isMusl();


Maybe we don't need OS::isMusl() anymore? The only situation when this was appropriate is for the main executable, in which case isMainExe is true anyways

fandreuz · 2025-04-17T19:27:34Z

test/native/libs/callsmalloc.c

+
+#include <stdlib.h>
+
+__attribute__((visibility("default"))) void* call_malloc(size_t size) {


Maybe you could reuse Java_test_nativemem_Native_malloc from jnimalloc.c instead of creating a new file?

I think this will be confusing to the reader. We do not use JNI but method name looks like JNI and has this signature:

JNIEXPORT jlong JNICALL Java_test_nativemem_Native_malloc(JNIEnv* env, jclass clazz, jlong size)

as opposed to what we need:

void* call_malloc(size_t size)

fandreuz · 2025-04-17T19:30:09Z

test/one/profiler/test/RunnableTest.java

+        return test.nameSuffix().isEmpty()
+            ? className() + '.' + m.getName()
+            : className() + '.' + m.getName() + ' ' + test.nameSuffix();


Suggested change

return test.nameSuffix().isEmpty()

? className() + '.' + m.getName()

: className() + '.' + m.getName() + ' ' + test.nameSuffix();

return String.format("%s.%s %s", className(), m.getName(), test.nameSuffix()).trim();

Kerem's version is better: it's more obvious and far more efficient (although the latter is not important here, let's not support bad practice).

fandreuz · 2025-04-17T19:31:25Z

test/test/nativemem/profile_with_dlopen.c

+#define ASSERT_NO_DLERROR()  \
+    err = dlerror();         \
+    if (err != NULL) {       \
+        printf("%s\n", err); \


stderr? Same for L21 and L39

fandreuz · 2025-04-17T19:33:22Z

test/test/nativemem/profile_with_dlopen.c

+#include <stdlib.h>
+#include <string.h>
+
+#define ASSERT_NO_DLERROR()  \


What's the advantage of #define over a function?

We can exit directly, without checking the return value of a new function. I used macros here to reduce repetition.

apangin

Thank you for root causing the issue.

apangin · 2025-04-17T19:41:38Z

src/symbols_linux.cpp

+            buf[size] = 0;
+            exePath = buf;
+        }
+    }


There is a simpler and more efficient way to find main executable:

const void* main_phdr = NULL; dl_iterate_phdr([](struct dl_phdr_info* info, size_t size, void* data) { *(const void**)data = info->dlpi_phdr; return 1; }, &main_phdr);

Then check if main_phdr >= lib.image_base && main_phdr < lib.map_end

Calling dl_iterate_phdr should be fine as long as there are no allocations inside.

apangin · 2025-04-17T19:45:09Z

src/symbols_linux.cpp

+
+            // Main executable will return NULL from handle, we need to parse it anyway.
+            // dlopen is not required in this case as the main exe cannot be unloaded.
+            bool isMainExe = handle == NULL && exePath != NULL && strcmp(lib.file, exePath) == 0;


snake_case for variable names.

Baraa-Hasheesh · 2025-04-17T20:04:32Z

test/test/nativemem/NativememTests.java

+    @Test(os = Os.LINUX, sh = "%testbin/profile_with_dlopen dlopen_first %f.jfr", output = true, env = {"LD_LIBRARY_PATH=build/test/lib:build/lib"}, nameSuffix = "dlopen_first")
+    @Test(os = Os.LINUX, sh = "%testbin/profile_with_dlopen profile_first %f.jfr", output = true, env = {"LD_LIBRARY_PATH=build/test/lib:build/lib"}, nameSuffix = "profile_first")
+    @Test(os = Os.LINUX, sh = "LD_PRELOAD=%lib %testbin/profile_with_dlopen dlopen_first %f.jfr", output = true, env = {"LD_LIBRARY_PATH=build/test/lib:build/lib"}, nameSuffix = "dlopen_first with LD_PRELOAD")
+    @Test(os = Os.LINUX, sh = "LD_PRELOAD=%lib %testbin/profile_with_dlopen profile_first %f.jfr", output = true, env = {"LD_LIBRARY_PATH=build/test/lib:build/lib"}, nameSuffix = "profile_first with LD_PRELOAD")


We should add a new test that checks the dlopen patching on both MacOs & Linux
As this issue of not patching the main executable is also relevant to MacOs

Macos support is "best effort", will leave it out from the current PR.

Baraa-Hasheesh · 2025-04-17T20:08:04Z

src/symbols_linux.cpp

        array->add(cc);
    }

+    free(exePath);


is this safe? as in some cases it points toward the buf[PATH_MAX] which is defined in the stack not the heap

Good catch! This will be deleted in the new version.

Baraa-Hasheesh · 2025-04-17T20:10:23Z

test/test/nativemem/profile_with_dlopen.c

+        exit(1);                                \
+    }
+
+asprof_error_str_t _asprof_error_str;


no need for this global variable, you can define it in the main function

Baraa-Hasheesh · 2025-04-18T06:15:42Z

@krk @apangin @fandreuz

The above fix doesn't fully fix the issue of patching libs,
Due to how the dynamic linker work & common optimization it does -> if you do the following steps

void* lib1 = dlopen("libcallsmalloc.so", RTLD_NOW);
dlclose(lib1);
void2 lib2 = dlopen("libcallsmalloc.so", RTLD_NOW);

You will actually find that the lib1 & lib2 point to the same address even though the shared objects were deleted & recreated

=> you can try this in the new test by doing the following

This means due to the Async-profiler built in optimization of skipping already patched libs the second dlopen will never be patched as it will be seen as already patched from the first dlopen

This is quite an old issue (Even the old logic before #1220 has the issue) that could require significant changes to address

Hooking the dlclose calls & actually removing the patches from the async profiler cache if reference counts reaches 0 for a lib.
We could need locking logic as it's possible race conditions may happen when dlclose-ing & dlopen-ing from multiple threads.

Interesting edge cases will be handling the RTLD_NODELETE flag as even if the reference counter reaches 0, there is no need to cleanup the async-profiler local cache as the shared objects are never unloaded.

Baraa-Hasheesh · 2025-04-18T08:30:43Z

Creating a new issue here #1245

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

src/symbols_linux.cpp

+            // dlopen is not required in this case as the main exe cannot be unloaded.
+            bool isMainExe = handle == NULL && main_phdr >= lib.image_base && main_phdr < lib.map_end;
+            bool dlopenSuccess = handle != NULL || dlerror_str == NULL;
+            if (isMainExe || dlopenSuccess) {


What about

bool is_main_exe = main_phdr >= lib.image_base && main_phdr < lib.map_end; if (handle != NULL || dlerror() == NULL || is_main_exe) {

apangin · 2025-04-18T14:58:49Z

test/one/profiler/test/RunnableTest.java

-        return className() + '.' + m.getName();
+        return test.nameSuffix().isEmpty()
+            ? className() + '.' + m.getName()
+            : className() + '.' + m.getName() + ' ' + test.nameSuffix();


testName() is also a name of a log subdirectory; spaces in directory names are not convenient.

…1243)

Add nativemem test with dlopen after profiler start.

0fd527d

fandreuz reviewed Apr 17, 2025

View reviewed changes

apangin reviewed Apr 17, 2025

View reviewed changes

Baraa-Hasheesh reviewed Apr 17, 2025

View reviewed changes

Baraa-Hasheesh mentioned this pull request Apr 18, 2025

async-profiler doesn't patch previously patched library in case of library reload #1245

Open

fandreuz mentioned this pull request Apr 18, 2025

Build and run tests on amazonlinux:2023 and amazonlinux:2 #1246

Merged

address comments

6c2d1c5

apangin reviewed Apr 18, 2025

View reviewed changes

krk and others added 2 commits April 18, 2025 16:47

address comments

10702e8

Formatting

ca581c3

apangin merged commit fa417c8 into async-profiler:master Apr 19, 2025
10 checks passed

apangin mentioned this pull request Apr 19, 2025

Symbols of main executable are not parsed #1241

Closed

This was referenced Apr 23, 2025

Fix NativememTests#dlopenCustomLib on Alpine #1254

Merged

Protect patching with an open handle #1264

Merged

krk added a commit to krk/async-profiler that referenced this pull request Apr 25, 2025

Add nativemem test with dlopen after profiler start. (async-profiler#…

aba8fd8

…1243)


		#include <stdlib.h>

		__attribute__((visibility("default"))) void* call_malloc(size_t size) {

Add nativemem test with dlopen after profiler start. #1243

Add nativemem test with dlopen after profiler start. #1243

Uh oh!

Conversation

krk commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

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!

fandreuz Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

apangin left a comment

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 Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 18, 2025

Uh oh!

Baraa-Hasheesh commented Apr 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

krk commented Apr 17, 2025 •

edited

Loading

fandreuz Apr 17, 2025 •

edited

Loading

Baraa-Hasheesh Apr 17, 2025 •

edited

Loading