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

archive: fix race condition in cmdStream #39860

Merged
merged 1 commit into from Sep 12, 2019

Conversation

@stbenjam
Copy link
Contributor

commented Sep 3, 2019

- What I did

Fixed a race condition, see details in issue #39859

- How I did it

Ensured that cmd.Wait is done, before any clean up happens.

- How to verify it

There is a reproducer at https://github.com/stbenjam/docker-race-reproducer. Run the code as-is, and then apply the changes in this PR to vendor/github.com/docker/docker/pkg/archive/archive.go. You'll see the issue is fixed.

- Description for the changelog

Fixed a race condition in cmdStream to ensure that we wait for the command to exit before any clean up.

- A picture of a cute animal (not mandatory but encouraged)

My dog, Tilly:

dogTilly

@GordonTheTurtle

This comment has been minimized.

Copy link

commented Sep 3, 2019

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "cmd-race" git@github.com:stbenjam/moby.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@@ -1218,16 +1219,27 @@ func cmdStream(cmd *exec.Cmd, input io.Reader) (io.ReadCloser, error) {
return nil, err
}

// Ensure the command has exited before we clean anything up
var wg sync.WaitGroup
wg.Add(1)

This comment has been minimized.

Copy link
@tiborvass

tiborvass Sep 10, 2019

Collaborator

@stbenjam I personally use WaitGroups when there's more than one thing to wait on, otherwise I just close a chan struct{}, but no big deal. Otherwise PR looks good to me.

This comment has been minimized.

Copy link
@stbenjam

stbenjam Sep 11, 2019

Author Contributor

Sure, no problem, I suppose that's more efficient. I updated it to use chan struct{}.

@tiborvass

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

Another way to find the race is to run go test -race ./pkg/archive. We could/should run unit tests with -race as well.

There is a race condition in pkg/archive when using `cmd.Start` for pigz
and xz where the `*bufio.Reader` could be returned to the pool while the
command is still writing to it, and then picked up and used by a new
command.

The command is wrapped in a `CommandContext` where the process will be
killed when the context is cancelled, however this is not instantaneous,
so there's a brief window while the command is still running but the
`*bufio.Reader` was already returned to the pool.

wrapReadCloser calls `cancel()`, and then `readBuf.Close()` which
eventually returns the buffer to the pool. However, because cmdStream
runs `cmd.Wait` in a go routine that we never wait for to finish, it is
not safe to return the reader to the pool yet.  We need to ensure we
wait for `cmd.Wait` to finish!

Signed-off-by: Stephen Benjamin <stephen@redhat.com>
@stbenjam stbenjam force-pushed the stbenjam:cmd-race branch from 70ccc74 to 89dd10b Sep 11, 2019
@thaJeztah

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@tiborvass @kolyshkin PR was updated; PTAL

Copy link
Contributor

left a comment

LGTM

Copy link
Contributor

left a comment

LGTM

@cpuguy83 cpuguy83 merged commit 0c099fa into moby:master Sep 12, 2019
3 checks passed
3 checks passed
DCO DCO
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
dco-signed All commits are signed
@stbenjam stbenjam deleted the stbenjam:cmd-race branch Sep 12, 2019
@tiborvass

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2019

FYI a followup to this PR is in #39920

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.