[dockerfile2llb] fix daemon crash: check for circular dependency in convert by Blyschak · Pull Request #999 · moby/buildkit

Blyschak · 2019-05-10T15:55:56Z

Signed-off-by: Stepan Blyshchak stepanblischak@gmail.com

By creating a simple dockerfile like this:

FROM alpine:latest AS stage0
FROM alpine:latest AS stage1
COPY --from=stage1 bin /

and building using following command:
DOCKER_BUILDKIT=1 /usr/bin/docker build -t test .

dockerd crashes (part of log):

runtime: goroutine stack exceeds 1000000000-byte limit
fatal error: stack overflow

runtime stack:
runtime.throw(0x1d33169, 0xe)
        /usr/local/go/src/runtime/panic.go:617 +0x74
runtime.newstack()
        /usr/local/go/src/runtime/stack.go:1041 +0x6f4
runtime.morestack()
        /usr/local/go/src/runtime/asm_amd64.s:429 +0x84

goroutine 677 [running]:
github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.isReachable(0x0, 0xc0008b8900, 0xffffffffffffffff)
        /go/src/github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb/convert.go:1118 +0x15c fp=0xc024000360 sp=0xc024000358 pc=0x161990c
github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.isReachable(0xc0008b8c00, 0xc0008b8900, 0xc024000400)
        /go/src/github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb/convert.go:1122 +0x128 fp=0xc0240003e8 sp=0xc024000360 pc=0x16198d8
github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.isReachable(0xc0008b8c00, 0xc0008b8900, 0xc024000488)
        /go/src/github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb/convert.go:1126 +0xcd fp=0xc024000470 sp=0xc0240003e8 pc=0x161987d
github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.isReachable(0xc0008b8c00, 0xc0008b8900, 0xc024000510)
        /go/src/github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb/convert.go:1126 +0xcd fp=0xc0240004f8 sp=0xc024000470 pc=0x161987d
github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.isReachable(0xc0008b8c00, 0xc0008b8900, 0xc024000598)
        /go/src/github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb/convert.go:1126 +0xcd fp=0xc024000580 sp=0xc0240004f8 pc=0x161987d

Interestingly that dockerfile:

FROM alpine:latest AS stage1
COPY --from=stage2 bin /

FROM alpine:latest AS stage2
COPY --from=stage1 bin /

does not crashes that daemon however there is a cyclic dependency on stages and IMO resulted image is undefined.
Lagacy builder with same dockerfile fails with a another error:

root@819191141f53:~/test# /usr/bin/docker build -t test .
Sending build context to Docker daemon  2.048kB
Step 1/4 : FROM alpine:latest AS stage1
 ---> cdf98d1859c1
Step 2/4 : COPY --from=stage2 bin /
invalid from flag value stage2: pull access denied for stage2, repository does not exist or may require 'docker login': denied: requested access to the resource is denied

I have added a check for circular dependency after convert fills all dependencies in, so now I get:

root@819191141f53:~/test# cat Dockerfile
FROM alpine:latest AS stage1
COPY --from=stage2 bin /

FROM alpine:latest AS stage2
COPY --from=stage1 bin /

root@819191141f53:~/test# DOCKER_BUILDKIT=1 /usr/bin/docker build -t test .
[+] Building 0.2s (2/2) FINISHED
 => [internal] load build definition from Dockerfile                                                           0.1s
 => => transferring dockerfile: 31B                                                                            0.0s
 => [internal] load .dockerignore                                                                              0.1s
 => => transferring context: 2B                                                                                0.0s
failed to create LLB definition: circular dependency detected on stage: stage1

AkihiroSuda · 2019-05-11T01:43:30Z

#12 [4/4] RUN --mount=target=/go/src/github.com/moby/buildkit 	gometalinter ...
#12 0.785 frontend/dockerfile/dockerfile2llb/convert.go:1::warning: file is not gofmted with -s (gofmt)
#12 1.955 frontend/dockerfile/dockerfile2llb/convert.go:1::warning: file is not goimported (goimports)
#12 ERROR: executor failed running [/bin/sh -c gometalinter --config=gometalinter.json ./...]: exit code: 1

AkihiroSuda · 2019-05-11T01:44:01Z

 	return false
 }

+func hasCircularDependency(states []*dispatchState) (bool, *dispatchState) {


Can we have UT?

sure, added "TestDockerfileCircularDependencies"

AkihiroSuda · 2019-05-11T01:45:16Z

+       visited := make(map[*dispatchState]struct{})
+       path := make(map[*dispatchState]struct{})
+
+       visit = func (state *dispatchState) bool {


nit: can be visit := ...

visit is called recursively, I get "undefined: visit" when using " := "; this is the go's order of evaluation

Signed-off-by: Stepan Blyshchak <stepanblischak@gmail.com>

… dependency To have stageName in error output in case one stage depends on itself Signed-off-by: Stepan Blyshchak <stepanblischak@gmail.com>

Signed-off-by: Stepan Blyshchak <stepanblischak@gmail.com>

tonistiigi · 2019-05-12T03:37:35Z

Thanks @Blyschak

AkihiroSuda · 2019-05-12T11:19:03Z

This should be back ported to Docker 18.09?

Blyschak · 2019-05-16T15:57:39Z

@AkihiroSuda I have no specific wish to back port it to 18.09, so only if you want

AkihiroSuda reviewed May 11, 2019

View reviewed changes

Blyschak added 4 commits May 11, 2019 18:25

[dockerfile2llb] check for circular dependency in convert

ad4ae59

Signed-off-by: Stepan Blyshchak <stepanblischak@gmail.com>

apply gofmt

98a6ff4

Signed-off-by: Stepan Blyshchak <stepanblischak@gmail.com>

[convert] move 'stageName = ""' in covert after checking for circular…

981ee38

… dependency To have stageName in error output in case one stage depends on itself Signed-off-by: Stepan Blyshchak <stepanblischak@gmail.com>

[convert_test] add ut for circular dependency cases in dockerfile

ea4bb02

Signed-off-by: Stepan Blyshchak <stepanblischak@gmail.com>

Blyschak force-pushed the fix-inf-recursion branch from 1e6367b to ea4bb02 Compare May 11, 2019 16:49

AkihiroSuda approved these changes May 12, 2019

View reviewed changes

tonistiigi approved these changes May 12, 2019

View reviewed changes

tonistiigi merged commit 255defe into moby:master May 12, 2019

Blyschak deleted the fix-inf-recursion branch May 12, 2019 10:43

tonistiigi mentioned this pull request May 13, 2019

vendor: update buildkit to f238f1ef moby/moby#39210

Merged

thaJeztah mentioned this pull request May 14, 2019

[19.03 backport] BuildKit fixes docker-archive/engine#215

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dockerfile2llb] fix daemon crash: check for circular dependency in convert#999

[dockerfile2llb] fix daemon crash: check for circular dependency in convert#999
tonistiigi merged 4 commits intomoby:masterfrom
Blyschak:fix-inf-recursion

Blyschak commented May 10, 2019

Uh oh!

AkihiroSuda commented May 11, 2019

Uh oh!

AkihiroSuda May 11, 2019

Uh oh!

Blyschak May 11, 2019

Uh oh!

AkihiroSuda May 11, 2019

Uh oh!

Blyschak May 11, 2019

Uh oh!

tonistiigi commented May 12, 2019

Uh oh!

AkihiroSuda commented May 12, 2019

Uh oh!

Blyschak commented May 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Blyschak commented May 10, 2019

Uh oh!

AkihiroSuda commented May 11, 2019

Uh oh!

AkihiroSuda May 11, 2019

Choose a reason for hiding this comment

Uh oh!

Blyschak May 11, 2019

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda May 11, 2019

Choose a reason for hiding this comment

Uh oh!

Blyschak May 11, 2019

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented May 12, 2019

Uh oh!

AkihiroSuda commented May 12, 2019

Uh oh!

Blyschak commented May 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants