Skip to content

Refactor _swift_withWin32DbgHelpLibrary() to avoid using GetCurrentProcess() per Microsoft documentation #62294

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

Merged
merged 3 commits into from
Dec 1, 2022

Conversation

grynspan
Copy link
Contributor

@grynspan grynspan commented Nov 29, 2022

The Microsoft documentation for the DbgHelp library warns developers not to pass GetCurrentProcess() as the handle to SymInitialize() etc.

This change uses a privately-held handle duplicated from GetCurrentProcess(). It also takes care to set DbgHelp library options consistently in case something elsewhere calls SymSetOptions() and sets them to something incompatible with the runtime's expectations.

We still have no way to thread-safely use the library in coordination with code outside the runtime, but at least the runtime should avoid stomping on shared system resources.

Resolves #62290.

@grynspan grynspan requested a review from compnerd November 29, 2022 17:15
@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

2 similar comments
@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

@grynspan grynspan force-pushed the jgrynspan/dbghelp-pass-handle branch from 12ad7f0 to e199f0b Compare November 29, 2022 21:03
@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

@grynspan grynspan closed this Nov 29, 2022
@grynspan grynspan reopened this Nov 29, 2022
@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

@grynspan
Copy link
Contributor Author

@swift-ci please test windows

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I don't think I ever noticed the fine print there. This is terrifying, this is terrible, and we should feel bad. Thanks for this fix, sorry for any scars that it may have inflicted upon you.

@al45tair
Copy link
Contributor

al45tair commented Nov 30, 2022

I particularly enjoy this part (emphasis mine):

[in] hProcess

A handle that identifies the caller. This value should be unique and nonzero, but need not be a process handle.

So, they called it hProcess and gave it the type HANDLE, but it can actually be any unique value.

Maybe we'd be better off just casting the address of something to HANDLE and passing that, since that will be unique and means we don't need to worry about DuplicateHandle() failing.

@grynspan
Copy link
Contributor Author

That comment lies. If you pass some other address with fInvadeProcess = true then SymInitialize() succeeds but symbolication with SymFromAddr() fails with error ERROR_MOD_NOT_FOUND.

I suspect UnDecorateSymbolName() would work in that scenario since it doesn't take a handle argument, but I haven't tested it specifically.

@al45tair
Copy link
Contributor

That comment lies. If you pass some other address with fInvadeProcess = true then SymInitialize() succeeds but symbolication with SymFromAddr() fails with error ERROR_MOD_NOT_FOUND.

Joy 😂

@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

@grynspan
Copy link
Contributor Author

grynspan commented Dec 1, 2022

@swift-ci please smoke test

@grynspan grynspan merged commit c30f62f into swiftlang:main Dec 1, 2022
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.

_swift_withWin32DbgHelpLibrary() should not use GetCurrentProcess()
4 participants