Repository: rclone/rclone
Pull request #4902 (2021-01-04 17:01:42)

chunker: test detection of incompatible metadata, fix NewObject on case-insensitive filesystem
#### What is the purpose of this change?

Chunker was developed with a goal to work smoothly with most of 30+ rclone storage providers. As no single size fits all, chunker was engineered with versioning in mind. The main principles are:
- newer chunker/rclone versions accept and transparently upgrade metadata produced by _all_ previous versions
- if growing number of formats becomes hard to maintain, the ancient formats must be deprecated _explicitly_, at least one rclone release _in advance_
- chunker is smart enough to tell valid metadata from random garbage
- chunker can detect valid but unsupported metadata produced by future versions (vs garbage)
- upon detecting future metadata chunker must **refuse to modify** such file and explicitly ask user to **update** rclone

Hopefully this rather conservative approach will facilitate wider adoption of chunker/rclone. 

Development of a new metadata format by PR https://github.com/rclone/rclone/pull/4675#issuecomment-740206801 has discovered cases when chunker in rclone v1.50.0-v1.53.3 **fails to detect** composite objects produced and allows to inadvertently corrupt user data. This PR seeks to fix that and proposes a few provisions for compatiblity with future formats.

This PR is separate from #4675 due to different set of targeted rclone releases. I would like to merge it for v1.54 and also to backport it to **v1.53 stable**. I will help to rebase PR #4675 on top of this one.
rclone releases v1.50,v1.51 will **stay affected** by potential corruption. This PR will add a warning in the chunker documentation urging users of affected versions to upgrade.

@ncw Is that at all possible to backport to v1.52? 🙏🤗

The problem with metadata detection is that while chunker uses both directory scanning and metadata reading to detect composite objects, chunker tries to optimize out file reading as much as possible to save on storage API calls/charges. First, there is hardcoded metadata size limit, and files larger then that are not even considered. Next, chunker uses pattern of file names in a given directory to predict whether given set of files are parts of a composite object. Before this patch chunker required that there is at least one data chunk to start checking for a composite.

This PR extends the logic gently: if chunker cannot find data chunks accompanying metaobject but finds at least one potential control or temporary chunk, it marks found files as a _suspected_ composite object and marks its metaobject for _lazy_ reading later (too large metaobjects are thrown away as before). When rclone later tries a _concrete_ operation on the object, chunker performs postponed read and takes the final verdict: _is this compatible composite object, future metadata or just a garbage_? This approach allows to keep directory scan as fast as before but detect future chunked objects better.

Additionally, this PR splits single metadata size limit in two: _maxMetadataSizeWrite_ (255) is the current hard maximum metadata object produced by rclone, _maxMetadataSize_ (1023) is the new increased threshold for files considered by chunker for metadata scan. It is higher to make room for future metadata improvements. Nevertheless, as before, every increase of the _former_ limit _must_ increment the produced metadata version to prevent incorrect treatment and make user update ancient installations of rclone.

I am also hiding from command line a few chunker options which critically change chunker's notion of what a chunk is. I would like to prohibit them on command line completely if rclone provided such an option. As it's imporrible for now, chunker's user documentation provides users with appropriate warnings.

#### Was the change discussed in an issue or in the forum before?

Yes, in https://github.com/rclone/rclone/pull/4675#issuecomment-739639666 and https://github.com/rclone/rclone/pull/4675#issuecomment-740206801

#### Checklist

- [x] I have read the [contribution guidelines](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#submitting-a-pull-request).
- [x] I have added tests for all changes in this PR if appropriate.
- [x] I have added documentation for the changes if appropriate.
- [x] All commit messages are in [house style](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#commit-messages).
- [x] I'm done, this Pull Request is ready for review :-)


ivandeex: I am including here a fix for the recent issue #4830 (NewObject on a case-insensitive FS).
The quick integration test suite now passes: https://github.com/ivandeex/rclone/runs/1650174935?check_suite_focus=true
ivandeex: @ncw Please review
ivandeex: @ncw Please set due date on the 1.54 milestone.
ncw: > Chunker was developed with a goal to work smoothly with most of 30+ rclone storage providers. As no single size fits all, chunker was engineered with versioning in mind. The main principles are:
> 
> * newer chunker/rclone versions accept and transparently upgrade metadata produced by _all_ previous versions
> * if growing number of formats becomes hard to maintain, the ancient formats must be deprecated _explicitly_, at least one rclone release _in advance_
> * chunker is smart enough to tell valid metadata from random garbage
> * chunker can detect valid but unsupported metadata produced by future versions (vs garbage)
> * upon detecting future metadata chunker must **refuse to modify** such file and explicitly ask user to **update** rclone
> 
> Hopefully this rather conservative approach will facilitate wider adoption of chunker/rclone.

Seems reasonable. Though we might want to specify a deprecation period (1 year say) rather than tying it to the rclone release schedule.

> Development of a new metadata format by PR [#4675 (comment)](https://github.com/rclone/rclone/pull/4675#issuecomment-740206801) has discovered cases when chunker in rclone v1.50.0-v1.53.3 **fails to detect** composite objects produced and allows to inadvertently corrupt user data. This PR seeks to fix that and proposes a few provisions for compatiblity with future formats.

OK

> This PR is separate from #4675 due to different set of targeted rclone releases. I would like to merge it for v1.54 and also to backport it to **v1.53 stable**. I will help to rebase PR #4675 on top of this one.
> rclone releases v1.50,v1.51 will **stay affected** by potential corruption. This PR will add a warning in the chunker documentation urging users of affected versions to upgrade.

So merge this PR first the rebase #4675?

> @ncw Is that at all possible to backport to v1.52?

I think this patch is too big for a point release. I try to keep the point release patches really small as not to break stuff. So I think we should target this for 1.54 which I'll try to get out before the end of January 2021.

I don't normally update previous rclones once a new .0 has been released to try and keep the number of releases we need to maintain down.

> The problem with metadata detection is that while chunker uses both directory scanning and metadata reading to detect composite objects, chunker tries to optimize out file reading as much as possible to save on storage API calls/charges. First, there is hardcoded metadata size limit, and files larger then that are not even considered. Next, chunker uses pattern of file names in a given directory to predict whether given set of files are parts of a composite object. Before this patch chunker required that there is at least one data chunk to start checking for a composite.
> 
> This PR extends the logic gently: if chunker cannot find data chunks accompanying metaobject but finds at least one potential control or temporary chunk, it marks found files as a _suspected_ composite object and marks its metaobject for _lazy_ reading later (too large metaobjects are thrown away as before). When rclone later tries a _concrete_ operation on the object, chunker performs postponed read and takes the final verdict: _is this compatible composite object, future metadata or just a garbage_? This approach allows to keep directory scan as fast as before but detect future chunked objects better.
> 
> Additionally, this PR splits single metadata size limit in two: _maxMetadataSizeWrite_ (255) is the current hard maximum metadata object produced by rclone, _maxMetadataSize_ (1023) is the new increased threshold for files considered by chunker for metadata scan. It is higher to make room for future metadata improvements. Nevertheless, as before, every increase of the _former_ limit _must_ increment the produced metadata version to prevent incorrect treatment and make user update ancient installations of rclone.

OK

> I am also hiding from command line a few chunker options which critically change chunker's notion of what a chunk is. I would like to prohibit them on command line completely if rclone provided such an option. As it's imporrible for now, chunker's user documentation provides users with appropriate warnings.

That is the normal process for deprecation - Hide them from the command line and configurator, make them generate a warning (or error) then eventually remove. If you want them to generate an error then you'll need to do that by hand at the moment (in `NewFs`).

I'll review the code now
ivandeex: > > * if growing number of formats becomes hard to maintain, the ancient formats must be deprecated _explicitly_, at least one rclone release _in advance_
> Seems reasonable. Though we might want to specify a deprecation period (1 year say) rather than tying it to the rclone release schedule.

I'm not sure about the schedule. I'm tying that with the `hard to maintain` condition.

> So merge this PR first the rebase #4675?

Yes
> I think this patch is too big for a point release. I try to keep the point release patches really small as not to break stuff. So I think we should target this for 1.54 which I'll try to get out before the end of January 2021.

That is sad. My intention was to reduce number of releases affected by the bug.
Would it be possible to backport to 1.53-stable the single commit "chunker: ensure that chunker rejects future metadata" alone, if I factor it to a separate PR?
(https://github.com/rclone/rclone/pull/4902/commits/71b23f9bb2ce77fb9f424900d8d20aad3aaccb49)

> I don't normally update previous rclones once a new .0 has been released to try and keep the number of releases we need to maintain down.

So backporting to 1.52.x is impossible as I expected.

ncw: > > > * if growing number of formats becomes hard to maintain, the ancient formats must be deprecated _explicitly_, at least one rclone release _in advance_
> > >   Seems reasonable. Though we might want to specify a deprecation period (1 year say) rather than tying it to the rclone release schedule.
> 
> I'm not sure about the schedule. I'm tying that with the `hard to maintain` condition.

I'll leave the exact period up to you :-)

> > I think this patch is too big for a point release. I try to keep the point release patches really small as not to break stuff. So I think we should target this for 1.54 which I'll try to get out before the end of January 2021.
> 
> That is sad. My intention was to reduce number of releases affected by the bug.
> Would it be reasonable to backport to 1.53-stable the single commit "chunker: ensure that chunker rejects future metadata" alone, if I factored it as a separate PR?
> ([71b23f9](https://github.com/rclone/rclone/commit/71b23f9bb2ce77fb9f424900d8d20aad3aaccb49))

Yes that sounds reasonable. Howe about we do that first? I can accept it into 1.54 then we can backport it to 1.53. There are a couple of fixes to 1.53 we could push out too with it, and try and get that done in the next few days?
 
> > I don't normally update previous rclones once a new .0 has been released to try and keep the number of releases we need to maintain down.
> 
> So backporting to 1.52.x is impossible as I expected.

Not impossible, but I wonder of how much use? Do people follow the 1.52.x series when the 1.53.x series is available? I really don't know!

ivandeex: - factored the core commit into PR #4917
- kept the commit here as this change was pre-rebased on top of the other one
- labeled #4917 as a candidate for 1.53.4, removed the mark here
- changed release number in documentation to 1.54
- done?

Patch:
diff --git a/backend/chunker/chunker.go b/backend/chunker/chunker.go
index fdb235a86..b2379551a 100644
--- a/backend/chunker/chunker.go
+++ b/backend/chunker/chunker.go
@@ -97,7 +97,8 @@ var (
 //
 // And still chunker's primary function is to chunk large files
 // rather than serve as a generic metadata container.
-const maxMetadataSize = 255
+const maxMetadataSize = 1023
+const maxMetadataSizeWritten = 255
 
 // Current/highest supported metadata format.
 const metadataVersion = 1
@@ -152,6 +153,7 @@ Normally should contain a ':' and a path, e.g. "myremote:path/to/dir",
 		}, {
 			Name:     "name_format",
 			Advanced: true,
+			Hide:     fs.OptionHideCommandLine,
 			Default:  `*.rclone_chunk.###`,
 			Help: `String format of chunk file names.
 The two placeholders are: base file name (*) and chunk number (#...).
@@ -162,12 +164,14 @@ Possible chunk files are ignored if their name does not match given format.`,
 		}, {
 			Name:     "start_from",
 			Advanced: true,
+			Hide:     fs.OptionHideCommandLine,
 			Default:  1,
 			Help: `Minimum valid chunk number. Usually 0 or 1.
 By default chunk numbers start from 1.`,
 		}, {
 			Name:     "meta_format",
 			Advanced: true,
+			Hide:     fs.OptionHideCommandLine,
 			Default:  "simplejson",
 			Help: `Format of the metadata object or "none". By default "simplejson".
 Metadata is a small JSON file named after the composite file.`,
@@ -725,6 +729,9 @@ func (f *Fs) processEntries(ctx context.Context, origEntries fs.DirEntries, dirP
 					fs.Infof(f, "ignore non-data chunk %q", remote)
 				}
 				// need to read metadata to ensure actual object type
+				// no need to read if metaobject is too big or absent,
+				// use the fact that before calling validate()
+				// the `size` field caches metaobject size, if any
 				if f.useMeta && mainObject != nil && mainObject.size <= maxMetadataSize {
 					mainObject.unsure = true
 				}
@@ -802,9 +809,10 @@ func (f *Fs) scanObject(ctx context.Context, remote string, quickScan bool) (fs.
 	}
 
 	var (
-		o       *Object
-		baseObj fs.Object
-		err     error
+		o        *Object
+		baseObj  fs.Object
+		err      error
+		sameMain bool
 	)
 
 	if f.useMeta {
@@ -818,6 +826,7 @@ func (f *Fs) scanObject(ctx context.Context, remote string, quickScan bool) (fs.
 		// as a hard limit. Anything larger than that is treated as a
 		// non-chunked file without even checking its contents, so it's
 		// paramount to prevent metadata from exceeding the maximum size.
+		// Anything smaller is additionally checked for format.
 		o = f.newObject("", baseObj, nil)
 		if o.size > maxMetadataSize {
 			return o, nil
@@ -847,18 +856,27 @@ func (f *Fs) scanObject(ctx context.Context, remote string, quickScan bool) (fs.
 		return nil, errors.Wrap(err, "can't detect composite file")
 	}
 
+	caseInsensitive := f.features.CaseInsensitive
 	for _, dirOrObject := range entries {
 		entry, ok := dirOrObject.(fs.Object)
 		if !ok {
 			continue
 		}
 		entryRemote := entry.Remote()
-		if !strings.Contains(entryRemote, remote) {
+		if !caseInsensitive && !strings.Contains(entryRemote, remote) {
 			continue // bypass regexp to save cpu
 		}
 		mainRemote, chunkNo, ctrlType, xactID := f.parseChunkName(entryRemote)
-		if mainRemote == "" || mainRemote != remote {
-			continue // skip non-conforming chunks
+		if mainRemote == "" {
+			continue // skip non-chunks
+		}
+		if caseInsensitive {
+			sameMain = strings.EqualFold(mainRemote, remote)
+		} else {
+			sameMain = mainRemote == remote
+		}
+		if !sameMain {
+			continue // skip alien chunks
 		}
 		if ctrlType != "" || xactID != "" {
 			if f.useMeta {
@@ -906,11 +924,22 @@ func (f *Fs) scanObject(ctx context.Context, remote string, quickScan bool) (fs.
 	return o, nil
 }
 
+// readMetadata reads composite object metadata and caches results,
+// in case of critical errors metadata is not cached.
+// Returns ErrMetaUnknown if an unsupported metadata format is detected.
+// If object is not chunked but marked by List or NewObject for recheck,
+// readMetadata will attempt to parse object as composite with fallback
+// to non-chunked representation if the attempt fails.
 func (o *Object) readMetadata(ctx context.Context) error {
+	// return quickly if metadata is absent or has been already cached
+	if !o.f.useMeta {
+		o.isFull = true
+	}
 	if o.isFull {
 		return nil
 	}
-	if !o.f.useMeta || (!o.isComposite() && !o.unsure) {
+	if !o.isComposite() && !o.unsure {
+		// this for sure is a non-chunked standalone file
 		o.isFull = true
 		return nil
 	}
@@ -928,6 +957,7 @@ func (o *Object) readMetadata(ctx context.Context) error {
 		return ErrMetaTooBig
 	}
 
+	// size is within limits, perform consistency checks
 	reader, err := metaObject.Open(ctx)
 	if err != nil {
 		return err
@@ -965,7 +995,7 @@ func (o *Object) readMetadata(ctx context.Context) error {
 		o.sha1 = metaInfo.sha1
 	}
 
-	o.isFull = true
+	o.isFull = true // cache results
 	return nil
 }
 
@@ -974,11 +1004,14 @@ func (f *Fs) put(
 	ctx context.Context, in io.Reader, src fs.ObjectInfo, remote string, options []fs.OpenOption,
 	basePut putFn, action string, target fs.Object) (obj fs.Object, err error) {
 
+	// Perform consistency checks
 	if err := f.forbidChunk(src, remote); err != nil {
 		return nil, errors.Wrap(err, action+" refused")
 	}
 	if target == nil {
 		// Get target object with a quick directory scan
+		// skip metadata check if target object does not exist.
+		// ignore not-chunked objects, skip chunk size checks.
 		if obj, err := f.scanObject(ctx, remote, true); err == nil {
 			target = obj
 		}
@@ -991,6 +1024,7 @@ func (f *Fs) put(
 		}
 	}
 
+	// Prepare to upload
 	c := f.newChunkingReader(src)
 	wrapIn := c.wrapStream(ctx, in, src)
 
@@ -1593,6 +1627,8 @@ func (f *Fs) okForServerSide(ctx context.Context, src fs.Object, opName string)
 		diff = "chunk sizes"
 	case f.opt.NameFormat != obj.f.opt.NameFormat:
 		diff = "chunk name formats"
+	case f.opt.StartFrom != obj.f.opt.StartFrom:
+		diff = "chunk numbering"
 	case f.opt.MetaFormat != obj.f.opt.MetaFormat:
 		diff = "meta formats"
 	}
@@ -1821,6 +1857,9 @@ func (o *Object) addChunk(chunk fs.Object, chunkNo int) error {
 		copy(newChunks, o.chunks)
 		o.chunks = newChunks
 	}
+	if o.chunks[chunkNo] != nil {
+		return fmt.Errorf("duplicate chunk number %d", chunkNo+o.f.opt.StartFrom)
+	}
 	o.chunks[chunkNo] = chunk
 	return nil
 }
@@ -2248,15 +2287,17 @@ func marshalSimpleJSON(ctx context.Context, size int64, nChunks int, md5, sha1 s
 		SHA1: sha1,
 	}
 	data, err := json.Marshal(&metadata)
-	if err == nil && data != nil && len(data) >= maxMetadataSize {
+	if err == nil && data != nil && len(data) >= maxMetadataSizeWritten {
 		// be a nitpicker, never produce something you can't consume
 		return nil, errors.New("metadata can't be this big, please report to rclone developers")
 	}
 	return data, err
 }
 
-// unmarshalSimpleJSON
+// unmarshalSimpleJSON parses metadata.
 //
+// In case of errors returns a flag telling whether input has been
+// produced by incompatible version of rclone vs wasn't metadata at all.
 // Only metadata format version 1 is supported atm.
 // Future releases will transparently migrate older metadata objects.
 // New format will have a higher version number and cannot be correctly
@@ -2266,7 +2307,7 @@ func marshalSimpleJSON(ctx context.Context, size int64, nChunks int, md5, sha1 s
 func unmarshalSimpleJSON(ctx context.Context, metaObject fs.Object, data []byte) (info *ObjectInfo, madeByChunker bool, err error) {
 	// Be strict about JSON format
 	// to reduce possibility that a random small file resembles metadata.
-	if data != nil && len(data) > maxMetadataSize {
+	if data != nil && len(data) > maxMetadataSizeWritten {
 		return nil, false, ErrMetaTooBig
 	}
 	if data == nil || len(data) < 2 || data[0] != '{' || data[len(data)-1] != '}' {
diff --git a/docs/content/chunker.md b/docs/content/chunker.md
index 2250808a6..08f351304 100644
--- a/docs/content/chunker.md
+++ b/docs/content/chunker.md
@@ -299,6 +299,9 @@ If wrapped remote is case insensitive, the chunker overlay will inherit
 that property (so you can't have a file called "Hello.doc" and "hello.doc"
 in the same directory).
 
+Chunker included in rclone releases up to `v1.54` can sometimes fail to
+detect metadata produced by recent versions of rclone. We recommend users
+to keep rclone up-to-date to avoid data corruption.
 
 {{< rem autogenerated options start" - DO NOT EDIT - instead edit fs.RegInfo in backend/chunker/chunker.go then run make backenddocs" >}}
 ### Standard Options


Test patch:
diff --git a/backend/chunker/chunker_internal_test.go b/backend/chunker/chunker_internal_test.go
index 6ba6890d2..d34c0abef 100644
--- a/backend/chunker/chunker_internal_test.go
+++ b/backend/chunker/chunker_internal_test.go
@@ -13,6 +13,7 @@ import (
 
 	"github.com/rclone/rclone/fs"
 	"github.com/rclone/rclone/fs/hash"
+	"github.com/rclone/rclone/fs/object"
 	"github.com/rclone/rclone/fs/operations"
 	"github.com/rclone/rclone/fstest"
 	"github.com/rclone/rclone/fstest/fstests"
@@ -663,6 +664,80 @@ func testMetadataInput(t *testing.T, f *Fs) {
 	runSubtest(futureMeta, "future")
 }
 
+// test that chunker refuses to change on objects with future/unknowm metadata
+func testFutureProof(t *testing.T, f *Fs) {
+	if f.opt.MetaFormat == "none" {
+		t.Skip("this test requires metadata support")
+	}
+
+	saveOpt := f.opt
+	ctx := context.Background()
+	f.opt.FailHard = true
+	const dir = "future"
+	const file = dir + "/test"
+	defer func() {
+		f.opt.FailHard = false
+		_ = operations.Purge(ctx, f.base, dir)
+		f.opt = saveOpt
+	}()
+
+	modTime := fstest.Time("2001-02-03T04:05:06.499999999Z")
+	putPart := func(name string, part int, data, msg string) {
+		if part > 0 {
+			name = f.makeChunkName(name, part-1, "", "")
+		}
+		item := fstest.Item{Path: name, ModTime: modTime}
+		_, obj := fstests.PutTestContents(ctx, t, f.base, &item, data, true)
+		assert.NotNil(t, obj, msg)
+	}
+
+	// simulate chunked object from future
+	meta := `{"ver":999,"nchunks":3,"size":9,"garbage":"litter","sha1":"0707f2970043f9f7c22029482db27733deaec029"}`
+	putPart(file, 0, meta, "metaobject")
+	putPart(file, 1, "abc", "chunk1")
+	putPart(file, 2, "def", "chunk2")
+	putPart(file, 3, "ghi", "chunk3")
+
+	// List should succeed
+	ls, err := f.List(ctx, dir)
+	assert.NoError(t, err)
+	assert.Equal(t, 1, len(ls))
+	assert.Equal(t, int64(9), ls[0].Size())
+
+	// NewObject should succeed
+	obj, err := f.NewObject(ctx, file)
+	assert.NoError(t, err)
+	assert.Equal(t, file, obj.Remote())
+	assert.Equal(t, int64(9), obj.Size())
+
+	// Hash must fail
+	_, err = obj.Hash(ctx, hash.SHA1)
+	assert.Equal(t, ErrMetaUnknown, err)
+
+	// Move must fail
+	mobj, err := operations.Move(ctx, f, nil, file+"2", obj)
+	assert.Nil(t, mobj)
+	assert.Error(t, err)
+	if err != nil {
+		assert.Contains(t, err.Error(), "please upgrade rclone")
+	}
+
+	// Put must fail
+	oi := object.NewStaticObjectInfo(file, modTime, 3, true, nil, nil)
+	buf := bytes.NewBufferString("abc")
+	_, err = f.Put(ctx, buf, oi)
+	assert.Error(t, err)
+
+	// Rcat must fail
+	in := ioutil.NopCloser(bytes.NewBufferString("abc"))
+	robj, err := operations.Rcat(ctx, f, file, in, modTime)
+	assert.Nil(t, robj)
+	assert.NotNil(t, err)
+	if err != nil {
+		assert.Contains(t, err.Error(), "please upgrade rclone")
+	}
+}
+
 // InternalTest dispatches all internal tests
 func (f *Fs) InternalTest(t *testing.T) {
 	t.Run("PutLarge", func(t *testing.T) {
@@ -686,6 +761,9 @@ func (f *Fs) InternalTest(t *testing.T) {
 	t.Run("MetadataInput", func(t *testing.T) {
 		testMetadataInput(t, f)
 	})
+	t.Run("FutureProof", func(t *testing.T) {
+		testFutureProof(t, f)
+	})
 }
 
 var _ fstests.InternalTester = (*Fs)(nil)