mirror of
https://github.com/moby/buildkit.git
synced 2026-11-06 05:16:45 +00:00
source/git: fix daemon panic on resolver cache poisoning
resolveMetadata stored its result into the resolver cache through release(v) where v was a *Metadata. On a failed resolve or a cache hit it set the pointer to nil, but passing a typed nil pointer as an interface stores a non-nil value, so a nil *Metadata was cached for that remote#ref. A later resolve of the same key read it back and dereferenced nil, crashing buildkitd. Because the cache is shared across the jobs on a solve state, an untrusted Dockerfile or frontend could crash the daemon, and one build could poison another's. Store an untyped nil instead, matching the HTTP source, so only a successfully resolved Metadata is ever cached. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> (cherry picked from commit 4dd98355a71d79bdf93bd3456fda3a64847ce1de) (cherry picked from commit c1399f7266310397f5615f4751147f886afff6d2)
This commit is contained in:
1 parent
d4bfe06a4d
commit
16c6856432
3 files changed
+87
-3
No files matched your search
@@ -543,6 +543,88 @@ func testGitResolveMutatedSource(t *testing.T, sb integration.Sandbox) {
|
||||
checkAllReleasable(t, c, sb, false)
|
||||
}
|
||||
|
||||
// testGitResolveCacheAfterFailure resolves the same ref repeatedly in one
|
||||
// build after a failed and a cached resolve, so the resolver cache is read
|
||||
// after both have released their lock.
|
||||
func testGitResolveCacheAfterFailure(t *testing.T, sb integration.Sandbox) {
|
||||
integration.SkipOnPlatform(t, "windows")
|
||||
ctx := sb.Context()
|
||||
c, err := New(ctx, sb.Address())
|
||||
require.NoError(t, err)
|
||||
defer c.Close()
|
||||
|
||||
gitDir := t.TempDir()
|
||||
gitCommands := []string{
|
||||
"git init",
|
||||
"git config --local user.email test",
|
||||
"git config --local user.name test",
|
||||
"echo a > a",
|
||||
"git add a",
|
||||
"git commit -m a",
|
||||
"git branch v1",
|
||||
"echo b > b",
|
||||
"git add b",
|
||||
"git commit -m b",
|
||||
"git update-server-info",
|
||||
}
|
||||
err = runInDir(gitDir, gitCommands...)
|
||||
require.NoError(t, err)
|
||||
|
||||
cmd := exec.CommandContext(t.Context(), "git", "rev-parse", "v1")
|
||||
cmd.Dir = gitDir
|
||||
out, err := cmd.Output()
|
||||
require.NoError(t, err)
|
||||
commitV1 := strings.TrimSpace(string(out))
|
||||
|
||||
cmd = exec.CommandContext(t.Context(), "git", "rev-parse", "HEAD")
|
||||
cmd.Dir = gitDir
|
||||
out, err = cmd.Output()
|
||||
require.NoError(t, err)
|
||||
commitHead := strings.TrimSpace(string(out))
|
||||
|
||||
server := httptest.NewServer(http.FileServer(http.Dir(filepath.Clean(gitDir))))
|
||||
defer server.Close()
|
||||
|
||||
_, err = c.Build(ctx, SolveOpt{}, "test", func(ctx context.Context, c gateway.Client) (*gateway.Result, error) {
|
||||
resolve := func(checksum string) (*sourceresolver.MetaResponse, error) {
|
||||
attrs := map[string]string{
|
||||
"git.fullurl": server.URL + "/.git",
|
||||
}
|
||||
if checksum != "" {
|
||||
attrs[pb.AttrGitChecksum] = checksum
|
||||
}
|
||||
return c.ResolveSourceMetadata(ctx, &pb.SourceOp{
|
||||
Identifier: "git://" + strings.TrimPrefix(server.URL, "http://") + "/.git#v1",
|
||||
Attrs: attrs,
|
||||
}, sourceresolver.Opt{})
|
||||
}
|
||||
|
||||
// failed resolve
|
||||
_, err := resolve(commitHead)
|
||||
require.ErrorContains(t, err, "expected checksum to match")
|
||||
|
||||
md, err := resolve("")
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, md.Git)
|
||||
require.Equal(t, commitV1, md.Git.Checksum)
|
||||
|
||||
// resolved from cache
|
||||
md, err = resolve("")
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, md.Git)
|
||||
require.Equal(t, commitV1, md.Git.Checksum)
|
||||
|
||||
// cached value does not match the checksum
|
||||
_, err = resolve(commitHead)
|
||||
require.ErrorContains(t, err, "expected checksum to match")
|
||||
|
||||
return gateway.NewResult(), nil
|
||||
}, nil)
|
||||
require.NoError(t, err)
|
||||
|
||||
checkAllReleasable(t, c, sb, false)
|
||||
}
|
||||
|
||||
func testGitResolveSourceMetadata(t *testing.T, sb integration.Sandbox) {
|
||||
// TODO: remove once CI has Git for Windows >= 2.56.0.windows.2
|
||||
// https://github.com/git-for-windows/git/issues/6449
|
||||
|
||||
@@ -156,6 +156,7 @@ var allTests = []func(t *testing.T, sb integration.Sandbox){
|
||||
testGitBundleRoundTrip,
|
||||
testGitBundleRoundTripRegistry,
|
||||
testGitResolveMutatedSource,
|
||||
testGitResolveCacheAfterFailure,
|
||||
testGitResolveSourceMetadata,
|
||||
|
||||
// client_http_source_test.go
|
||||
|
||||
@@ -597,9 +597,10 @@ func (gs *gitSourceHandler) resolveMetadata(ctx context.Context, jobCtx solver.J
|
||||
}
|
||||
saveResolved := true
|
||||
defer func() {
|
||||
v := md
|
||||
if retErr != nil || !saveResolved {
|
||||
v = nil
|
||||
// untyped nil, a nil *Metadata would be stored as a value
|
||||
var v any
|
||||
if retErr == nil && saveResolved {
|
||||
v = md
|
||||
}
|
||||
if err := release(v); err != nil {
|
||||
bklog.G(ctx).Warnf("failed to release resolver cache lock for %s: %v", gs.remoteKey(), err)
|
||||
|
||||
Reference in new issue
Block a user