Fix race checking for process exit and waiting for exec fifo

[liggitt]

Fixes #2183, a race condition in checking for a dead process while waiting for the exec fifo to open:

• Moves the wait for the blocking fifo open (happy path) and the 100ms poll for a missing/zombie process into the same loop
• When the missing/zombie process check fires, does a non-blocking open/read of the fifo to see if the process already completed, and only returns the "container process is already dead" error if that fails.

To test this, I inserted a synthetic 150ms delay in the awaitFifoOpen path after opening the fifo (simulating a slow send on the fifoOpened channel losing the race to the 100ms dead process detection timeout), and the integration suite passed (this PR includes #2186 which fixes an existing race condition in one of the integration tests).

diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
index bec72def..a5a4d1ad 100644
--- a/libcontainer/container_linux.go
+++ b/libcontainer/container_linux.go
@@ -301,6 +301,8 @@ func awaitFifoOpen(path string) <-chan openResult {
        fifoOpened := make(chan openResult)
        go func() {
                result := fifoOpen(path, true)
+               // simulate a slow send on fifoOpened, racing with the 100ms dead process detection timeout
+               time.Sleep(150 * time.Millisecond)
                fifoOpened <- result
        }()
        return fifoOpened

When I insert the same simulated race in master, 14 integration tests fail with "container is already dead" errors.

Still working out a way to unit/integration test this without modifying non-test code like that.

cc @Random-Liu

[liggitt]

cc @crosbymichael @mrunalp as approvers on the introducing PR (#1698)

[mrunalp]

I am away this week and have only phone access. This looks fine from a quick look but would let other maintainers review it deeper. Thanks!

[Random-Liu]

Adding a check after detecting the process is gone sounds good.

[crosbymichael]

LGTM

[liggitt]

cc @cyphar since @mrunalp is OOO

[mrunalp]

LGTM

[liggitt]

Thank you all for the reviews. I'm working on getting this propagated into containerd/docker versions we use for CI in Kubernetes, since our test scenarios trigger this bug frequently. Is there an existing timeframe for the next tagged version of runc, or is that done on an as-needed basis?

[cyphar]

Is there an existing timeframe for the next tagged version of runc, or is that done on an as-needed basis?

It's mostly done on an as-needed basis, but right now we are all waiting (with bated breath) for opencontainers/runtime-spec#1008 to be merged so that runc can finally have a 1.0 release and we can switch to a more periodic release schedule. If you need a 1.0-rc10 we can do that, but right now we need to also deal with CVE-2019-19921.

[liggitt]

That context is helpful, thanks. I don't have a good sense for how much work opencontainers/runtime-spec#1008 will require in this repo before a 1.0 tag could be cut, or from downstream consumers before that 1.0 tag could be consumed... if it's O(weeks), an earlier RC tag would be appreciated if possible.

[cyphar]

It will definitely be O(weeks) -- especially given the time of year. In that case, I will first try to get a fix for CVE-2019-19921 merged first (probably closer to a week unless we have long arguments about the solution) and then do an rc10 release.

[liggitt]

Thanks, much appreciated

[h-vetinari]

@cyphar: It's mostly done on an as-needed basis, but right now we are all waiting (with bated breath) for opencontainers/runtime-spec#1008 to be merged so that runc can finally have a 1.0 release and we can switch to a more periodic release schedule. If you need a 1.0-rc10 we can do that, but right now we need to also deal with CVE-2019-19921.

Considering that it's taken 2 years to sort out the hooks (more or less), shouldn't there be an 1.0-rc11 with the new hooks (once landed in spec and runc), to weed out potential implementation errors, see how the ecosystem can move away from the now-deprecated hooks to the new ones, and get feedback from other runtime authors? What's 2 additional month in this whole saga? ;)

[cyphar]

shouldn't there be an 1.0-rc11 with the new hooks (once landed in spec and runc), to weed out potential implementation errors, see how the ecosystem can move away from the now-deprecated hooks to the new ones, and get feedback from other runtime authors?

Yeah, that's mostly likely what we'll end up doing.

What's 2 additional month in this whole saga? ;)

😭 😭 😭 😭

[liggitt]

@cyphar any estimate on getting this in a tagged release?

[liggitt]

from #2185 (comment)

I will first try to get a fix for CVE-2019-19921 merged first (probably closer to a week unless we have long arguments about the solution) and then do an rc10 release.

For anyone following along, it looks like that is being discussed/tracked in #2197 (comment)

[tedyu]

I wonder if this deferred close would function correctly w.r.t. the os.Remove call a few lines down.
On windows, we cannot rename/delete/move a file with open file handles.

Should the code be refactored so that this works on all platforms ?

Thanks

[liggitt]

This is moving existing code that was previously on line 279. It is also explicitly in a file that it only runs on Linux.

[tedyu]

Should have placed my previous comment on the original code.