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 #206

Merged
merged 11 commits into from
Jan 26, 2025

Conversation

firesgc
Copy link

@firesgc firesgc commented Jan 24, 2025

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

Copy link
Owner

@jeremy-rifkin jeremy-rifkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together! Some commends below

src/unwind/unwind_with_dbghelp.cpp Outdated Show resolved Hide resolved
} else {
if(!SymInitialize(proc, NULL, TRUE)) {
if (!DuplicateHandle(proc, proc, proc, &duplicatedHandle, 0, FALSE, DUPLICATE_SAME_ACCESS)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic appears in a couple places and having grown I'm thinking it would be good to refactor out. That's something I can do after merging if you'd like to not delay this pr.

src/platform/dbghelp_syminit_manager.hpp Outdated Show resolved Hide resolved
src/platform/dbghelp_syminit_manager.cpp Outdated Show resolved Hide resolved
src/platform/dbghelp_syminit_manager.cpp Outdated Show resolved Hide resolved
@jeremy-rifkin
Copy link
Owner

I tested locally and was able to reproduce the failing tests, I think I fixed it

Copy link
Owner

@jeremy-rifkin jeremy-rifkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, thanks so much! Happy to follow up on this PR with the few items we talked about above

@jeremy-rifkin jeremy-rifkin changed the base branch from main to dev January 25, 2025 17:25
@jeremy-rifkin
Copy link
Owner

I'm going to merge into dev and then this'll be part of the next release

@jeremy-rifkin jeremy-rifkin merged commit 1bcfcff into jeremy-rifkin:dev Jan 26, 2025
81 of 82 checks passed
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

Successfully merging this pull request may close these issues.

Incompatible with JNI on Windows: use unique handle for SymInitialize
2 participants