From 622c4df14c09fd0a1599c54d00bdbe46453bd05e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 26 Oct 2021 15:54:09 -0400 Subject: [PATCH 1/8] remove the use of panicwrap Stop using panicwrap, and execute terraform in the main process. --- go.mod | 1 - go.sum | 2 - internal/command/console_interactive.go | 5 +- internal/terminal/panicwrap_ugh.go | 78 ---------------------- main.go | 89 ++----------------------- main_test.go | 6 +- 6 files changed, 11 insertions(+), 170 deletions(-) delete mode 100644 internal/terminal/panicwrap_ugh.go diff --git a/go.mod b/go.mod index 646309e2a..11095b765 100644 --- a/go.mod +++ b/go.mod @@ -63,7 +63,6 @@ require ( github.com/mitchellh/go-wordwrap v1.0.1 github.com/mitchellh/gox v1.0.1 github.com/mitchellh/mapstructure v1.1.2 - github.com/mitchellh/panicwrap v1.0.0 github.com/mitchellh/reflectwalk v1.0.2 github.com/nishanths/exhaustive v0.2.3 github.com/packer-community/winrmcp v0.0.0-20180921211025-c76d91c1e7db diff --git a/go.sum b/go.sum index bd8b487fb..0fb3794c2 100644 --- a/go.sum +++ b/go.sum @@ -515,8 +515,6 @@ github.com/mitchellh/iochan v1.0.0/go.mod h1:JwYml1nuB7xOzsp52dPpHFffvOCDupsG0Qu github.com/mitchellh/mapstructure v0.0.0-20160808181253-ca63d7c062ee/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y= github.com/mitchellh/mapstructure v1.1.2 h1:fmNYVwqnSfB9mZU6OS2O6GsXM+wcskZDuKQzvN1EDeE= github.com/mitchellh/mapstructure v1.1.2/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y= -github.com/mitchellh/panicwrap v1.0.0 h1:67zIyVakCIvcs69A0FGfZjBdPleaonSgGlXRSRlb6fE= -github.com/mitchellh/panicwrap v1.0.0/go.mod h1:pKvZHwWrZowLUzftuFq7coarnxbBXU4aQh3N0BJOeeA= github.com/mitchellh/reflectwalk v1.0.0/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw= github.com/mitchellh/reflectwalk v1.0.2 h1:G2LzWKi524PWgd3mLHV8Y5k7s6XUvT0Gef6zxSIeXaQ= github.com/mitchellh/reflectwalk v1.0.2/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw= diff --git a/internal/command/console_interactive.go b/internal/command/console_interactive.go index c9d63245c..15fde13b4 100644 --- a/internal/command/console_interactive.go +++ b/internal/command/console_interactive.go @@ -10,7 +10,6 @@ import ( "fmt" "io" - "github.com/hashicorp/terraform/internal/helper/wrappedreadline" "github.com/hashicorp/terraform/internal/repl" "github.com/chzyer/readline" @@ -19,12 +18,12 @@ import ( func (c *ConsoleCommand) modeInteractive(session *repl.Session, ui cli.Ui) int { // Configure input - l, err := readline.NewEx(wrappedreadline.Override(&readline.Config{ + l, err := readline.NewEx(&readline.Config{ Prompt: "> ", InterruptPrompt: "^C", EOFPrompt: "exit", HistorySearchFold: true, - })) + }) if err != nil { c.Ui.Error(fmt.Sprintf( "Error initializing console: %s", diff --git a/internal/terminal/panicwrap_ugh.go b/internal/terminal/panicwrap_ugh.go deleted file mode 100644 index b17165b2c..000000000 --- a/internal/terminal/panicwrap_ugh.go +++ /dev/null @@ -1,78 +0,0 @@ -package terminal - -import "os" - -// This file has some annoying nonsense to, yet again, work around the -// panicwrap hack. -// -// Specifically, typically when we're running Terraform the stderr handle is -// not directly connected to the terminal but is instead a pipe into a parent -// process gathering up the output just in case a panic message appears. -// However, this package needs to know whether the _real_ stderr is connected -// to a terminal and what its width is. -// -// To work around that, we'll first initialize the terminal in the parent -// process, and then capture information about stderr into an environment -// variable so we can pass it down to the child process. The child process -// will then use the environment variable to pretend that the panicwrap pipe -// has the same characteristics as the terminal that it's indirectly writing -// to. -// -// This file has some helpers for implementing that awkward handshake, but the -// handshake itself is in package main, interspersed with all of the other -// panicwrap machinery. -// -// You might think that the code in helper/wrappedstreams could avoid this -// problem, but that package is broken on Windows: it always fails to recover -// the real stderr, and it also gets an incorrect result if the user was -// redirecting or piping stdout/stdin. So... we have this hack instead, which -// gets a correct result even on Windows and even with I/O redirection. - -// StateForAfterPanicWrap is part of the workaround for panicwrap that -// captures some characteristics of stderr that the caller can pass to the -// panicwrap child process somehow and then use ReinitInsidePanicWrap. -func (s *Streams) StateForAfterPanicWrap() *PrePanicwrapState { - return &PrePanicwrapState{ - StderrIsTerminal: s.Stderr.IsTerminal(), - StderrWidth: s.Stderr.Columns(), - } -} - -// ReinitInsidePanicwrap is part of the workaround for panicwrap that -// produces a Streams containing a potentially-lying Stderr that might -// claim to be a terminal even if it's actually a pipe connected to the -// parent process. -// -// That's an okay lie in practice because the parent process will copy any -// data it recieves via that pipe verbatim to the real stderr anyway. (The -// original call to Init in the parent process should've already done any -// necessary modesetting on the Stderr terminal, if any.) -// -// The state argument can be nil if we're not running in panicwrap mode, -// in which case this function behaves exactly the same as Init. -func ReinitInsidePanicwrap(state *PrePanicwrapState) (*Streams, error) { - ret, err := Init() - if err != nil { - return ret, err - } - if state != nil { - // A lying stderr, then. - ret.Stderr = &OutputStream{ - File: ret.Stderr.File, - isTerminal: func(f *os.File) bool { - return state.StderrIsTerminal - }, - getColumns: func(f *os.File) int { - return state.StderrWidth - }, - } - } - return ret, nil -} - -// PrePanicwrapState is a horrible thing we use to work around panicwrap, -// related to both Streams.StateForAfterPanicWrap and ReinitInsidePanicwrap. -type PrePanicwrapState struct { - StderrIsTerminal bool - StderrWidth int -} diff --git a/main.go b/main.go index 793db83c8..99e2c58a3 100644 --- a/main.go +++ b/main.go @@ -3,7 +3,6 @@ package main import ( "encoding/json" "fmt" - "io/ioutil" "log" "net" "os" @@ -24,7 +23,6 @@ import ( "github.com/mattn/go-shellwords" "github.com/mitchellh/cli" "github.com/mitchellh/colorstring" - "github.com/mitchellh/panicwrap" backendInit "github.com/hashicorp/terraform/internal/backend/init" ) @@ -35,12 +33,6 @@ const ( // The parent process will create a file to collect crash logs envTmpLogPath = "TF_TEMP_LOG_PATH" - - // Environment variable name used for smuggling true stderr terminal - // settings into a panicwrap child process. This is an implementation - // detail, subject to change in future, and should not ever be directly - // set by an end-user. - envTerminalPanicwrapWorkaround = "TF_PANICWRAP_STDERR" ) // ui wraps the primary output cli.Ui, and redirects Warn calls to Output @@ -54,67 +46,6 @@ func (u *ui) Warn(msg string) { u.Ui.Output(msg) } -func main() { - os.Exit(realMain()) -} - -func realMain() int { - var wrapConfig panicwrap.WrapConfig - - // don't re-exec terraform as a child process for easier debugging - if os.Getenv("TF_FORK") == "0" { - return wrappedMain() - } - - if !panicwrap.Wrapped(&wrapConfig) { - // We always send logs to a temporary file that we use in case - // there is a panic. Otherwise, we delete it. - logTempFile, err := ioutil.TempFile("", "terraform-log") - if err != nil { - fmt.Fprintf(os.Stderr, "Couldn't set up logging tempfile: %s", err) - return 1 - } - // Now that we have the file, close it and leave it for the wrapped - // process to write to. - logTempFile.Close() - defer os.Remove(logTempFile.Name()) - - // store the path in the environment for the wrapped executable - os.Setenv(envTmpLogPath, logTempFile.Name()) - - // We also need to do our terminal initialization before we fork, - // because the child process doesn't necessarily have access to - // the true stderr in order to initialize it. - streams, err := terminal.Init() - if err != nil { - fmt.Fprintf(os.Stderr, "Failed to initialize terminal: %s", err) - return 1 - } - - // We need the child process to behave _as if_ connected to the real - // stderr, even though panicwrap is about to add a pipe in the way, - // so we'll smuggle the true stderr information in an environment - // varible. - streamState := streams.StateForAfterPanicWrap() - os.Setenv(envTerminalPanicwrapWorkaround, fmt.Sprintf("%t:%d", streamState.StderrIsTerminal, streamState.StderrWidth)) - - // Create the configuration for panicwrap and wrap our executable - wrapConfig.Handler = logging.PanicHandler(logTempFile.Name()) - wrapConfig.IgnoreSignals = ignoreSignals - wrapConfig.ForwardSignals = forwardSignals - exitStatus, err := panicwrap.Wrap(&wrapConfig) - if err != nil { - fmt.Fprintf(os.Stderr, "Couldn't start Terraform: %s", err) - return 1 - } - - return exitStatus - } - - // Call the real main - return wrappedMain() -} - func init() { Ui = &ui{&cli.BasicUi{ Writer: os.Stdout, @@ -123,7 +54,11 @@ func init() { }} } -func wrappedMain() int { +func main() { + os.Exit(realMain()) +} + +func realMain() int { var err error tmpLogPath := os.Getenv(envTmpLogPath) @@ -145,19 +80,7 @@ func wrappedMain() int { log.Printf("[INFO] Go runtime version: %s", runtime.Version()) log.Printf("[INFO] CLI args: %#v", os.Args) - // This is the recieving end of our workaround to retain the metadata - // about the real stderr even though we're talking to it via the panicwrap - // pipe. See the call to StateForAfterPanicWrap above for the producer - // part of this. - var streamState *terminal.PrePanicwrapState - if raw := os.Getenv(envTerminalPanicwrapWorkaround); raw != "" { - streamState = &terminal.PrePanicwrapState{} - if _, err := fmt.Sscanf(raw, "%t:%d", &streamState.StderrIsTerminal, &streamState.StderrWidth); err != nil { - log.Printf("[WARN] %s is set but is incorrectly-formatted: %s", envTerminalPanicwrapWorkaround, err) - streamState = nil // leave it unset for a normal init, then - } - } - streams, err := terminal.ReinitInsidePanicwrap(streamState) + streams, err := terminal.Init() if err != nil { Ui.Error(fmt.Sprintf("Failed to configure the terminal: %s", err)) return 1 diff --git a/main_test.go b/main_test.go index 102ea44bb..15a09f283 100644 --- a/main_test.go +++ b/main_test.go @@ -130,7 +130,7 @@ func TestMain_cliArgsFromEnv(t *testing.T) { // Run it! os.Args = args testCommand.Args = nil - exit := wrappedMain() + exit := realMain() if (exit != 0) != tc.Err { t.Fatalf("bad: %d", exit) } @@ -237,7 +237,7 @@ func TestMain_cliArgsFromEnvAdvanced(t *testing.T) { // Run it! os.Args = args testCommand.Args = nil - exit := wrappedMain() + exit := realMain() if (exit != 0) != tc.Err { t.Fatalf("unexpected exit status %d; want 0", exit) } @@ -275,7 +275,7 @@ func TestMain_autoComplete(t *testing.T) { // Run it! os.Args = []string{"terraform", "terraform", "versio"} - exit := wrappedMain() + exit := realMain() if exit != 0 { t.Fatalf("unexpected exit status %d; want 0", exit) } From 42742c173df4c4472d5812f00f20ab31e47493ca Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 27 Oct 2021 10:28:19 -0400 Subject: [PATCH 2/8] remove wrapped streams and readline --- internal/command/console.go | 8 +- internal/command/console_interactive.go | 4 + .../helper/wrappedreadline/wrappedreadline.go | 77 ------------------- .../wrappedreadline/wrappedreadline_unix.go | 47 ----------- .../wrappedreadline_windows.go | 9 --- internal/helper/wrappedstreams/streams.go | 44 ----------- .../helper/wrappedstreams/streams_other.go | 22 ------ .../helper/wrappedstreams/streams_windows.go | 53 ------------- 8 files changed, 8 insertions(+), 256 deletions(-) delete mode 100644 internal/helper/wrappedreadline/wrappedreadline.go delete mode 100644 internal/helper/wrappedreadline/wrappedreadline_unix.go delete mode 100644 internal/helper/wrappedreadline/wrappedreadline_windows.go delete mode 100644 internal/helper/wrappedstreams/streams.go delete mode 100644 internal/helper/wrappedstreams/streams_other.go delete mode 100644 internal/helper/wrappedstreams/streams_windows.go diff --git a/internal/command/console.go b/internal/command/console.go index 319598868..0a3471bd2 100644 --- a/internal/command/console.go +++ b/internal/command/console.go @@ -3,11 +3,11 @@ package command import ( "bufio" "fmt" + "os" "strings" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/backend" - "github.com/hashicorp/terraform/internal/helper/wrappedstreams" "github.com/hashicorp/terraform/internal/repl" "github.com/hashicorp/terraform/internal/terraform" "github.com/hashicorp/terraform/internal/tfdiags" @@ -113,8 +113,8 @@ func (c *ConsoleCommand) Run(args []string) int { // Set up the UI so we can output directly to stdout ui := &cli.BasicUi{ - Writer: wrappedstreams.Stdout(), - ErrorWriter: wrappedstreams.Stderr(), + Writer: os.Stdout, + ErrorWriter: os.Stderr, } evalOpts := &terraform.EvalOpts{} @@ -164,7 +164,7 @@ func (c *ConsoleCommand) Run(args []string) int { func (c *ConsoleCommand) modePiped(session *repl.Session, ui cli.Ui) int { var lastResult string - scanner := bufio.NewScanner(wrappedstreams.Stdin()) + scanner := bufio.NewScanner(os.Stdin) for scanner.Scan() { result, exit, diags := session.Handle(strings.TrimSpace(scanner.Text())) if diags.HasErrors() { diff --git a/internal/command/console_interactive.go b/internal/command/console_interactive.go index 15fde13b4..32cc3a9ef 100644 --- a/internal/command/console_interactive.go +++ b/internal/command/console_interactive.go @@ -9,6 +9,7 @@ package command import ( "fmt" "io" + "os" "github.com/hashicorp/terraform/internal/repl" @@ -23,6 +24,9 @@ func (c *ConsoleCommand) modeInteractive(session *repl.Session, ui cli.Ui) int { InterruptPrompt: "^C", EOFPrompt: "exit", HistorySearchFold: true, + Stdin: os.Stdin, + Stdout: os.Stdout, + Stderr: os.Stderr, }) if err != nil { c.Ui.Error(fmt.Sprintf( diff --git a/internal/helper/wrappedreadline/wrappedreadline.go b/internal/helper/wrappedreadline/wrappedreadline.go deleted file mode 100644 index 6d2ffd15f..000000000 --- a/internal/helper/wrappedreadline/wrappedreadline.go +++ /dev/null @@ -1,77 +0,0 @@ -// wrappedreadline is a package that has helpers for interacting with -// readline from a panicwrap executable. -// -// panicwrap overrides the standard file descriptors so that the child process -// no longer looks like a TTY. The helpers here access the extra file descriptors -// passed by panicwrap to fix that. -// -// panicwrap should be checked for with panicwrap.Wrapped before using this -// librar, since this library won't adapt if the binary is not wrapped. -package wrappedreadline - -import ( - "runtime" - - "github.com/chzyer/readline" - - "github.com/hashicorp/terraform/internal/helper/wrappedstreams" -) - -// Override overrides the values in readline.Config that need to be -// set with wrapped values. -func Override(cfg *readline.Config) *readline.Config { - cfg.Stdin = wrappedstreams.Stdin() - cfg.Stdout = wrappedstreams.Stdout() - cfg.Stderr = wrappedstreams.Stderr() - - cfg.FuncGetWidth = TerminalWidth - cfg.FuncIsTerminal = IsTerminal - - rm := RawMode{StdinFd: int(wrappedstreams.Stdin().Fd())} - cfg.FuncMakeRaw = rm.Enter - cfg.FuncExitRaw = rm.Exit - - return cfg -} - -// IsTerminal determines if this process is attached to a TTY. -func IsTerminal() bool { - // Windows is always a terminal - if runtime.GOOS == "windows" { - return true - } - - // Same implementation as readline but with our custom fds - return readline.IsTerminal(int(wrappedstreams.Stdin().Fd())) && - (readline.IsTerminal(int(wrappedstreams.Stdout().Fd())) || - readline.IsTerminal(int(wrappedstreams.Stderr().Fd()))) -} - -// TerminalWidth gets the terminal width in characters. -func TerminalWidth() int { - if runtime.GOOS == "windows" { - return readline.GetScreenWidth() - } - - return getWidth() -} - -// RawMode is a helper for entering and exiting raw mode. -type RawMode struct { - StdinFd int - - state *readline.State -} - -func (r *RawMode) Enter() (err error) { - r.state, err = readline.MakeRaw(r.StdinFd) - return err -} - -func (r *RawMode) Exit() error { - if r.state == nil { - return nil - } - - return readline.Restore(r.StdinFd, r.state) -} diff --git a/internal/helper/wrappedreadline/wrappedreadline_unix.go b/internal/helper/wrappedreadline/wrappedreadline_unix.go deleted file mode 100644 index 25657f8c3..000000000 --- a/internal/helper/wrappedreadline/wrappedreadline_unix.go +++ /dev/null @@ -1,47 +0,0 @@ -//go:build darwin || dragonfly || freebsd || (linux && !appengine) || netbsd || openbsd -// +build darwin dragonfly freebsd linux,!appengine netbsd openbsd - -package wrappedreadline - -import ( - "syscall" - "unsafe" - - "github.com/hashicorp/terraform/internal/helper/wrappedstreams" -) - -// getWidth impl for Unix -func getWidth() int { - stdoutFd := int(wrappedstreams.Stdout().Fd()) - stderrFd := int(wrappedstreams.Stderr().Fd()) - - w := getWidthFd(stdoutFd) - if w < 0 { - w = getWidthFd(stderrFd) - } - - return w -} - -type winsize struct { - Row uint16 - Col uint16 - Xpixel uint16 - Ypixel uint16 -} - -// get width of the terminal -func getWidthFd(stdoutFd int) int { - ws := &winsize{} - retCode, _, errno := syscall.Syscall(syscall.SYS_IOCTL, - uintptr(stdoutFd), - uintptr(syscall.TIOCGWINSZ), - uintptr(unsafe.Pointer(ws))) - - if int(retCode) == -1 { - _ = errno - return -1 - } - - return int(ws.Col) -} diff --git a/internal/helper/wrappedreadline/wrappedreadline_windows.go b/internal/helper/wrappedreadline/wrappedreadline_windows.go deleted file mode 100644 index b06a60bf8..000000000 --- a/internal/helper/wrappedreadline/wrappedreadline_windows.go +++ /dev/null @@ -1,9 +0,0 @@ -//go:build windows -// +build windows - -package wrappedreadline - -// getWidth impl for other -func getWidth() int { - return 0 -} diff --git a/internal/helper/wrappedstreams/streams.go b/internal/helper/wrappedstreams/streams.go deleted file mode 100644 index 1ccc43973..000000000 --- a/internal/helper/wrappedstreams/streams.go +++ /dev/null @@ -1,44 +0,0 @@ -// Package wrappedstreams provides access to the standard OS streams -// (stdin, stdout, stderr) even if wrapped under panicwrap. -package wrappedstreams - -import ( - "os" - - "github.com/mitchellh/panicwrap" -) - -// Stdin returns the true stdin of the process. -func Stdin() *os.File { - stdin, _, _ := fds() - return stdin -} - -// Stdout returns the true stdout of the process. -func Stdout() *os.File { - _, stdout, _ := fds() - return stdout -} - -// Stderr returns the true stderr of the process. -func Stderr() *os.File { - _, _, stderr := fds() - return stderr -} - -func fds() (stdin, stdout, stderr *os.File) { - stdin, stdout, stderr = os.Stdin, os.Stdout, os.Stderr - if panicwrap.Wrapped(nil) { - initPlatform() - stdin, stdout, stderr = wrappedStdin, wrappedStdout, wrappedStderr - } - return -} - -// These are the wrapped standard streams. These are set up by the -// platform specific code in initPlatform. -var ( - wrappedStdin *os.File - wrappedStdout *os.File - wrappedStderr *os.File -) diff --git a/internal/helper/wrappedstreams/streams_other.go b/internal/helper/wrappedstreams/streams_other.go deleted file mode 100644 index 49dc0157c..000000000 --- a/internal/helper/wrappedstreams/streams_other.go +++ /dev/null @@ -1,22 +0,0 @@ -//go:build !windows -// +build !windows - -package wrappedstreams - -import ( - "os" - "sync" -) - -var initOnce sync.Once - -func initPlatform() { - // These must be initialized lazily, once it's been determined that this is - // a wrapped process. - initOnce.Do(func() { - // The standard streams are passed in via extra file descriptors. - wrappedStdin = os.NewFile(uintptr(3), "stdin") - wrappedStdout = os.NewFile(uintptr(4), "stdout") - wrappedStderr = os.NewFile(uintptr(5), "stderr") - }) -} diff --git a/internal/helper/wrappedstreams/streams_windows.go b/internal/helper/wrappedstreams/streams_windows.go deleted file mode 100644 index 0508cc8db..000000000 --- a/internal/helper/wrappedstreams/streams_windows.go +++ /dev/null @@ -1,53 +0,0 @@ -//go:build windows -// +build windows - -package wrappedstreams - -import ( - "log" - "os" - "syscall" -) - -func initPlatform() { - wrappedStdin = openConsole("CONIN$", os.Stdin) - wrappedStdout = openConsole("CONOUT$", os.Stdout) - wrappedStderr = wrappedStdout -} - -// openConsole opens a console handle, using a backup if it fails. -// This is used to get the exact console handle instead of the redirected -// handles from panicwrap. -func openConsole(name string, backup *os.File) *os.File { - // Convert to UTF16 - path, err := syscall.UTF16PtrFromString(name) - if err != nil { - log.Printf("[ERROR] wrappedstreams: %s", err) - return backup - } - - // Determine the share mode - var shareMode uint32 - switch name { - case "CONIN$": - shareMode = syscall.FILE_SHARE_READ - case "CONOUT$": - shareMode = syscall.FILE_SHARE_WRITE - } - - // Get the file - h, err := syscall.CreateFile( - path, - syscall.GENERIC_READ|syscall.GENERIC_WRITE, - shareMode, - nil, - syscall.OPEN_EXISTING, - 0, 0) - if err != nil { - log.Printf("[ERROR] wrappedstreams: %s", err) - return backup - } - - // Create the Go file - return os.NewFile(uintptr(h), name) -} From 3f31533f86a5b551d727c68547d722ecbf5f0792 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 27 Oct 2021 15:37:35 -0400 Subject: [PATCH 3/8] logging.PanicHandler can now be for internal use Repurpose logging.PanicHandler to recover internal panics and print a message to users similar to panicwrap. --- internal/logging/panic.go | 76 +++++++++------------------------- internal/logging/panic_test.go | 31 -------------- 2 files changed, 20 insertions(+), 87 deletions(-) diff --git a/internal/logging/panic.go b/internal/logging/panic.go index 211a1231d..207b3a444 100644 --- a/internal/logging/panic.go +++ b/internal/logging/panic.go @@ -2,14 +2,12 @@ package logging import ( "fmt" - "io" - "io/ioutil" "os" + "runtime/debug" "strings" "sync" "github.com/hashicorp/go-hclog" - "github.com/mitchellh/panicwrap" ) // This output is shown if a panic happens. @@ -18,59 +16,33 @@ const panicOutput = ` !!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!! Terraform crashed! This is always indicative of a bug within Terraform. -A crash log has been placed at %[1]q relative to your current -working directory. It would be immensely helpful if you could please -report the crash with Terraform[1] so that we can fix this. +Please report the crash with Terraform[1] so that we can fix this. -When reporting bugs, please include your terraform version. That -information is available on the first line of crash.log. You can also -get it by running 'terraform --version' on the command line. - -SECURITY WARNING: the %[1]q file that was created may contain -sensitive information that must be redacted before it is safe to share -on the issue tracker. +When reporting bugs, please include your terraform version, the stack trace +shown below, and any additional information which may help replicate the issue. [1]: https://github.com/hashicorp/terraform/issues !!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!! ` -// panicHandler is what is called by panicwrap when a panic is encountered -// within Terraform. It is guaranteed to run after the resulting process has -// exited so we can take the log file, add in the panic, and store it -// somewhere locally. -func PanicHandler(tmpLogPath string) panicwrap.HandlerFunc { - return func(m string) { - // Create the crash log file where we'll write the logs - f, err := ioutil.TempFile(".", "crash.*.log") - if err != nil { - fmt.Fprintf(os.Stderr, "Failed to create crash log file: %s", err) - return - } - defer f.Close() - - tmpLog, err := os.Open(tmpLogPath) - if err != nil { - fmt.Fprintf(os.Stderr, "Failed to open log file %q: %v\n", tmpLogPath, err) - return - } - defer tmpLog.Close() - - // Copy the contents to the crash file. This will include - // the panic that just happened. - if _, err = io.Copy(f, tmpLog); err != nil { - fmt.Fprintf(os.Stderr, "Failed to write crash log: %s", err) - return - } - - // add the trace back to the log - f.WriteString("\n" + m) - - // Tell the user a crash occurred in some helpful way that - // they'll hopefully notice. - fmt.Printf("\n\n") - fmt.Printf(panicOutput, f.Name()) +// PanicHandler is called to recover from an internal panic in Terraform, and +// is intended to replace the standard stack trace with a more user friendly +// error message. +// PanicHandler must be called as a defered function. +func PanicHandler() { + recovered := recover() + if recovered == nil { + return } + + fmt.Fprint(os.Stderr, panicOutput) + fmt.Fprint(os.Stderr, recovered) + + // When called from a deferred function, debug.PrintStack will include the + // full stack from the point of the pending panic. + debug.PrintStack() + os.Exit(2) } const pluginPanicOutput = ` @@ -181,13 +153,5 @@ func (l *logPanicWrapper) Debug(msg string, args ...interface{}) { l.panicRecorder(msg) } - // If we have logging turned on, we need to prevent panicwrap from seeing - // this as a core panic. This can be done by obfuscating the panic error - // line. - if panicPrefix { - colon := strings.Index(msg, ":") - msg = strings.ToUpper(msg[:colon]) + msg[colon:] - } - l.Logger.Debug(msg, args...) } diff --git a/internal/logging/panic_test.go b/internal/logging/panic_test.go index d2eb7a90b..e83a0ba5a 100644 --- a/internal/logging/panic_test.go +++ b/internal/logging/panic_test.go @@ -1,12 +1,9 @@ package logging import ( - "bytes" "fmt" "strings" "testing" - - "github.com/hashicorp/go-hclog" ) func TestPanicRecorder(t *testing.T) { @@ -52,31 +49,3 @@ func TestPanicLimit(t *testing.T) { } } } - -func TestLogPanicWrapper(t *testing.T) { - var buf bytes.Buffer - logger := hclog.NewInterceptLogger(&hclog.LoggerOptions{ - Name: "test", - Level: hclog.Debug, - Output: &buf, - DisableTime: true, - }) - - wrapped := (&logPanicWrapper{ - Logger: logger, - }).Named("test") - - wrapped.Debug("panic: invalid foo of bar") - wrapped.Debug("\tstack trace") - - expected := `[DEBUG] test.test: PANIC: invalid foo of bar -[DEBUG] test.test: stack trace -` - - got := buf.String() - - if expected != got { - t.Fatalf("Expected:\n%q\nGot:\n%q", expected, got) - } - -} From d03a037567b938169c2e5ec553cf706109b2697d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 27 Oct 2021 16:33:35 -0400 Subject: [PATCH 4/8] insert panic handlers --- internal/backend/local/backend.go | 2 ++ internal/backend/local/backend_apply.go | 2 ++ internal/backend/local/backend_plan.go | 2 ++ internal/backend/local/backend_refresh.go | 2 ++ internal/backend/remote/backend.go | 2 ++ internal/backend/remote/backend_common.go | 3 +++ internal/backend/remote/backend_plan.go | 3 +++ internal/command/login.go | 2 ++ internal/logging/panic.go | 7 +++---- internal/terraform/context.go | 3 +++ internal/terraform/graph.go | 5 +++++ main.go | 2 ++ 12 files changed, 31 insertions(+), 4 deletions(-) diff --git a/internal/backend/local/backend.go b/internal/backend/local/backend.go index f5d07b20f..dd6c9cc56 100644 --- a/internal/backend/local/backend.go +++ b/internal/backend/local/backend.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/terraform/internal/backend" "github.com/hashicorp/terraform/internal/command/views" "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/states/statemgr" "github.com/hashicorp/terraform/internal/terraform" "github.com/hashicorp/terraform/internal/tfdiags" @@ -313,6 +314,7 @@ func (b *Local) Operation(ctx context.Context, op *backend.Operation) (*backend. // Do it go func() { + defer logging.PanicHandler() defer done() defer stop() defer cancel() diff --git a/internal/backend/local/backend_apply.go b/internal/backend/local/backend_apply.go index 5b143a74f..897c36e40 100644 --- a/internal/backend/local/backend_apply.go +++ b/internal/backend/local/backend_apply.go @@ -7,6 +7,7 @@ import ( "github.com/hashicorp/terraform/internal/backend" "github.com/hashicorp/terraform/internal/command/views" + "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/states/statefile" @@ -156,6 +157,7 @@ func (b *Local) opApply( var applyDiags tfdiags.Diagnostics doneCh := make(chan struct{}) go func() { + defer logging.PanicHandler() defer close(doneCh) log.Printf("[INFO] backend/local: apply calling Apply") applyState, applyDiags = lr.Core.Apply(plan, lr.Config) diff --git a/internal/backend/local/backend_plan.go b/internal/backend/local/backend_plan.go index c721074b4..b27f98c68 100644 --- a/internal/backend/local/backend_plan.go +++ b/internal/backend/local/backend_plan.go @@ -6,6 +6,7 @@ import ( "log" "github.com/hashicorp/terraform/internal/backend" + "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/plans/planfile" "github.com/hashicorp/terraform/internal/states/statefile" @@ -79,6 +80,7 @@ func (b *Local) opPlan( var planDiags tfdiags.Diagnostics doneCh := make(chan struct{}) go func() { + defer logging.PanicHandler() defer close(doneCh) log.Printf("[INFO] backend/local: plan calling Plan") plan, planDiags = lr.Core.Plan(lr.Config, lr.InputState, lr.PlanOpts) diff --git a/internal/backend/local/backend_refresh.go b/internal/backend/local/backend_refresh.go index ddbedaf19..8ce3b6aff 100644 --- a/internal/backend/local/backend_refresh.go +++ b/internal/backend/local/backend_refresh.go @@ -7,6 +7,7 @@ import ( "os" "github.com/hashicorp/terraform/internal/backend" + "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/states/statemgr" "github.com/hashicorp/terraform/internal/tfdiags" @@ -77,6 +78,7 @@ func (b *Local) opRefresh( var refreshDiags tfdiags.Diagnostics doneCh := make(chan struct{}) go func() { + defer logging.PanicHandler() defer close(doneCh) newState, refreshDiags = lr.Core.Refresh(lr.Config, lr.InputState, lr.PlanOpts) log.Printf("[INFO] backend/local: refresh calling Refresh") diff --git a/internal/backend/remote/backend.go b/internal/backend/remote/backend.go index e427befa0..bc4c03175 100644 --- a/internal/backend/remote/backend.go +++ b/internal/backend/remote/backend.go @@ -18,6 +18,7 @@ import ( "github.com/hashicorp/terraform-svchost/disco" "github.com/hashicorp/terraform/internal/backend" "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/states/remote" "github.com/hashicorp/terraform/internal/states/statemgr" "github.com/hashicorp/terraform/internal/terraform" @@ -755,6 +756,7 @@ func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend // Do it. go func() { + defer logging.PanicHandler() defer done() defer stop() defer cancel() diff --git a/internal/backend/remote/backend_common.go b/internal/backend/remote/backend_common.go index 845402199..dc102f594 100644 --- a/internal/backend/remote/backend_common.go +++ b/internal/backend/remote/backend_common.go @@ -13,6 +13,7 @@ import ( tfe "github.com/hashicorp/go-tfe" "github.com/hashicorp/terraform/internal/backend" + "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/terraform" ) @@ -464,6 +465,8 @@ func (b *Remote) confirm(stopCtx context.Context, op *backend.Operation, opts *t result := make(chan error, 2) go func() { + defer logging.PanicHandler() + // Make sure we cancel doneCtx before we return // so the input command is also canceled. defer cancel() diff --git a/internal/backend/remote/backend_plan.go b/internal/backend/remote/backend_plan.go index 736c040b4..ca74d18b6 100644 --- a/internal/backend/remote/backend_plan.go +++ b/internal/backend/remote/backend_plan.go @@ -17,6 +17,7 @@ import ( tfe "github.com/hashicorp/go-tfe" version "github.com/hashicorp/go-version" "github.com/hashicorp/terraform/internal/backend" + "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/tfdiags" ) @@ -319,6 +320,8 @@ in order to capture the filesystem context the remote workspace expects: // cancellable after that period, we attempt to cancel it. if lockTimeout := op.StateLocker.Timeout(); lockTimeout > 0 { go func() { + defer logging.PanicHandler() + select { case <-stopCtx.Done(): return diff --git a/internal/command/login.go b/internal/command/login.go index 4ede60ddb..6b1d8bddd 100644 --- a/internal/command/login.go +++ b/internal/command/login.go @@ -22,6 +22,7 @@ import ( "github.com/hashicorp/terraform-svchost/disco" "github.com/hashicorp/terraform/internal/command/cliconfig" "github.com/hashicorp/terraform/internal/httpclient" + "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/terraform" "github.com/hashicorp/terraform/internal/tfdiags" @@ -450,6 +451,7 @@ func (c *LoginCommand) interactiveGetTokenByCode(hostname svchost.Hostname, cred }), } go func() { + defer logging.PanicHandler() err := server.Serve(listener) if err != nil && err != http.ErrServerClosed { diags = diags.Append(tfdiags.Sourceless( diff --git a/internal/logging/panic.go b/internal/logging/panic.go index 207b3a444..9ae6252ca 100644 --- a/internal/logging/panic.go +++ b/internal/logging/panic.go @@ -12,7 +12,6 @@ import ( // This output is shown if a panic happens. const panicOutput = ` - !!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!! Terraform crashed! This is always indicative of a bug within Terraform. @@ -24,11 +23,11 @@ shown below, and any additional information which may help replicate the issue. [1]: https://github.com/hashicorp/terraform/issues !!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!! + ` -// PanicHandler is called to recover from an internal panic in Terraform, and -// is intended to replace the standard stack trace with a more user friendly -// error message. +// PanicHandler is called to recover from an internal panic in Terraform, and +// augments the standard stack trace with a more user friendly error message. // PanicHandler must be called as a defered function. func PanicHandler() { recovered := recover() diff --git a/internal/terraform/context.go b/internal/terraform/context.go index 2174b0fe0..115f62276 100644 --- a/internal/terraform/context.go +++ b/internal/terraform/context.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" + "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/provisioners" "github.com/hashicorp/terraform/internal/states" @@ -264,6 +265,8 @@ func (c *Context) watchStop(walker *ContextGraphWalker) (chan struct{}, <-chan s done := c.runContext.Done() go func() { + defer logging.PanicHandler() + defer close(wait) // Wait for a stop or completion select { diff --git a/internal/terraform/graph.go b/internal/terraform/graph.go index 3d09f329b..9e2f19553 100644 --- a/internal/terraform/graph.go +++ b/internal/terraform/graph.go @@ -4,6 +4,7 @@ import ( "log" "strings" + "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/tfdiags" "github.com/hashicorp/terraform/internal/addrs" @@ -39,6 +40,10 @@ func (g *Graph) walk(walker GraphWalker) tfdiags.Diagnostics { // Walk the graph. walkFn := func(v dag.Vertex) (diags tfdiags.Diagnostics) { + // the walkFn is called asynchronously, and needs to be recovered + // separately in the case of a panic. + defer logging.PanicHandler() + log.Printf("[TRACE] vertex %q: starting visit (%T)", dag.VertexName(v), v) defer func() { diff --git a/main.go b/main.go index 99e2c58a3..da5dfd9c9 100644 --- a/main.go +++ b/main.go @@ -59,6 +59,8 @@ func main() { } func realMain() int { + defer logging.PanicHandler() + var err error tmpLogPath := os.Getenv(envTmpLogPath) From f031fcaa970bced527ecb9a465d3167b3a3b1f56 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 27 Oct 2021 17:12:20 -0400 Subject: [PATCH 5/8] no need for TF_FORK=0 --- scripts/debug-terraform | 5 ----- 1 file changed, 5 deletions(-) diff --git a/scripts/debug-terraform b/scripts/debug-terraform index a057c0d3c..f2dfae08c 100755 --- a/scripts/debug-terraform +++ b/scripts/debug-terraform @@ -18,11 +18,6 @@ set -eu -# Make sure we're debugging the process where the code is actually running. -# (This also, as a side effect, causes raw logs to go directly to stderr, -# and panics to be expressed directly, since we lose the log/panic wrapper.) -export TF_FORK=0 - echo "Launching Terraform in a headless debug session" echo "Connect to it using: dlv connect 127.0.0.1:2345" echo "(Terraform takes a long time to build and launch in this mode; some logs will appear below)" From d2d2508f5cb500432e72c510d0cee4f75e2216d0 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 27 Oct 2021 17:13:00 -0400 Subject: [PATCH 6/8] write provider panics to error We no longer have to hide these from panicwrap --- main.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/main.go b/main.go index da5dfd9c9..38c7141cc 100644 --- a/main.go +++ b/main.go @@ -316,9 +316,7 @@ func realMain() int { // plugins crashing if exitCode != 0 { for _, panicLog := range logging.PluginPanics() { - // we don't write this to Error, or else panicwrap will think this - // process panicked - Ui.Info(panicLog) + Ui.Error(panicLog) } } From bd37f43daa6bdea3ed24a9d33a97e7bf468afbbf Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 28 Oct 2021 13:56:18 -0400 Subject: [PATCH 7/8] add note about calling PanicHandler first --- internal/logging/panic.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/logging/panic.go b/internal/logging/panic.go index 9ae6252ca..2bc763f9d 100644 --- a/internal/logging/panic.go +++ b/internal/logging/panic.go @@ -28,7 +28,8 @@ shown below, and any additional information which may help replicate the issue. // PanicHandler is called to recover from an internal panic in Terraform, and // augments the standard stack trace with a more user friendly error message. -// PanicHandler must be called as a defered function. +// PanicHandler must be called as a defered function, and must be the first +// defer called at the start of a new goroutine. func PanicHandler() { recovered := recover() if recovered == nil { From 583e3a5f0b1978a73ba1b69c4d3ce963675a22c2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 28 Oct 2021 15:34:02 -0400 Subject: [PATCH 8/8] use 11 for the panic exit code --- internal/logging/panic.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/logging/panic.go b/internal/logging/panic.go index 2bc763f9d..785cbfb6f 100644 --- a/internal/logging/panic.go +++ b/internal/logging/panic.go @@ -42,7 +42,11 @@ func PanicHandler() { // When called from a deferred function, debug.PrintStack will include the // full stack from the point of the pending panic. debug.PrintStack() - os.Exit(2) + + // An exit code of 11 keeps us out of the way of the detailed exitcodes + // from plan, and also happens to be the same code as SIGSEGV which is + // roughly the same type of condition that causes most panics. + os.Exit(11) } const pluginPanicOutput = `