Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stack overflow in trap handlers #835

Closed
zuevmaxim opened this issue Nov 2, 2023 · 7 comments
Closed

Stack overflow in trap handlers #835

zuevmaxim opened this issue Nov 2, 2023 · 7 comments

Comments

@zuevmaxim
Copy link

Why do we need a call to orig_trapHandler here

orig_trapHandler(signo, siginfo, ucontext);
?

As I can see trapHandler method is only called from AllocTracer::trapHandler which is installed as orig_trapHandler. I think that it leads to a crash:
crash2050223224811971188.txt.

We use a patched version of the async-profiler built from master d29cb86 (I can share dylib in DM)

@apangin
Copy link
Collaborator

apangin commented Nov 3, 2023

Can you reproduce the issue with unmodified async-profiler?

setupSignalHandlers() sets SIGTRAP handler to AllocTracer::trapHandler while storing the previous value in orig_trapHandler. The stack trace from the crash log suggests that setupSignalHandlers() has been called twice. I don't see how this may happen in the original async-profiler code.

An obvious workaround is to check that orig_trapHandler is not equal to (void*)AllocTracer::trapHandler. But I don't want to apply it without knowing the root cause of the issue (why setupSignalHandlers() was called twice).

@zuevmaxim
Copy link
Author

We don't call setupSignalHandlers() more than async-profiler does. Can it be possibly called from Hooks::init?

@apangin
Copy link
Collaborator

apangin commented Nov 3, 2023

We don't call setupSignalHandlers() more than async-profiler does

The problem might be related to anything - even to the way how the libary is compiled/linked. So I kindly ask you to see if the issue occurs with the genuine async-profiler or provide more evidence that it's a bug in the original code.

Can it be possibly called from Hooks::init?

Hooks::init is intended for native applications; it is not normally called in a JVM process.

@zuevmaxim
Copy link
Author

zuevmaxim commented Nov 3, 2023

I see setupSignalHandlers method is called twice with my dylib. Once from VM init, and once from Hooks::init (that is called from dlopen). Can I comment out DLLEXPORT methods from hooks.cpp if I'm not interested in native applications profiling?

@apangin
Copy link
Collaborator

apangin commented Nov 4, 2023

Thank you for the update. DLLEXPORT functions from hooks.cpp should not be called, unless libasyncProfiler is preloaded using LD_PRELOAD on Linux or DYLD_INSERT_LIBRARIES on macOS.
It's OK to comment them out in your branch, but I'm still wondering why they are called in your case. Can you check if the error happens with the binary from here? https://github.com/async-profiler/async-profiler/files/12647822/async-profiler-3.0-ea-macos.zip

@zuevmaxim
Copy link
Author

Thank you for your help!
The crash happened only a cople of times on some anonymous user machine, so I can't check whether it is reproducible with the original agent

@apangin
Copy link
Collaborator

apangin commented Jan 3, 2024

I refactored hooks and setupSignalHandlers in recent commits.
The mentioned issue should not occur anymore.

@apangin apangin closed this as completed Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants