1699328 - MozDescribeCodeAddress prints SymInitialize: the parameter is incorrect and doesn't give useful frame info

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Mike Hommey [:glandium]]

On Windows, it often happens, in unclear circumstances, that MozDescribeCodeAddress prints "SymInitialize: the parameter is incorrect" between each frame processed by e.g. WalkTheStack, and each frame ends up with no useful information (something like "??? [???:???]").

Some googling led me to https://emptyhammock.com/blog/gotta-love-syminitialize.html which suggests SymInitialize is broken in funny ways, and indeed, if I just ignore its return code and let MozDescribeCodeAddress continue instead of returning early, I do get what looks like valid traces.

Comment 1

[Mike Hommey [:glandium]]

Bob, David, do you have any insight to give before I just remove the return false in https://searchfox.org/mozilla-central/rev/b7b156e53643f0237f3e98a76e5fc7fa9e3b4e71/mozglue/misc/StackWalk.cpp#585 ?

Some other data point: https://github.com/rust-lang/backtrace-rs/blob/master/src/dbghelp.rs#L344-L356 (TL;DR llvm and rust ignore the return value too)

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1699328 - Ignore SymInitialize failures.

Comment 3

[David Parks [:handyman]]

No knowledge of any of this on my end. A quick look at GitHub shows that the error code is usually respected, but that's not much of a reason to believe that means it is good. The Rust history of this is long -- I was hoping to see what error they were getting but its not written down in any of the issues.

FWIW, this old code had luck with simply retrying after getting error 0xC0000004 : https://codereview.appspot.com/11997044/patch/17001/18002
(but it sounds like ignoring the error code works fine so we don't really have to try to finesse like this).

Comment 4

[Bob Owen (:bobowen)]

I commented on the patch.

Comment 5

[Yannis Juglaret [:yannis]]

(In reply to Mike Hommey [:glandium] from comment #0)

On Windows, it often happens, in unclear circumstances, that MozDescribeCodeAddress prints "SymInitialize: the parameter is incorrect" between each frame processed by e.g. WalkTheStack, and each frame ends up with no useful information (something like "??? [???:???]").

SymInitializeW failing with ERROR_INVALID_PARAMETER 87 (0x57) The parameter is incorrect occurs when calling SymInitializeW with a hProcess value that was already used in a previous (successful) call to SymInitializeW. The hProcess value is actually used as a unique identifier in an internal data structure in DbgHelp to distinguish sessions initiated from different components. If you ignore the result of SymInitializeW, you could end up sharing a session with some other component (e.g. Rust?), which can be dangerous. Same problem if you are doing the first call and then the other component ignores the result. As per the current documentation:

[in] hProcess

A handle that identifies the caller. This value should be unique and nonzero, but need not be a process handle. However, if you do use a process handle, be sure to use the correct handle. If the application is a debugger, use the process handle for the process being debugged. Do not use the handle returned by GetCurrentProcess. The handle used must be unique to avoid sharing a session with another component, and using GetCurrentProcess can have unexpected results when multiple components are attempting to use dbghelp to inspect the current process. Using GetCurrentProcess when debugging another process will also cause functions like SymLoadModuleEx to have unexpected results.

This parameter cannot be NULL.

The two parts I bolded might sound contradictory, but there is a simple solution here which is to duplicate the handle for the current process:

HANDLE h;
if (!DuplicateHandle(GetCurrentProcess(), GetCurrentProcess(), GetCurrentProcess(), &h, 0, FALSE, DUPLICATE_SAME_ACCESS)) {
  // bad
}
if (!SymInitializeW(h, ...)) {
  // bad
}
// Starting from here, Sym* functions should keep using h, not GetCurrentProcess().

The documentation's own example does not follow the documentation's advice, but that would be because it assumes a situation where there would be only one component doing symbol stuff.

(In reply to Mike Hommey [:glandium] from comment #1)

Some other data point: https://github.com/rust-lang/backtrace-rs/blob/master/src/dbghelp.rs#L344-L356 (TL;DR llvm and rust ignore the return value too)

If all these components do SymInitialize(GetCurrentProcess(), ...), they should ideally all be patched to use code similar to the one shared above. With the patch from bug 1843354, we will no longer risk calling SymInitializeW twice ourselves, but if we could be competing with other components here it's just better to move to something like above.

Comment 6

[Yannis Juglaret [:yannis]]

Attachment: WIP: Bug 1699328 - Avoid collisions with DbgHelp symbol sessions from other components when walking the stack on Windows. r=glandium,bobowen

SymInitialize can fail with ERROR_INVALID_PARAMETER if some other piece
of code has already called it with the handle value that we are
providing. Therefore it is not recommended to pass GetCurrentProcess()
to this function. Instead, this patch duplicates the handle for the
current process, so that we can pass a unique handle to the current
process and thus avoid collision with handle values that other
components might pass to SymInitialize.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/161d82672606
Avoid collisions with DbgHelp symbol sessions from other components when walking the stack on Windows. r=bobowen

Comment 8

[Narcis Beleuzu [:NarcisB]]

Backed out for SM bustages on StackWalk.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/34cd09652b0eb2acb6b9c1e9412b2f5a107c46f2
Log link: https://treeherder.mozilla.org/logviewer?job_id=430777396&repo=autoland&lineNumber=3238

Comment 9

[Yannis Juglaret [:yannis]]

Attachment: WIP: Bug 1699328 - Modernize DbgHelp helper code and move it to a dedicated file. r=glandium,bobowen

This patch moves the DbgHelp initialization code to dedicated
DbgHelpUtils.h and DbgHelpUtils.cpp which also provides a
convenient way to use the library. This patch also makes the code more
modern.

Comment 10

[Yannis Juglaret [:yannis]]

Attachment: WIP: Bug 1699328 - Use a unique token in DbgHelp calls. r=glandium,bobowen

DbgHelp functions require a unique token hProcess that should be the
same throughout the current session and ideally a handle to the target
process. We currently use GetCurrentProcess() but that can conflict with
other sessions from third-party code on which we depend.

This patch instead uses a duplicated handle to the current process as a
unique token. This token is now passed as a parameter to functions
that call RunThreadSafe(), now renamed RunThreadSafeWithToken().

Depends on D197114

Comment 11

[Yannis Juglaret [:yannis]]

Still WIP

Comment 12

[Yannis Juglaret [:yannis]]

Attachment: Bug 1699328 - Use a unique session identifier for DbgHelp APIs in StackWalk.cpp. r=glandium,bobowen

This patch adds a RAII helper class for using DbgHelp in StackWalk.cpp.
It also ensures that we use a unique identifier as a HANDLE for
DbgHelp APIs, instead of GetCurrentProcess() which can be used by
other code running within our process.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b3a3dfc7151
Use a unique session identifier for DbgHelp APIs in StackWalk.cpp. r=glandium,bobowen

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/3b3a3dfc7151