Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

main: coredump on internal error #58

Closed

Conversation

jodh-intel
Copy link
Contributor

Override the default golang panic handling by printing a full traceback
(not just for the current goroutine) and dumping core to allow
crash-handling programs to log the details.

Fixes #57.

Signed-off-by: James O. D. Hunt [email protected]

Override the default golang panic handling by printing a full traceback
(not just for the current goroutine) and dumping core to allow
crash-handling programs to log the details.

Fixes kata-containers#57.

Signed-off-by: James O. D. Hunt <[email protected]>
Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -60,6 +61,11 @@ func initLogger(logLevel string) error {
return nil
}

func init() {
// Force coredump + full stacktrace on internal error
debug.SetTraceback("crash")
Copy link
Member

Choose a reason for hiding this comment

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

Does it show up through stderr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. afaics, when an internal error occurs, the go runtime calls builtin.print(), which does indeed write to stderr:

Copy link

Choose a reason for hiding this comment

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

@jodh-intel I assume that if we set a logger with logrus, those traces will go to this defined logger right ?
Otherwise we'll get the error printed on stderr of our console, instead of the journal, which is not the expected behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe so. This is a golang facility which doesn't provide us with a lot of control. The ideal would obviously be to log the error but as this is an unrecoverable failure scenario, the best we can do (easily) I think is squirt the data back to the user. We could start playing games like modifying the value of Stderr I guess.

Copy link

Choose a reason for hiding this comment

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

Okay I get it. Then this is fine for now, but we could investigate how to make this being redirected through the logrus logs.

Copy link
Member

Choose a reason for hiding this comment

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

Can we override stderr with a real local file or something like syslogd in such case? Sending backtrace to user's stderr might leak security bugs. If not, IMO we should make the dump configurable and do not enable it in a production environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bergwolf - this fix is too simplistic. I'm closing this and replacing it with #59.

@jodh-intel
Copy link
Contributor Author

CI failing due to clearcontainers/runtime#1044.

@sboeuf
Copy link

sboeuf commented Mar 5, 2018

LGTM

Approved with PullApprove

@jodh-intel jodh-intel closed this Mar 7, 2018
@jodh-intel jodh-intel removed the review label Mar 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants