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

NPE in RunAndWait in 2.5.0 #255

Closed
tgross opened this issue Dec 1, 2016 · 2 comments
Closed

NPE in RunAndWait in 2.5.0 #255

tgross opened this issue Dec 1, 2016 · 2 comments

Comments

@tgross
Copy link
Contributor

tgross commented Dec 1, 2016

Got the following panic which didn't show up in our integration tests:

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x4c90e7]

goroutine 1 [running]:
panic(0xb50160, 0xc820018080)
        /usr/local/go/src/runtime/panic.go:481 +0x3e6
github.com/joyent/containerpilot/commands.RunAndWait(0xc820180180, 0xc8201974d0, 0xc820189e20, 0x0, 0x0)
        /cp/src/github.com/joyent/containerpilot/commands/commands.go:97 +0xb87
github.com/joyent/containerpilot/core.(*App).Run(0xc82017c0e0)
        /cp/src/github.com/joyent/containerpilot/core/app.go:147 +0x37e
main.main()
        /cp/src/github.com/joyent/containerpilot/main.go:25 +0xf1

The relevant section appears to be here in RunAndWait where we're incorrectly assuming state is never nil if we get an non-nil err (which seemed reasonable but wasn't documented and turns out to be wrong). We want to capture the state if it's available but otherwise return the error without getting the state. Fix for this should be:

	state, err := c.Cmd.Process.Wait()
	if state != nil && !state.Success()) {
		if status, ok := state.Sys().(syscall.WaitStatus); ok {
			return status.ExitStatus(), err
		}
	}
	if err != nil {
		return 1, err
	}
@tgross tgross added the bug label Dec 1, 2016
@tgross
Copy link
Contributor Author

tgross commented Dec 1, 2016

I'm adding some test coverage improvements and pruning an unused branch of the error handling in the process here, but should have a PR up later today.

@tgross
Copy link
Contributor Author

tgross commented Dec 1, 2016

Opened #256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant