From 617d8ed67b61ec470cdb3526493a03827f8a135b Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Fri, 6 Nov 2026 18:28:27 +0800 Subject: [PATCH] fix(pools): check copy names before locking them CopyObject locks its source and destination before it reads the source, and lockObjects merged two requests only when their raw names were equal. A lock is taken on the cleaned name, so an invalid source such as a//x or a/./x and the destination a/x asked for a read lock and a write lock on one resource: the copy waited on itself until the operation timeout and answered 503, holding the key's lock in short windows meanwhile. Before the lock-domain change the invalid source was refused at once, when the source was read. This reached single nodes, multi-pool deployments and single pools whose sets share hosts. - Check the source with checkGetObjArgs before any lock, the first check and the first error the release reported. Lock the destination only when its name is valid. An invalid destination fails at the copy's own destination check before it writes, after a missing source (404) or a failed source precondition (412), as before. - lockObjects merges requests by the resource the lockers see, the cleaned bucket/object, within one lock table: the object's set where the sets of a pool have their own lockers, else one table. This keeps aliases on a shared local table from conflicting and never merges requests of different distributed domains. Tests: aliased sources and destinations are refused with 400 at once in every lock layout and two pools; an invalid destination keeps the 404 and 412 precedence; lockObjects merges an alias with its clean name. The alias and merge tests fail without the change. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Feng Ruohang --- cmd/erasure-server-pool-lock-copy_test.go | 148 ++++++++++++++++++++++ cmd/erasure-server-pool-locks.go | 23 ++-- cmd/object-handlers.go | 13 +- 3 files changed, 175 insertions(+), 9 deletions(-) create mode 100644 cmd/erasure-server-pool-lock-copy_test.go diff --git a/cmd/erasure-server-pool-lock-copy_test.go b/cmd/erasure-server-pool-lock-copy_test.go new file mode 100644 index 000000000..da088fd5f --- /dev/null +++ b/cmd/erasure-server-pool-lock-copy_test.go @@ -0,0 +1,148 @@ +// Copyright (c) 2026 PGSTY +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "context" + "fmt" + "net/http" + "slices" + "testing" + "time" + + xhttp "github.com/minio/minio/internal/http" +) + +// ldCopyAPI returns an API router for mode, or for two pools. +func ldCopyAPI(t *testing.T, mode string) (*erasureServerPools, string, http.Handler) { + t.Helper() + if mode != "pools" { + return ldAPI(t, mode) + } + z, bucket := bulkDeletePools(t, 2) + if err := newTestConfig(globalMinioDefaultRegion, z); err != nil { + t.Fatal(err) + } + return z, bucket, initTestAPIEndPoints(z, nil) +} + +// A lock is taken on the cleaned name, so an invalid name that aliases +// another one locks the same resource. A copy whose source or destination is +// such an alias is refused before it locks anything, as the release refused +// it when it read the source. +func TestLockDomainCopyRejectsAliasNames(t *testing.T) { + for _, mode := range append(slices.Clone(ldModes), "pools") { + t.Run(mode, func(t *testing.T) { + z, bucket, router := ldCopyAPI(t, mode) + prev := globalOperationTimeout + globalOperationTimeout = newDynamicTimeout(5*time.Second, 5*time.Second) + t.Cleanup(func() { globalOperationTimeout = prev }) + ldMustPut(t, z, bucket, "alias/x", "victim") + + for _, tc := range []struct{ src, dst string }{ + {"alias//x", "alias/x"}, + {"alias/./x", "alias/x"}, + {"alias/x", "alias//x"}, + } { + t.Run(fmt.Sprintf("%s->%s", tc.src, tc.dst), func(t *testing.T) { + start := time.Now() + rec := ldCopyRequest(t.Context(), t, router, bucket, tc.src, tc.dst, nil) + if took := time.Since(start); took > time.Second { + t.Errorf("refused after %v", took) + } + if rec.Code != http.StatusBadRequest { + t.Errorf("status %d, want 400: %s", rec.Code, rec.Body.String()) + } + ctx, cancel := context.WithTimeout(t.Context(), time.Second) + defer cancel() + if err := ldPut(ctx, z, bucket, "alias/x", "victim"); err != nil { + t.Errorf("a PUT of alias/x after the copy: %v", err) + } + if body, _ := ldBody(t, z, bucket, "alias/x"); body != "victim" { + t.Errorf("alias/x = %q", body) + } + }) + } + }) + } +} + +// An invalid destination does not change which error a copy reports first: +// a missing source and a failed source precondition still answer as in the +// release. +func TestLockDomainCopyErrorPrecedence(t *testing.T) { + for _, mode := range append(slices.Clone(ldModes), "pools") { + t.Run(mode, func(t *testing.T) { + z, bucket, router := ldCopyAPI(t, mode) + ldMustPut(t, z, bucket, "prec/src", "source") + rec := ldCopyRequest(t.Context(), t, router, bucket, "prec/missing", "prec//dst", nil) + if rec.Code != http.StatusNotFound { + t.Errorf("missing source: status %d, want 404: %s", rec.Code, rec.Body.String()) + } + rec = ldCopyRequest(t.Context(), t, router, bucket, "prec/src", "prec//dst", map[string]string{ + xhttp.AmzCopySourceIfMatch: `"00000000000000000000000000000000"`, + }) + if rec.Code != http.StatusPreconditionFailed { + t.Errorf("failed precondition: status %d, want 412: %s", rec.Code, rec.Body.String()) + } + rec = ldCopyRequest(t.Context(), t, router, bucket, "prec/src", "prec//dst", nil) + if rec.Code != http.StatusBadRequest { + t.Errorf("invalid destination: status %d, want 400: %s", rec.Code, rec.Body.String()) + } + }) + } +} + +// lockObjects merges the requests of one resource in one lock table, also for +// names that differ before cleaning; the same resource in two distributed +// lock domains stays two locks. +func TestLockObjectsMergesByResource(t *testing.T) { + for _, mode := range ldModes { + t.Run(mode, func(t *testing.T) { + z, bucket := ldPool(t, mode) + pool := z.serverPools[0] + prev := globalOperationTimeout + globalOperationTimeout = newDynamicTimeout(3*time.Second, 3*time.Second) + t.Cleanup(func() { globalOperationTimeout = prev }) + for i := range 16 { + clean := fmt.Sprintf("merge-%d/x", i) + alias := fmt.Sprintf("merge-%d//x", i) + start := time.Now() + _, unlock, err := z.lockObjects(t.Context(), + objectLockRequest{bucket: bucket, object: alias}, + objectLockRequest{bucket: bucket, object: clean, write: true}) + if err != nil { + t.Fatalf("%s + %s: %v after %v", alias, clean, err, time.Since(start)) + } + if took := time.Since(start); took > time.Second { + t.Errorf("%s + %s took %v", alias, clean, took) + } + sameTable := pool.sharesLocalNSLock() || pool.getHashedSetIndex(alias) == pool.getHashedSetIndex(clean) + // The resource is write locked in the clean name's table. + if ldTry(t, pool.getHashedSet(clean).NewNSLock(bucket, clean), false) { + t.Errorf("%s: not write locked", clean) + } + if !sameTable && !ldTry(t, pool.getHashedSet(alias).NewNSLock(bucket, clean), false) { + // The alias holds only a read lock in its own domain. + t.Errorf("%s: alias domain write locked", alias) + } + unlock() + } + }) + } +} diff --git a/cmd/erasure-server-pool-locks.go b/cmd/erasure-server-pool-locks.go index b577bf566..b8e998b2e 100644 --- a/cmd/erasure-server-pool-locks.go +++ b/cmd/erasure-server-pool-locks.go @@ -87,27 +87,34 @@ type objectLockRequest struct { func (z *erasureServerPools) lockObjects(ctx context.Context, reqs ...objectLockRequest) (LockContext, func(), error) { type entry struct { objectLockRequest - domain int + domain int + resource string } + // A lock is one resource in one lock table. The resource is the cleaned + // name the lockers see; the domain is the set whose lockers grant it, + // or 0 where every set shares one local table or the pools lock + // applies. + splitDomains := z.SinglePool() && !z.serverPools[0].sharesLocalNSLock() locks := make([]entry, 0, len(reqs)) for _, r := range reqs { r.object = encodeDirObject(r.object) - i := slices.IndexFunc(locks, func(e entry) bool { return e.bucket == r.bucket && e.object == r.object }) + domain := 0 + if splitDomains { + domain = z.serverPools[0].getHashedSetIndex(r.object) + } + resource := pathJoin(r.bucket, r.object) + i := slices.IndexFunc(locks, func(e entry) bool { return e.domain == domain && e.resource == resource }) if i >= 0 { locks[i].write = locks[i].write || r.write continue } - domain := 0 - if z.SinglePool() { - domain = z.serverPools[0].getHashedSetIndex(r.object) - } - locks = append(locks, entry{objectLockRequest: r, domain: domain}) + locks = append(locks, entry{objectLockRequest: r, domain: domain, resource: resource}) } slices.SortFunc(locks, func(a, b entry) int { if a.domain != b.domain { return a.domain - b.domain } - return strings.Compare(pathJoin(a.bucket, a.object), pathJoin(b.bucket, b.object)) + return strings.Compare(a.resource, b.resource) }) nsLocks := make([]nsLockRequest, len(locks)) for i, l := range locks { diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 4f9100df2..759c2dbb1 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -1483,8 +1483,19 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re // request's context. Both locks are released once the copy has been // written: a synchronous replication below reads the destination with its // own lock. + // + // Names are checked before they are locked: a lock is taken on the + // cleaned name, so an invalid alias such as a//x would lock the resource + // of a/x a second time. The source is checked first, as reading it did + // before. An invalid destination is not locked; the copy fails at its + // own destination check before it writes, after the source errors that + // take precedence over it. + if err := checkGetObjArgs(ctx, srcBucket, srcObject); err != nil { + writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) + return + } locks := []objectLockRequest{{bucket: srcBucket, object: srcObject}} - if !remoteCallRequired { + if !remoteCallRequired && checkCopyObjArgs(ctx, dstBucket, dstObject) == nil { locks = append(locks, objectLockRequest{bucket: dstBucket, object: dstObject, write: true}) } lkctx, unlock, err := objectAPI.lockObjects(ctx, locks...)