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

proposal: runtime: use WER for GOTRACEBACK=crash on Windows #57441

Open
qmuntal opened this issue Dec 22, 2022 · 12 comments
Open

proposal: runtime: use WER for GOTRACEBACK=crash on Windows #57441

qmuntal opened this issue Dec 22, 2022 · 12 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Debugging OS-Windows Proposal
Milestone

Comments

@qmuntal
Copy link
Contributor

qmuntal commented Dec 22, 2022

WER is a Windows built-in infrastructure provided by Microsoft that helps troubleshooting hardware and software problems. Many systems rely on the core dumps generated by WER for post-mortem analyses. These dumps are collected by a dedicated process running on the host and uploaded to the WER server and\or stored locally (see Collecting User-Mode Dumps).

#20498 added support for WER when setting GOTRACEBACK=crash, but was reverted and replaced by #49471. The reason to revert it was because WER uploads dumps to Microsoft by default, and this bit was not discussed in the original issue. See this comment #49471 (comment) to understand why I'm counterproposing #49471 after implementing it.

Here I'm proposing to revert the revert, enabling WER when GOTRACEBACK=crash is set. The arguments come next, but first the definition of the crash mode:

GOTRACEBACK=crash is like “system” but crashes in an operating system-specific manner instead of exiting. For example, on Unix systems, the crash raises SIGABRT to trigger a core dump.

I have two simple arguments in favor of WER:

  • WER is the Windows specific manner to crash applications. It is the default experience and many production systems rely on WER to do post-mortem analysis, not only from within Microsoft.

  • WER can be used for collecting user-mode dumps without adding any complexity to the Go runtime.

It's true that some users might not want their diagnosis data to be uploaded to Microsoft servers, but my point is that they already have enough knobs to turn it off or limit what's uploaded:

  • GOTRACEBACK=crash is not the default runtime behavior, one have to manually set via environment variable or programmatically via debug.SetTraceback. We can inform about the WER gotchas in the release notes and in the GOTRACEBACK docs.
  • On Windows, GOTRACEBACK=crash does nothing over GOTRACEBACK=system. If this proposal is approved, the only reason to set GOTRACEBACK=crash would be to opt into WER.
  • Privacy-concerned users will most likely opt-out to uploading diagnosis data when setting up windows for the first time, which disables WER.
  • WER can easily be disabled by running Disable-WindowsErrorReporting on PowerShell. One can also disable just some applications using the system registry.

Side note: #20498 is a bug report, not a proposal, so I think it makes sense to create this as a formal proposal.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 22, 2022
@ianlancetaylor ianlancetaylor changed the title runtime: use WER for GOTRACEBACK=crash on Windows proposal: runtime: use WER for GOTRACEBACK=crash on Windows Dec 22, 2022
@gopherbot gopherbot added this to the Proposal milestone Dec 22, 2022
@ianlancetaylor
Copy link
Contributor

CC @zx2c4 @zhizheng052 @bhiggins @alexbrainman @aarzilli @aclements @golang/windows

@zx2c4
Copy link
Contributor

zx2c4 commented Dec 22, 2022

Wasn't this already discussed and rejected in an earlier proposal? The privacy concerns seem real, especially given how hard opting out has become on Windows, and how little trust exists on the user side these days...

@ianlancetaylor
Copy link
Contributor

This is my personal summary: The earlier attempt was removed due to privacy concerns (which you raised on https://go.dev/cl/307372 -- thanks). Then we decided to adopt minidump in #49471. However, the actual implementation of minidump (https://go.dev/cl/458955) turns out to have some problems (see #49471 (comment)). So this proposal suggests that we reconsider WER despite the privacy issues.

That is, I don't think we made a clear decision that we could not live with the privacy issues, because it seemed that minidump was a viable alternative. Now that we have learned more, it's possible that minidump is not a viable alternative. So let's consider whether we can live with the privacy issues.

I don't have an opinion myself.

@bhiggins
Copy link

IMO, it would be nice to have (and simple to implement) an opt-in only means for Go programs on Windows to work with WER. In addition to being opt-in only, there could be a warning in documentation that states that it's possible for WER to be configured to send crash dumps to Microsoft.

@qmuntal
Copy link
Contributor Author

qmuntal commented Dec 23, 2022

The privacy concerns seem real, especially given how hard opting out has become on Windows, and how little trust exists on the user side these days.

If this proposal is approved, WER would still be opt-in, so only users that need it would have it.

Opting out is just one PS command, Disable-WindowsErrorReporting, so I wouldn't say its hard. Additionally, the previous command only disables uploading dumps to the server, one can still use WER to generate local dumps.

@alexbrainman
Copy link
Member

@qmuntal Thank you for creating this proposal. I agree with your sentiment.

I always supported this functionality #49471 (comment) .

And it looks silly that Delve supports Go Windows minidumps, but Go tools do not provide tools to generate these dumps (see #49471 (comment) ).

Alex

@zx2c4
Copy link
Contributor

zx2c4 commented Dec 25, 2022

My process address space can contain private keys. I can't put those in special memory regions because Go's GC. Ordinary users use my software who can't be fiddling around with powershell. Sometimes my software crashes. When this happens, uploading private keys to some error reporting server is a nonstarter. It must not happen. I can't imagine I'm the only one writing software with this situation, and I wouldn't want to choose the dangerous thing for other developers who might not even be aware that their private keys are sensitive - like casual users of TLS/HTTPS.

@aarzilli
Copy link
Contributor

aarzilli commented Dec 25, 2022

My opinion remains as in #49471 (comment) (i.e. in favor of this).

@bhiggins
Copy link

bhiggins commented Dec 25, 2022 via email

@qmuntal
Copy link
Contributor Author

qmuntal commented Dec 25, 2022

Can we go with an explicit opt-in method to crash in a WER-compatible way
(which really just means not swallowing the error)? This could involve a
new GOTRACEBACK=crash-wer option and/or a runtime function.

We could, but I don't see how crash-wer would bring any benefit over crash, considering that the later is not currently implemented on Windows. It would add confusion to an already overloaded knob.

@zx2c4 ordinary users will not disable WER using a PS command, but neitheir will set GOTRACEBACK=crash unless someone guides them. I completly understand your privacy concerns, but I don't get why WER being opt-in is not enough of a mitigation.

Additionally, by default WER does not upload the full virtual memory, just the data segment, which can contain global variables, but not the heap nor the stack.

@iglendd
Copy link

iglendd commented Dec 27, 2022

Thank you @qmuntal again for bring it forward again. I would actually argue that the proposal title is a bit misnomer and least as far as intent of the proposal is concerned. The proposal effectively intends to implement Go documented behavior “crashes in an operating system-specific manner”. No more nor less (never mind that this is on par with other platforms). On Windows it means either WER, custom crash handling, debugger activation, documented and used report entry in Windows Event log and usage of built-in and 3rd party tools. Another word, it is expected behavior for bazillion of applications running on Windows for many decades. Besides, it is a Go-runtime debugging and troubleshooting feature. If one is concerned with some side-effects for whatever reasons, do not activate it.

@danicat
Copy link
Contributor

danicat commented Jan 3, 2023

I got to this topic from #20498 and I've spend the last hour or so trying to catch up with all the back and forths, so I'm sorry if I'm missing the point, but I just wanted to give my 2 cents.

As @qmuntal expressed in their opening statement, the documentation on GOTRACEBACK=crash is very opinionated:

GOTRACEBACK=crash is like “system” but crashes in an operating system-specific manner instead of exiting. For example, on Unix systems, the crash raises SIGABRT to trigger a core dump.

Based on that, IMHO, whether the SO sends or sends not the crash information to Microsoft is out of context from a Go program perspective. Microsoft says in their documentation:

Beginning with Windows Vista, Windows provides crash, non-response, and kernel fault error reporting by default without requiring changes to your application.

Then they further elaborate:

Because Windows automatically reports unhandled exceptions, application should not handle fatal exceptions.

First impression is that the decision to send or not send WER data to MS is in the scope of the system admin, not the developers. It is a bit old, but they document the controls of this feature in this article here.

Also, it is not clear to me if the way runtime is currently treating error is effectively "handling fatal exceptions" from a Windows perspective and that's why we are losing this functionality. If that's the case, GOTRACEBACK=crash should only disable any special treatment we do and let the application... er... crash. :) [By that I mean I wouldn't try to add any side effects like generating different types of dumps other than the default]

Another concern is that we are not generating Event Log ID 1000, as previously mentioned by @iglendd in #49471 (comment). This was the main reason why I got into this rabbit hole.

Finally, I'm more than happy to help with any coding if needed. I'm way too invested in this now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Debugging OS-Windows Proposal
Projects
Status: No status
Status: Incoming
Development

No branches or pull requests

9 participants