From cdbebb06d6fd87fabcaa2930e28e95ed5e6373a6 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Thu, 24 Sep 2026 16:58:30 +0400 Subject: [PATCH] fix: place the null version by its recorded position in posix versioning `latestObjVersion` and `fileToObjVersions` placed the null version among the `ulid` versions by comparing file modification times, but a delete marker keeps the data and mtime of the version it hides. A null delete marker created in a versioning-suspended bucket therefore tied with the version below it and lost. Deleting the current version of an object whose history held a null delete marker restored the older version instead of the delete marker, and `ListObjectVersions` listed the delete marker below that version. Directory objects were exposed too, as a directory's mtime changes when objects are created under it. `createObjVersion` now records the position of the null version when it moves it to the versioning directory. At that moment the null version is the current one, so it is newer than every version already there and older than every version created later. The id of the newest version in the directory is stored in the new `null-version-prev` attribute, and `nullVersionPos` compares it with the `ulid` version ids for both `DeleteObject` and `ListObjectVersions`. Null versions moved before this change carry no attribute and still fall back to the modification time. The attribute is not copied onto the object when a null version is restored. `fileToObjVersions` also listed the null version without checking the `VersionIdMarker`, so it was repeated on every later page, and a `null` marker never matched, which dropped every version below it. The current version was listed again when it was the marker, so paging with the marker at a current version looped forever. The null version is now listed through `addNullVersion`, which skips it before the marker and continues after it when it is the marker, and the current version is no longer listed again when the marker names it. --- backend/posix/posix.go | 185 ++++++++++++++++-------- tests/integration/ListObjectVersions.go | 130 +++++++++++++++++ tests/integration/group-tests.go | 4 + tests/integration/versioning.go | 141 ++++++++++++++++++ 4 files changed, 399 insertions(+), 61 deletions(-) diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 18b5ca19..9b7792fe 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -27,6 +27,7 @@ import ( "net/http" "os" "path/filepath" + "slices" "sort" "strconv" "strings" @@ -191,6 +192,7 @@ const ( versioningKey = "versioning" deleteMarkerKey = "delete-marker" versionIdKey = "version-id" + nullVersionPrevKey = "null-version-prev" partCrc64nvme = "part-crc64nvme" mpMetaKey = "mp-metadata" @@ -1520,6 +1522,21 @@ func (p *Posix) createObjVersion(bucket, key string, size int64, acc auth.Accoun } } + if versionId == nullVersionId { + // the null version id doesn't sort with the ulid version ids, so + // record its place in the version history: it's the current + // version, which makes it newer than the versions already in the + // versioning directory and older than any version created later + prev, err := newestObjVersionId(filepath.Join(versionBucketPath, genObjVersionKey(key))) + if err != nil { + return versionPath, fmt.Errorf("get newest object version: %w", err) + } + err = p.meta.StoreAttribute(f.File(), versionPath, "", nullVersionPrevKey, []byte(prev)) + if err != nil { + return versionPath, fmt.Errorf("store %v attribute: %w", nullVersionPrevKey, err) + } + } + if err := f.link(); err != nil { return versionPath, err } @@ -1702,10 +1719,11 @@ func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc { if err == nil { versionId = string(versionIdBytes) } - if versionId == versionIdMarker { - *pastVersionIdMarker = true - } - if *pastVersionIdMarker { + // the version id marker is the version listed last, so the + // listing continues with the version following it + if !*pastVersionIdMarker { + *pastVersionIdMarker = versionId == versionIdMarker + } else { fi, err := d.Info() if errors.Is(err, fs.ErrNotExist) { return nil, backend.ErrSkipObj @@ -1794,11 +1812,17 @@ func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc { // before starting the object versions listing var nullVersionIdObj *s3response.ObjectVersion var nullObjDelMarker *types.DeleteMarkerEntry + var nullPos nullVersionPos nf, err := os.Stat(filepath.Join(versionPath, nullVersionId)) if err != nil && !errors.Is(err, fs.ErrNotExist) { return nil, err } if err == nil { + nullPos, err = p.getNullVersionPos(versionPath, nf) + if err != nil { + return nil, err + } + isDel, err := p.isObjDeleteMarker(versionPath, nullVersionId) if err != nil { return nil, err @@ -1847,34 +1871,39 @@ func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc { } isNullVersionIdObjFound := nullVersionIdObj != nil || nullObjDelMarker != nil + isNullVersionIdObjAdded := false - if len(dirEnts) == 1 && (isNullVersionIdObjFound) { - if nullObjDelMarker != nil { - delMarkers = append(delMarkers, *nullObjDelMarker) + // addNullVersion lists the null version at its place in the + // version history, or continues the listing after it when it's + // the version id marker. It returns the truncated result when + // the null version fills up the listing. + addNullVersion := func() *backend.ObjVersionFuncResult { + isNullVersionIdObjAdded = true + if !*pastVersionIdMarker { + *pastVersionIdMarker = versionIdMarker == nullVersionId + return nil } + if nullVersionIdObj != nil { objects = append(objects, *nullVersionIdObj) } + if nullObjDelMarker != nil { + delMarkers = append(delMarkers, *nullObjDelMarker) + } - if availableObjCount == 1 { + if availableObjCount--; availableObjCount == 0 { return &backend.ObjVersionFuncResult{ ObjectVersions: objects, DelMarkers: delMarkers, Truncated: true, NextVersionIdMarker: nullVersionId, - }, nil - } else { - return &backend.ObjVersionFuncResult{ - ObjectVersions: objects, - DelMarkers: delMarkers, - }, nil + } } + return nil } - isNullVersionIdObjAdded := false + for _, dEntry := range slices.Backward(dirEnts) { - for i := len(dirEnts) - 1; i >= 0; i-- { - dEntry := dirEnts[i] // Skip the null versionId object to not // break the object versions list if dEntry.Name() == nullVersionId { @@ -1890,26 +1919,11 @@ func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc { } // If the null versionId object is found, first push it - // by checking its creation date, then continue the adding - if isNullVersionIdObjFound && !isNullVersionIdObjAdded { - if nf.ModTime().After(f.ModTime()) { - if nullVersionIdObj != nil { - objects = append(objects, *nullVersionIdObj) - } - if nullObjDelMarker != nil { - delMarkers = append(delMarkers, *nullObjDelMarker) - } - - isNullVersionIdObjAdded = true - - if availableObjCount--; availableObjCount == 0 { - return &backend.ObjVersionFuncResult{ - ObjectVersions: objects, - DelMarkers: delMarkers, - Truncated: true, - NextVersionIdMarker: nullVersionId, - }, nil - } + // by checking its place in the version history, then + // continue the adding + if isNullVersionIdObjFound && !isNullVersionIdObjAdded && nullPos.isNewerThan(f) { + if res := addNullVersion(); res != nil { + return res, nil } } versionId := f.Name() @@ -1979,20 +1993,8 @@ func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc { // If null versionId object is found but not yet pushed, // push it after the listing, as it's the oldest object version if isNullVersionIdObjFound && !isNullVersionIdObjAdded { - if nullVersionIdObj != nil { - objects = append(objects, *nullVersionIdObj) - } - if nullObjDelMarker != nil { - delMarkers = append(delMarkers, *nullObjDelMarker) - } - - if availableObjCount--; availableObjCount == 0 { - return &backend.ObjVersionFuncResult{ - ObjectVersions: objects, - DelMarkers: delMarkers, - Truncated: true, - NextVersionIdMarker: nullVersionId, - }, nil + if res := addNullVersion(); res != nil { + return res, nil } } @@ -5179,7 +5181,7 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( }, nil } - srcObjVersion, err := latestObjVersion(ents) + srcObjVersion, err := p.latestObjVersion(versionPath, ents) if err != nil { return nil, fmt.Errorf("get latest obj version: %w", err) } @@ -5231,6 +5233,11 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( } for _, attr := range attrs { + // the place of the null version only applies in + // the versioning directory + if attr == nullVersionPrevKey { + continue + } data, err := p.meta.RetrieveAttribute(nil, versionPath, srcVersionId, attr) if err != nil { return nil, fmt.Errorf("load %v attribute", attr) @@ -5357,13 +5364,65 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( return &s3.DeleteObjectOutput{}, nil } -// latestObjVersion returns the version, among the version directory entries, -// that becomes the latest one when the latest version of the object is -// deleted. The entries are named after the version id and os.ReadDir sorts -// them by name, which puts the ulid version ids in creation order. The null -// version id doesn't sort with them, so it's placed by its modification time, -// which is the time the version was created. -func latestObjVersion(ents []fs.DirEntry) (fs.FileInfo, error) { +// newestObjVersionId returns the id of the newest version, other than the +// null version, in the object versioning directory at versionPath, or an +// empty string if it has none. The entries are named after the version id +// and os.ReadDir sorts them by name, which puts the ulid version ids in +// creation order. +func newestObjVersionId(versionPath string) (string, error) { + ents, err := os.ReadDir(versionPath) + if errors.Is(err, fs.ErrNotExist) { + return "", nil + } + if err != nil { + return "", err + } + for _, ent := range slices.Backward(ents) { + if ent.Name() != nullVersionId { + return ent.Name(), nil + } + } + return "", nil +} + +// nullVersionPos is the place of the null version in the version history of +// an object, among the versions ordered by their ulid version ids +type nullVersionPos struct { + // prev is the id of the newest version older than the null version, + // empty if the null version is the oldest one + prev string + // modTime places a null version that was moved to the versioning + // directory before its place was recorded + modTime *time.Time +} + +// isNewerThan reports whether the null version is newer than the version +// fi, an entry of the object versioning directory named after its version id +func (n nullVersionPos) isNewerThan(fi fs.FileInfo) bool { + if n.modTime != nil { + return n.modTime.After(fi.ModTime()) + } + return fi.Name() <= n.prev +} + +// getNullVersionPos returns the place of the null version, nullInfo, in the +// object versioning directory at versionPath +func (p *Posix) getNullVersionPos(versionPath string, nullInfo fs.FileInfo) (nullVersionPos, error) { + prev, err := p.meta.RetrieveAttribute(nil, versionPath, nullVersionId, nullVersionPrevKey) + if errors.Is(err, meta.ErrNoSuchKey) { + modTime := nullInfo.ModTime() + return nullVersionPos{modTime: &modTime}, nil + } + if err != nil { + return nullVersionPos{}, fmt.Errorf("get %v attribute: %w", nullVersionPrevKey, err) + } + return nullVersionPos{prev: string(prev)}, nil +} + +// latestObjVersion returns the version, among the entries of the object +// versioning directory at versionPath, that becomes the latest one when the +// latest version of the object is deleted +func (p *Posix) latestObjVersion(versionPath string, ents []fs.DirEntry) (fs.FileInfo, error) { var latest, nullEnt fs.DirEntry for _, ent := range ents { if ent.Name() == nullVersionId { @@ -5391,7 +5450,11 @@ func latestObjVersion(ents []fs.DirEntry) (fs.FileInfo, error) { return nil, err } - if nullInfo.ModTime().After(latestInfo.ModTime()) { + pos, err := p.getNullVersionPos(versionPath, nullInfo) + if err != nil { + return nil, err + } + if pos.isNewerThan(latestInfo) { return nullInfo, nil } @@ -5436,7 +5499,7 @@ func (p *Posix) deleteDirObjectLatestVersion(bucket, key string) error { return nil } - srcInfo, err := latestObjVersion(ents) + srcInfo, err := p.latestObjVersion(versionPath, ents) if err != nil { return fmt.Errorf("get latest obj version: %w", err) } diff --git a/tests/integration/ListObjectVersions.go b/tests/integration/ListObjectVersions.go index eaf9b51c..d5a8f801 100644 --- a/tests/integration/ListObjectVersions.go +++ b/tests/integration/ListObjectVersions.go @@ -599,6 +599,136 @@ func ListObjectVersions_single_null_versionId_object(s *S3Conf) error { }) } +func ListObjectVersions_paginate_null_version(s *S3Conf) error { + testName := "ListObjectVersions_paginate_null_version" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + // the null version sits in the middle of the version history: + // it's an object for "a-obj" and "c-dir/" and a delete marker + // for "b-obj" + objs := []string{"a-obj", "b-obj", "c-dir/"} + oldVersions := map[string][]types.ObjectVersion{} + for _, obj := range objs { + versions, err := createObjVersions(s3client, bucket, obj, 2) + if err != nil { + return err + } + versions[0].IsLatest = getBoolPtr(false) + oldVersions[obj] = versions + } + + err := putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusSuspended) + if err != nil { + return err + } + + nullVersions := map[string][]types.ObjectVersion{} + for _, obj := range []string{"a-obj", "c-dir/"} { + size := objDataLen(obj, 100) + out, err := putObjectWithData(size, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + }, s3client) + if err != nil { + return err + } + nullVersions[obj] = []types.ObjectVersion{ + { + ETag: out.res.ETag, + IsLatest: getBoolPtr(false), + Key: &obj, + Size: &size, + VersionId: &nullVersionId, + StorageClass: types.ObjectVersionStorageClassStandard, + }, + } + } + + delMarkerObj := "b-obj" + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &delMarkerObj, + }) + cancel() + if err != nil { + return err + } + delMarkers := []types.DeleteMarkerEntry{ + { + Key: &delMarkerObj, + VersionId: &nullVersionId, + IsLatest: getBoolPtr(false), + }, + } + + err = putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusEnabled) + if err != nil { + return err + } + + versions := []types.ObjectVersion{} + for _, obj := range objs { + newVersions, err := createObjVersions(s3client, bucket, obj, 2) + if err != nil { + return err + } + versions = append(versions, newVersions...) + versions = append(versions, nullVersions[obj]...) + versions = append(versions, oldVersions[obj]...) + } + + // the pages end on each of the versions, the null ones included, + // and each version is listed once + total := len(versions) + len(delMarkers) + for _, maxKeys := range []int32{1, 2, 3, 4, 1000} { + var gotVersions []types.ObjectVersion + var gotDelMarkers []types.DeleteMarkerEntry + var keyMarker, versionIdMarker *string + for page := 0; ; page++ { + if page > total { + return fmt.Errorf("max-keys %v: expected the listing to end within %v pages", + maxKeys, total) + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + MaxKeys: &maxKeys, + KeyMarker: keyMarker, + VersionIdMarker: versionIdMarker, + }) + cancel() + if err != nil { + return fmt.Errorf("max-keys %v, page %v: %w", maxKeys, page, err) + } + + if count := len(out.Versions) + len(out.DeleteMarkers); count > int(maxKeys) { + return fmt.Errorf("max-keys %v, page %v: expected at most %v entries, instead got %v", + maxKeys, page, maxKeys, count) + } + gotVersions = append(gotVersions, out.Versions...) + gotDelMarkers = append(gotDelMarkers, out.DeleteMarkers...) + + if out.IsTruncated == nil || !*out.IsTruncated { + break + } + keyMarker, versionIdMarker = out.NextKeyMarker, out.NextVersionIdMarker + } + + if !compareVersions(versions, gotVersions) { + return fmt.Errorf("max-keys %v: expected the listed object versions to be %v, instead got %v", + maxKeys, sprintVersions(versions), sprintVersions(gotVersions)) + } + if !compareDelMarkers(delMarkers, gotDelMarkers) { + return fmt.Errorf("max-keys %v: expected the listed delete markers to be %v, instead got %v", + maxKeys, delMarkers, gotDelMarkers) + } + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + func ListObjectVersions_checksum(s *S3Conf) error { testName := "ListObjectVersions_checksum" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index d707df24..658756b6 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -2017,6 +2017,7 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_DeleteObject_delete_object_version) ts.Run(Versioning_DeleteObject_dir_object_latest_version) ts.Run(Versioning_DeleteObject_latest_version_with_null_version) + ts.Run(Versioning_DeleteObject_latest_version_null_version_order) ts.Run(Versioning_DeleteObject_non_existing_object) ts.Run(Versioning_DeleteObject_implicit_dir) ts.Run(Versioning_DeleteObject_trailing_slash_counterpart) @@ -2041,6 +2042,7 @@ func TestVersioning(ts *TestState) { ts.Run(ListObjectVersions_multiple_object_versions_truncated) ts.Run(ListObjectVersions_with_delete_markers) ts.Run(ListObjectVersions_containing_null_versionId_obj) + ts.Run(ListObjectVersions_paginate_null_version) ts.Run(ListObjectVersions_single_null_versionId_object) ts.Run(ListObjectVersions_checksum) // Multipart upload @@ -3574,6 +3576,7 @@ func GetIntTests() IntTests { "Versioning_DeleteObject_invalid_versionId": Versioning_DeleteObject_invalid_versionId, "Versioning_DeleteObject_delete_object_version": Versioning_DeleteObject_delete_object_version, "Versioning_DeleteObject_latest_version_with_null_version": Versioning_DeleteObject_latest_version_with_null_version, + "Versioning_DeleteObject_latest_version_null_version_order": Versioning_DeleteObject_latest_version_null_version_order, "Versioning_DeleteObject_dir_object_latest_version": Versioning_DeleteObject_dir_object_latest_version, "Versioning_DeleteObject_non_existing_object": Versioning_DeleteObject_non_existing_object, "Versioning_DeleteObject_implicit_dir": Versioning_DeleteObject_implicit_dir, @@ -3596,6 +3599,7 @@ func GetIntTests() IntTests { "ListObjectVersions_multiple_object_versions_truncated": ListObjectVersions_multiple_object_versions_truncated, "ListObjectVersions_with_delete_markers": ListObjectVersions_with_delete_markers, "ListObjectVersions_containing_null_versionId_obj": ListObjectVersions_containing_null_versionId_obj, + "ListObjectVersions_paginate_null_version": ListObjectVersions_paginate_null_version, "ListObjectVersions_single_null_versionId_object": ListObjectVersions_single_null_versionId_object, "ListObjectVersions_checksum": ListObjectVersions_checksum, "Versioning_Multipart_Upload_success": Versioning_Multipart_Upload_success, diff --git a/tests/integration/versioning.go b/tests/integration/versioning.go index 68fd2e06..64bab6f7 100644 --- a/tests/integration/versioning.go +++ b/tests/integration/versioning.go @@ -2290,6 +2290,147 @@ func Versioning_DeleteObject_latest_version_with_null_version(s *S3Conf) error { }, withVersioning(types.BucketVersioningStatusEnabled)) } +func Versioning_DeleteObject_latest_version_null_version_order(s *S3Conf) error { + testName := "Versioning_DeleteObject_latest_version_null_version_order" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + // the null version is either an object put or a delete marker + // created while versioning is suspended + cases := []struct { + obj string + delMarker bool + }{ + {"my-obj", false}, + {"my-dir/", false}, + {"my-dm-obj", true}, + {"my-dm-dir/", true}, + } + for _, c := range cases { + obj := c.obj + // the versions are told apart by a metadata entry, as a + // directory object carries no data + put := func(marker string) (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + defer cancel() + out, err := s3client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + Metadata: map[string]string{"marker": marker}, + }) + if err != nil { + return "", err + } + return getString(out.VersionId), nil + } + + // history is the version ids from the newest to the oldest, + // with the marker each one is put with + type version struct { + id, marker string + } + var history []version + for _, marker := range []string{"v1", "v2"} { + id, err := put(marker) + if err != nil { + return err + } + history = append([]version{{id, marker}}, history...) + } + + err := putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusSuspended) + if err != nil { + return err + } + + // the null version sits in the middle of the version history + if c.delMarker { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return err + } + if getString(out.VersionId) != nullVersionId { + return fmt.Errorf("%v: expected the delete marker versionId to be %v, instead got %v", + obj, nullVersionId, getString(out.VersionId)) + } + history = append([]version{{nullVersionId, ""}}, history...) + } else { + if _, err := put("null"); err != nil { + return err + } + history = append([]version{{nullVersionId, "null"}}, history...) + } + + err = putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusEnabled) + if err != nil { + return err + } + + for _, marker := range []string{"v3", "v4"} { + id, err := put(marker) + if err != nil { + return err + } + history = append([]version{{id, marker}}, history...) + } + + // deleting the latest version makes the version created right + // before it the latest one, whether it's the null version or not + for i, deleted := range history { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &obj, + VersionId: &deleted.id, + }) + cancel() + if err != nil { + return fmt.Errorf("%v: delete version %v: %w", obj, deleted.id, err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + + // the object is left with no current version when the + // null delete marker or none of the versions is latest + if i+1 == len(history) || history[i+1].marker == "" { + if err == nil { + return fmt.Errorf("%v: expected the object to have no current version after deleting %v, instead got version %v", + obj, deleted.id, getString(res.VersionId)) + } + if err := checkSdkApiErr(err, "NotFound"); err != nil { + return fmt.Errorf("%v: after deleting %v: %w", obj, deleted.id, err) + } + continue + } + if err != nil { + return fmt.Errorf("%v: head object after deleting %v: %w", obj, deleted.id, err) + } + + expected := history[i+1] + if getString(res.VersionId) != expected.id { + return fmt.Errorf("%v: expected the current versionId after deleting %v to be %v, instead got %v", + obj, deleted.id, expected.id, getString(res.VersionId)) + } + expectedMeta := map[string]string{"marker": expected.marker} + if !areMapsSame(res.Metadata, expectedMeta) { + return fmt.Errorf("%v: expected the object metadata after deleting %v to be %v, instead got %v", + obj, deleted.id, expectedMeta, res.Metadata) + } + } + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + func Versioning_DeleteObject_non_existing_object(s *S3Conf) error { testName := "Versioning_DeleteObject_non_existing_object" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {