search

mitchellh / panicwrap

panicwrap is a Go library for catching and handling panics in Go applications.
https://gist.github.com/mitchellh/90029601268e59a29e64e55bab1c5bdc
MIT License
444 stars 66 forks source link

Prevent false positives if, after a relaunch, the program tries to execute itself independently of panicwrap #25

Closed DaemonSnake closed 5 years ago

DaemonSnake commented 5 years ago

Hi,

An issue as arised from Terraform use of panicwrap.

If, after a relaunch and panicwrap's setup, the application tries to execute itself recursively, as the cookie env variable will still be set, Wrapped will return true in this new process.

This is an issue for Terraform as they add prefix to the outputs when Wrapped yield true causing bad outputs.

To solve this I've made it that after we detect that we are indeed in a wrapped process using the env variable, we unset it.

Do you agree with the proposed solution?

You can find the Terraform issue and conversation here https://github.com/hashicorp/terraform/issues/20293

mitchellh commented 5 years ago

Hey thanks. The core logic here makes total sense to me. I think this can be tested with a combination of helperProcess usage. I'd be happy to look into that if you're not comfortable but it will slow down merging of this.

DaemonSnake commented 5 years ago

Thanks for the quick return. I've corrected the tests to reflect the change and added a new test

mitchellh commented 5 years ago

Hey the test looks good but when I check it out and run the test WITHOUT your change, it still passes. :) So somehow I think its not testing anything. I didn't dig into why yet...

DaemonSnake commented 5 years ago

Hi, I've found the issue and fixed the commit. I was calling Wrapped(nil) before Wrap(config) which always returns false.

mitchellh commented 5 years ago

Thank you works great!