Correctly check if profiler is preloaded #1374

Baraa-Hasheesh · 2025-07-09T14:51:27Z

Description

Add proper checking to validate that the profiler was preloaded as to avoid false starts

Currently if ASPROF_COMMAND env variable is defined any dlopen on the profiler will cause it to start which means the profiler might start with incorrect profiling options

Furthermore in this case async-profiler will assume that preload hooks exists which might not be true, which will to incomplete profiling by the profiler

Related issues

N/A

Motivation and context

Avoid false early starts for the async-profiler

How has this been tested?

New test added
make test

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

apangin · 2025-07-09T22:04:01Z

src/os_linux.cpp

+
+    if (info->dlpi_name == NULL) {
+        return 0;
+    } else if (strcmp(info->dlpi_name, current_info.dli_fname) == 0) {


Can we compare base address (dli_fbase) instead?

I will need to check on this

have been updated here 78edb2a

From the testing I did it looks like it does the trick

apangin · 2025-07-09T22:17:24Z

src/os_macos.cpp

+            continue;
+        } else if (strcmp(image_name, current_info.dli_fname) == 0) {
+            return true;
+        }else if(strcmp(image_name, sigaction_info.dli_fname) == 0) {


handled here 78edb2a

apangin · 2025-07-09T22:19:38Z

src/os_linux.cpp

+    }
+
+    Dl_info sigaction_info;
+    if (dladdr((const void*)sigaction, &sigaction_info) == 0 || sigaction_info.dli_fname == NULL) {


Just wondering why sigaction? Not the most obvious function in libc - may confuse someone who sees this for the first time. A short comment describing the idea of checkPreloaded would be useful anyway.

I wanted to avoid typical functions that some other developer might interpose/preload as that would effect the accuracy of the check

For example dlopenon Linux dladdr will resolve to the async-profiler when it's preloaded

=> I wanted to avoid using functions that we patch/preload to keep the check consistent for all cases

I will add a comment for describing what the function does

doc added here 78edb2a

Still, sigaction is a bad choice. Not only because it's some random unrelated function, but also because it does not meet the criteria you've mentioned:

I wanted to avoid typical functions that some other developer might interpose/preload

It's not uncommon to intercept sigaction for signal chaining purposes. A prominent example is JDK's libjsig library.

I'd choose some basic function like exit.

updated here 6e283d6

apangin · 2025-07-09T22:23:12Z

src/zInit.cpp

        }

-        if (!checkJvmLoaded()) {
+        if (!checkJvmLoaded() && OS::checkPreloaded()) {


A minor thing, but I'd propose to move this check after ASPROF_COMMAND check, since using async-profiler through native API will be a common thing, while LD_PRELOAD is used rarely and mostly for ad-hoc profiling only.

handled here 78edb2a

…iable, check image address for linux

apangin · 2025-07-10T13:57:37Z

src/os_linux.cpp

+    if (info->dlpi_name == NULL) {
+        return 0;


Redundant check?

removed 6e283d6

apangin · 2025-07-10T13:58:37Z

src/os_linux.cpp

+
+    // Find async-profiler shared objects
+    Dl_info current_info;
+    if (dladdr((const void*)OS::checkPreloaded, &current_info) == 0 || current_info.dli_fname == NULL) {


Is comparing dli_fname really necessary?

I will update it to check dli_fbase instead

updated here 6e283d6

apangin · 2025-07-10T14:12:02Z

src/os_linux.cpp

+    }
+
+    Dl_info sigaction_info;
+    if (dladdr((const void*)sigaction, &sigaction_info) == 0 || sigaction_info.dli_fname == NULL) {


Still, sigaction is a bad choice. Not only because it's some random unrelated function, but also because it does not meet the criteria you've mentioned:

I wanted to avoid typical functions that some other developer might interpose/preload

It's not uncommon to intercept sigaction for signal chaining purposes. A prominent example is JDK's libjsig library.

I'd choose some basic function like exit.

apangin · 2025-07-10T14:13:49Z

src/os_macos.cpp

+
+    uint32_t images = _dyld_image_count();
+    for (uint32_t i = 0; i < images; i++) {
+        const char* image_name = _dyld_get_image_name(i);


Is it possible to use base address on macOS, similarly to how you do this on Linux?

I did some double checking, yes we can check against _dyld_get_image_header

Updated here 6e283d6

apangin · 2025-07-10T16:09:46Z

src/os_linux.cpp

+
+    // Find async-profiler shared objects
+    Dl_info current_info;
+    if (dladdr((const void*)OS::checkPreloaded, &current_info) == 0 || current_info.dli_fbase == NULL) {


Why would dli_fbase be NULL? Is it even possible?

yeah it shouldn't be needed, will remove it

apangin · 2025-07-10T16:12:34Z

src/os_macos.cpp

+        return false;
+    }
+
+    // Find async-profiler shared objects


object (singular)?

apangin · 2025-07-10T16:26:52Z

src/os_macos.cpp

+    }
+
+    // Find async-profiler shared objects
+    Dl_info current_info;


current_info and exist_info sound a bit vague.
What about lib_async_profiler (or maybe libprofiler) and libc?

Then conditions like if (image_base == libc.dli_fbase) will be self-descriptive.

I will use libprofiler & libc

Updated here 486dc9d

Correctly check if profiler is preloaded

4ac62ea

apangin reviewed Jul 9, 2025

View reviewed changes

Baraa-Hasheesh added 2 commits July 10, 2025 06:04

Merge branch 'master' into preload_check

b7a6c14

add documentation, move check to be done after ASPROF_COMMAND env var…

78edb2a

…iable, check image address for linux

apangin reviewed Jul 10, 2025

View reviewed changes

Baraa-Hasheesh added 2 commits July 10, 2025 16:42

using exit & address check on mac

6e283d6

space

f32d5b6

apangin reviewed Jul 10, 2025

View reviewed changes

Baraa-Hasheesh and others added 2 commits July 10, 2025 16:37

feedback

486dc9d

Stylistic edits

701cde3

apangin merged commit 461a3c1 into async-profiler:master Jul 10, 2025
18 checks passed

Correctly check if profiler is preloaded #1374

Correctly check if profiler is preloaded #1374

Uh oh!

Conversation

Baraa-Hasheesh commented Jul 9, 2025

Description

Related issues

Motivation and context

How has this been tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Baraa-Hasheesh Jul 10, 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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Baraa-Hasheesh Jul 10, 2025 •

edited

Loading