Skip to content

layer/layer_store: ensure NewInputTarStream resources are released#38413

Merged
yongtang merged 1 commit into
moby:masterfrom
slp:master
Dec 23, 2018
Merged

layer/layer_store: ensure NewInputTarStream resources are released#38413
yongtang merged 1 commit into
moby:masterfrom
slp:master

Conversation

@slp
Copy link
Copy Markdown

@slp slp commented Dec 21, 2018

In applyTar, if the driver's ApplyDiff returns an error, the function
returns early without calling io.Copy.

As a consequence, the resources (a goroutine and some buffers holding
the uncompressed image, the digest, etc...) allocated or referenced by
NewInputTarStream above aren't released, as the worker goroutine only
finishes when it finds EOF or a closed pipe.

Signed-off-by: Sergio Lopez [email protected]

- What I did
I've changed layer/layer_store.go:applyTar to ensure the resources allocated by NewInputTarStream are always released.

- How I did it
By moving the call to io.Copy to put it before the place where the ApplyDiff error is checked, to ensure is unconditionally called.

- How to verify it
I'm using dlv to simulate the issue:

[root@localhost binary-daemon]# dlv exec ./dockerd-dev
Type 'help' for list of commands.
(dlv) break applyTar
Breakpoint 1 set at 0xee94da for github.com/docker/docker/layer.(*layerStore).applyTar() /go/src/github.com/docker/docker/layer/layer_store.go:234
(dlv) c

(another terminal)

[root@localhost ~]# docker run --rm -ti busybox /bin/sh
Unable to find image 'busybox:latest' locally
latest: Pulling from library/busybox
90e01955edcd: Extracting    728kB/728kB

(back to dlv)

> github.com/docker/docker/layer.(*layerStore).applyTar() /go/src/github.com/docker/docker/layer/layer_store.go:234 (hits goroutine(142):1 total:1) (PC: 0xee94da)
Warning: debugging optimized function
   229:		ls.mounts[ml.name] = ml
   230:	
   231:		return nil
   232:	}
   233:	
=> 234:	func (ls *layerStore) applyTar(tx *fileMetadataTransaction, ts io.Reader, parent string, layer *roLayer) error {
   235:		digester := digest.Canonical.Digester()
   236:		tr := io.TeeReader(ts, digester.Hash())
   237:	
   238:		rdr := tr
   239:		if ls.useTarSplit {
(dlv) n
(...)
> github.com/docker/docker/layer.(*layerStore).applyTar() /go/src/github.com/docker/docker/layer/layer_store.go:255 (PC: 0xee95d6)
Warning: debugging optimized function
   250:			if err != nil {
   251:				return err
   252:			}
   253:		}
   254:	
=> 255:		applySize, err := ls.driver.ApplyDiff(layer.cacheID, parent, rdr)
   256:		if err != nil {
   257:			return err
   258:		}
   259:	
   260:		// Discard trailing data but ensure metadata is picked up to reconstruct stream
(dlv) p layer.cacheID
"b4e6ddae7f5cace7bc165d0bdfe3efcc1650eb61d335806ea293e61391910b49"

(just another terminal more)

[root@localhost moby]# # Make sure extracting the tar will fail...
[root@localhost moby]# mount -t tmpfs -o ro tmpfs /var/lib/docker/overlay2/b4e6ddae7f5cace7bc165d0bdfe3efcc1650eb61d335806ea293e61391910b49

(back to dlv)

(dlv) c
INFO[2018-12-21T09:56:56.454866823+01:00] Attempting next endpoint for pull after error: failed to register layer: mkdir /var/lib/docker/overlay2/b4e6ddae7f5cace7bc165d0bdfe3efcc1650eb61d335806ea293e61391910b49/diff: read-only file system 
(dlv) goroutine 196
Switched from 0 to 196 (thread 15923)
(dlv) bt
 0  0x000000000042f821 in runtime.gopark
    at /usr/local/go/src/runtime/proc.go:303
 1  0x000000000043fb3c in runtime.selectgo
    at /usr/local/go/src/runtime/select.go:313
 2  0x000000000046a78e in io.(*pipe).Write
    at /usr/local/go/src/io/pipe.go:87
 3  0x000000000046ac3e in io.(*PipeWriter).Write
    at /usr/local/go/src/io/pipe.go:153
 4  0x00000000004695c3 in io.(*teeReader).Read
    at /usr/local/go/src/io/io.go:537
 5  0x00000000004688ba in io.ReadAtLeast
    at /usr/local/go/src/io/io.go:310
 6  0x0000000000468a5a in io.ReadFull
    at /usr/local/go/src/io/io.go:329
 7  0x0000000000edc99e in github.com/docker/docker/vendor/github.com/vbatts/tar-split/archive/tar.(*Reader).readHeader
    at /go/src/github.com/docker/docker/vendor/github.com/vbatts/tar-split/archive/tar/reader.go:389
 8  0x0000000000edab02 in github.com/docker/docker/vendor/github.com/vbatts/tar-split/archive/tar.(*Reader).next
    at /go/src/github.com/docker/docker/vendor/github.com/vbatts/tar-split/archive/tar/reader.go:111
 9  0x0000000000eda894 in github.com/docker/docker/vendor/github.com/vbatts/tar-split/archive/tar.(*Reader).Next
    at /go/src/github.com/docker/docker/vendor/github.com/vbatts/tar-split/archive/tar/reader.go:74
10  0x0000000000ee3c6e in github.com/docker/docker/vendor/github.com/vbatts/tar-split/tar/asm.NewInputTarStream.func1
    at /go/src/github.com/docker/docker/vendor/github.com/vbatts/tar-split/tar/asm/disassemble.go:49
11  0x000000000045e561 in runtime.goexit
    at /usr/local/go/src/runtime/asm_amd64.s:1333

Without the patch, we get a stuck goroutine as shown above. With it, we get the same error, but without the stuck goroutine.

- Description for the changelog
layer/layer_store: ensure NewInputTarStream resources are released

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

image

@slp
layer/layer_store: ensure NewInputTarStream resources are released
5846db1 
In applyTar, if the driver's ApplyDiff returns an error, the function
returns early without calling io.Copy.

As a consequence, the resources (a goroutine and some buffers holding
the uncompressed image, the digest, etc...) allocated or referenced by
NewInputTarStream above aren't released, as the worker goroutine only
finishes when it finds EOF or a closed pipe.

Signed-off-by: Sergio Lopez <[email protected]>
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 21, 2018

Codecov Report

Merging #38413 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #38413      +/-   ##
==========================================
- Coverage   36.58%   36.58%   -0.01%     
==========================================
  Files         608      608              
  Lines       44988    44988              
==========================================
- Hits        16458    16457       -1     
- Misses      26250    26251       +1     
  Partials     2280     2280

@kolyshkin
Copy link
Copy Markdown
Contributor

kolyshkin commented Dec 22, 2018

LGTM 🐳

thaJeztah
thaJeztah approved these changes Dec 22, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great catch; thanks!

yongtang
yongtang approved these changes Dec 23, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@yongtang yongtang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yongtang yongtang merged commit d147fe0 into moby:master Dec 23, 2018
@thaJeztah thaJeztah mentioned this pull request Mar 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@yongtang yongtang yongtang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@slp @kolyshkin @thaJeztah @yongtang @GordonTheTurtle