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

Incompatible with JNI on Windows: use unique handle for SymInitialize #204

Open
Kyuuhachi opened this issue Jan 15, 2025 · 7 comments · Fixed by #206
Open

Incompatible with JNI on Windows: use unique handle for SymInitialize #204

Kyuuhachi opened this issue Jan 15, 2025 · 7 comments · Fixed by #206
Labels
bug Something isn't working dbghelp resolved in next release Resolved in dev

Comments

@Kyuuhachi
Copy link

When trying to use cpptrace in a JNI library, it only gives empty traces, or with absorb_trace_exceptions(false), it gives an exception Cpptrace internal error: SymInitialize failed 87 (87 is ERROR_INVALID_PARAMETER). This appears to be caused by doing SymInitialize on the same handle twice, since openjdk also does that.

This can be solved by duplicating the handle, so that cpptrace initializes its own unique copy. The following diff (in {unwind,symbols}_with_dbghelp.cpp), in conjunction with cpptrace::experimental::set_cache_mode(cpptrace::cache_mode::prioritize_memory), allows capturing traces when GetCurrentProcess() is already initialized.

-         HANDLE proc = GetCurrentProcess();`
+         HANDLE proc0 = GetCurrentProcess(), proc;
+         DuplicateHandle(proc0, proc0, proc0, &proc, 0, FALSE, DUPLICATE_SAME_ACCESS);

This is only a sketch for a fix: a real one would need to close the handle and to support other cache modes. I don't currently have the time nor expertise to make a PR for a real fix, so a bug report will have to suffice.

@jeremy-rifkin
Copy link
Owner

Hi,
Thanks for the report and suggested fix. I don’t have enough windows api expertise to understand the full implications of duplicating the process handle for this purpose. I see in documentation that a process requires specific permissions in order to use this so it’s probably not something cpptrace should try to unconditionally use. It might be sufficient to use it only in the case of SymInitialize failure but I’m not immediately sure. I’ll have to investigate this more.

@jeremy-rifkin jeremy-rifkin added bug Something isn't working dbghelp labels Jan 18, 2025
@firesgc
Copy link

firesgc commented Jan 22, 2025

Hi!

I am running into the same issue because Tracy does the same as JNI.

In the official Microsoft example, the handle is duplicated as well (https://learn.microsoft.com/en-us/windows/win32/debug/initializing-the-symbol-handler).

Perhaps it is the best practice?

@jeremy-rifkin
Copy link
Owner

Thanks for the link, if microsoft shows this as an example that helps give me confidence in the approach. I can take a look at making changes in cpptrace here or I'd welcome a PR

@firesgc
Copy link

firesgc commented Jan 24, 2025

I have created a PR, and in my own project, it works as intended, but the integration tests are failing. In Windows, CMake doesn't create the integration tests (at least I dont see them in the VS solution), so I don't know how to debug it.

@jeremy-rifkin
Copy link
Owner

Thanks so much for taking the time to make a PR. I'm looking now and I'll give it a test locally.

@firesgc
Copy link

firesgc commented Jan 25, 2025

I have edited most of the comments and also added that the duplicate handles will be closed at the end now.

jeremy-rifkin added a commit that referenced this issue Jan 26, 2025
…204 (#206)

WinDbg: Duplicate the process handle before using it in SymInitialize,
because other libraries may have already called SymInitialize for the
process
(https://learn.microsoft.com/en-us/windows/win32/debug/initializing-the-symbol-handler)


Fixes #204

---------

Co-authored-by: Jeremy <[email protected]>
@jeremy-rifkin jeremy-rifkin added the resolved in next release Resolved in dev label Jan 26, 2025
@jeremy-rifkin
Copy link
Owner

Resolved on dev, will do a release some time in the near future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dbghelp resolved in next release Resolved in dev
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants