pkg/cli/image/extract: disable pigz to prevent race condition #104
Conversation
openshift-ci-robot
added
the
size/XS
There is a race condition in the vendored version of docker code that can cause image extraction to panic. When the `pigz` package is installed, docker's DecompressStream code prefers to use it, however, that code can return the io buffer to the pool while the command is still writing to it. If the buffer is reused while that's happening, `oc` will panic. There are many reports of this happening. As the vendored docker comes from kubectl, and the version there (even for k8s 1.16) is quite old, this disables using pigz at all by setting MOBY_DISABLE_PIGZ environment variable.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: soltysh, stbenjam The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/test e2e-aws |
| @@ -483,6 +483,10 @@ func (o *Options) Run() error { | |||
| } | |||
|
|
|||
| func layerByEntry(r io.Reader, options *archive.TarOptions, layerInfo LayerInfo, fn TarEntryFunc, allLayers bool, alreadySeen map[string]struct{}) (bool, error) { | |||
| // Prevents race condition present in vendored version of docker | |||
| // https://github.com/moby/moby/issues/39859 | |||
| os.Setenv("MOBY_DISABLE_PIGZ", "true") | |||
There was a problem hiding this comment.
This may introduce new race conditions; changing the environment in a multi-threaded process can create undefined behavior; see:
There was a problem hiding this comment.
The situation is certainly better with this fix that it was before this, the panics were unbearable. Would a mutex around this be sufficient to mitigate the risk or is that not sufficient? I think with pigz disabled we’re not shelling out for anything
There was a problem hiding this comment.
From a quick look, golang has a mutex around os.{G,S}etenv - so if there's no C threads running, it's generally OK.
But, it'd be safer to hoist this to early in main().
There was a problem hiding this comment.
Good point, it's also more efficient, no need to keep calling it. The only reason I did it in layerByEntry was so it was close to the image extraction code, I was worried if I drop this in main it'll get forgotten and once we have a newer docker, we'll lose out on the performance improvements of using pigz (not that I've benchmarked it to know that it's actually better).
Anyway, I proposed #130
There is a race condition in the vendored version of docker code that
can cause image extraction to panic. When the
pigzpackage isinstalled, docker's DecompressStream code prefers to use it, however,
that code can return the io buffer to the pool while the command is
still writing to it. If the buffer is reused while that's happening,
ocwill panic. There are many reports of this happening.As the vendored docker comes from kubectl, and the version there (even
for k8s 1.16) is quite old, this disables using pigz at all by setting
MOBY_DISABLE_PIGZ environment variable.
fixes #58