Avoid nil pointer dereference when copying from image with no layers #2197
Conversation
Fix this panic when copying from an image with no layers: ``` panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x50 pc=0xdd8c17] goroutine 326 [running]: github.com/moby/buildkit/cache/contenthash.(*cacheManager).Checksum(0xc0005ec030, 0x1682c00, 0xc000842140, 0x0, 0x0, 0xc0005d4023, 0x1, 0x0, 0x0, 0x0, ...) /src/cache/contenthash/checksum.go:95 +0x37 github.com/moby/buildkit/cache/contenthash.Checksum(0x1682c00, 0xc000842140, 0x0, 0x0, 0xc0005d4023, 0x1, 0x0, 0x0, 0x0, 0x0, ...) /src/cache/contenthash/checksum.go:59 +0xd5 github.com/moby/buildkit/solver/llbsolver.NewContentHashFunc.func1.1(0x0, 0x4425d6) /src/solver/llbsolver/result.go:59 +0x20a golang.org/x/sync/errgroup.(*Group).Go.func1(0xc00056a360, 0xc000594510) /src/vendor/golang.org/x/sync/errgroup/errgroup.go:57 +0x59 created by golang.org/x/sync/errgroup.(*Group).Go /src/vendor/golang.org/x/sync/errgroup/errgroup.go:54 +0x66 ``` When the path is "/", we allow it because it's a noop. Based on moby#2185 Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
|
We should add tests for copying from scratch/image with no layers and probably also the same for the mount sources. |
|
I don't understand the part about mount sources. |
Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
|
Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
Added |
cache/contenthash/checksum.go
Outdated
| if p == "/" { | ||
| return digest.FromBytes(nil), nil | ||
| } | ||
| return "", errors.Errorf("cannot checksum empty reference") |
There was a problem hiding this comment.
nit: probably better to return the same "not found" error that would happen if p does not exist. "empty reference" is an internal concept, users of build don't know what it means
client/client_test.go
Outdated
|
|
||
| _, err = c.Solve(sb.Context(), def, SolveOpt{}, nil) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "/foo: no such file or directory") |
There was a problem hiding this comment.
regarding errors, if I change llb.Scratch() to llb.Image("tonistiigi/test:nolayers") I get "failed to solve: rpc error: code = Unknown desc = failed to compute cache key: cannot checksum empty reference" here.
Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
|
Updated the error message. Do you think I should add a test using |
We could test these cases with that image indeed as the codepath they invoke is different than scratch that can be determined directly from llb. Add the image into mirrored images list https://github.com/moby/buildkit/blob/master/client/client_test.go#L69 that makes sure it is pulled only once (and can be cached between test invocations). Another way would be to build such an image inside the test and push it to the local registry https://github.com/moby/buildkit/blob/master/client/client_test.go#L559 . Either solution is ok by me. |
Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
Fix this panic when copying from an image with no layers:
When the path is "/", we allow it because it's a noop.
Based on #2185
Signed-off-by: Aaron Lehmann alehmann@netflix.com