From d03460010ff47f1beeaf376d40c5423ee238ab58 Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Sat, 15 Oct 2022 14:12:45 +0200 Subject: [PATCH 1/4] internal/restic: Fix ID.UnmarshalJSON, ParseID ID.UnmarshalJSON accepted non-JSON input with ' as the string delimiter. Also, the error message for non-hex input was less informative than it could be and it performed too many checks. Changed ParseID to keep the error messages consistent. --- internal/restic/id.go | 40 ++++++++++++---------------------------- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/internal/restic/id.go b/internal/restic/id.go index 5a25e0ebe..e71c6d71b 100644 --- a/internal/restic/id.go +++ b/internal/restic/id.go @@ -6,8 +6,6 @@ import ( "fmt" "io" - "github.com/restic/restic/internal/errors" - "github.com/minio/sha256-simd" ) @@ -24,14 +22,13 @@ type ID [idSize]byte // ParseID converts the given string to an ID. func ParseID(s string) (ID, error) { - b, err := hex.DecodeString(s) - - if err != nil { - return ID{}, errors.Wrap(err, "hex.DecodeString") + if len(s) != hex.EncodedLen(idSize) { + return ID{}, fmt.Errorf("invalid length for ID: %q", s) } - if len(b) != idSize { - return ID{}, errors.New("invalid length for hash") + b, err := hex.DecodeString(s) + if err != nil { + return ID{}, fmt.Errorf("invalid ID: %s", err) } id := ID{} @@ -96,34 +93,21 @@ func (id ID) MarshalJSON() ([]byte, error) { // UnmarshalJSON parses the JSON-encoded data and stores the result in id. func (id *ID) UnmarshalJSON(b []byte) error { // check string length - if len(b) < 2 { - return fmt.Errorf("invalid ID: %q", b) + if len(b) != len(`""`)+hex.EncodedLen(idSize) { + return fmt.Errorf("invalid length for ID: %q", b) } - if len(b)%2 != 0 { - return fmt.Errorf("invalid ID length: %q", b) - } - - // check string delimiters - if b[0] != '"' && b[0] != '\'' { + if b[0] != '"' { return fmt.Errorf("invalid start of string: %q", b[0]) } - last := len(b) - 1 - if b[0] != b[last] { - return fmt.Errorf("starting string delimiter (%q) does not match end (%q)", b[0], b[last]) - } - - // strip JSON string delimiters - b = b[1:last] - - if len(b) != 2*len(id) { - return fmt.Errorf("invalid length for ID") - } + // Strip JSON string delimiters. The json.Unmarshaler contract says we get + // a valid JSON value, so we don't need to check that b[len(b)-1] == '"'. + b = b[1 : len(b)-1] _, err := hex.Decode(id[:], b) if err != nil { - return errors.Wrap(err, "hex.Decode") + return fmt.Errorf("invalid ID: %s", err) } return nil From 22147e1e02a440ff7e19f96049195e050bdab88d Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Sun, 16 Oct 2022 10:30:59 +0200 Subject: [PATCH 2/4] all: Minor cleanups if x { return true } return false => return x fmt.Sprintf("%v", x) => fmt.Sprint(x) or x.String() The fmt.Sprintf idiom is still used in the SecretString tests, where it serves security hardening. --- internal/archiver/testing_test.go | 2 +- internal/cache/file_test.go | 2 +- internal/restic/blob.go | 8 ++------ internal/restic/ids.go | 2 +- internal/restic/tag_list.go | 2 +- 5 files changed, 6 insertions(+), 10 deletions(-) diff --git a/internal/archiver/testing_test.go b/internal/archiver/testing_test.go index e11b86250..6f6904e3f 100644 --- a/internal/archiver/testing_test.go +++ b/internal/archiver/testing_test.go @@ -217,7 +217,7 @@ func TestTestWalkFiles(t *testing.T) { return err } - got[p] = fmt.Sprintf("%v", item) + got[p] = fmt.Sprint(item) return nil }) diff --git a/internal/cache/file_test.go b/internal/cache/file_test.go index 3e92c1d96..17fc5048f 100644 --- a/internal/cache/file_test.go +++ b/internal/cache/file_test.go @@ -97,7 +97,7 @@ func TestFiles(t *testing.T) { } for _, tpe := range tests { - t.Run(fmt.Sprintf("%v", tpe), func(t *testing.T) { + t.Run(tpe.String(), func(t *testing.T) { ids := generateRandomFiles(t, tpe, c) id := randomID(ids) diff --git a/internal/restic/blob.go b/internal/restic/blob.go index 4ac149adb..260f40fde 100644 --- a/internal/restic/blob.go +++ b/internal/restic/blob.go @@ -114,11 +114,7 @@ func (h BlobHandles) Less(i, j int) bool { continue } - if b < h[j].ID[k] { - return true - } - - return false + return b < h[j].ID[k] } return h[i].Type < h[j].Type @@ -133,5 +129,5 @@ func (h BlobHandles) String() string { for _, e := range h { elements = append(elements, e.String()) } - return fmt.Sprintf("%v", elements) + return fmt.Sprint(elements) } diff --git a/internal/restic/ids.go b/internal/restic/ids.go index cc5ad18da..b3e6b6f22 100644 --- a/internal/restic/ids.go +++ b/internal/restic/ids.go @@ -65,5 +65,5 @@ func (ids IDs) String() string { for _, id := range ids { elements = append(elements, shortID(id)) } - return fmt.Sprintf("%v", elements) + return fmt.Sprint(elements) } diff --git a/internal/restic/tag_list.go b/internal/restic/tag_list.go index 4d57cb14b..b293b14f7 100644 --- a/internal/restic/tag_list.go +++ b/internal/restic/tag_list.go @@ -37,7 +37,7 @@ func (TagList) Type() string { type TagLists []TagList func (l TagLists) String() string { - return fmt.Sprintf("%v", []TagList(l)) + return fmt.Sprint([]TagList(l)) } // Flatten returns the list of all tags provided in TagLists From b513597546f1f7958d1fc46288d791aa2cc89bee Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Sun, 16 Oct 2022 10:36:37 +0200 Subject: [PATCH 3/4] internal/restic: Make FileType a uint8 instead of a string The string form was presumably useful before the introduction of layouts, but right now it just makes call sequences and garbage collection more expensive (the latter because every string contains a pointer to be scanned). --- internal/restic/file.go | 40 +++++++++++++++++++++++++----------- internal/restic/file_test.go | 30 +++++++++++++++++---------- 2 files changed, 47 insertions(+), 23 deletions(-) diff --git a/internal/restic/file.go b/internal/restic/file.go index d058a71c0..0e9f046ae 100644 --- a/internal/restic/file.go +++ b/internal/restic/file.go @@ -7,18 +7,38 @@ import ( ) // FileType is the type of a file in the backend. -type FileType string +type FileType uint8 // These are the different data types a backend can store. const ( - PackFile FileType = "data" // use data, as packs are stored under /data in repo - KeyFile FileType = "key" - LockFile FileType = "lock" - SnapshotFile FileType = "snapshot" - IndexFile FileType = "index" - ConfigFile FileType = "config" + PackFile FileType = 1 + iota + KeyFile + LockFile + SnapshotFile + IndexFile + ConfigFile ) +func (t FileType) String() string { + s := "invalid" + switch t { + case PackFile: + // Spelled "data" instead of "pack" for historical reasons. + s = "data" + case KeyFile: + s = "key" + case LockFile: + s = "lock" + case SnapshotFile: + s = "snapshot" + case IndexFile: + s = "index" + case ConfigFile: + s = "config" + } + return s +} + // Handle is used to store and access data in a backend. type Handle struct { Type FileType @@ -36,10 +56,6 @@ func (h Handle) String() string { // Valid returns an error if h is not valid. func (h Handle) Valid() error { - if h.Type == "" { - return errors.New("type is empty") - } - switch h.Type { case PackFile: case KeyFile: @@ -48,7 +64,7 @@ func (h Handle) Valid() error { case IndexFile: case ConfigFile: default: - return errors.Errorf("invalid Type %q", h.Type) + return errors.Errorf("invalid Type %d", h.Type) } if h.Type == ConfigFile { diff --git a/internal/restic/file_test.go b/internal/restic/file_test.go index 76f00baac..cc54c2924 100644 --- a/internal/restic/file_test.go +++ b/internal/restic/file_test.go @@ -1,20 +1,28 @@ package restic -import "testing" +import ( + "testing" -var handleTests = []struct { - h Handle - valid bool -}{ - {Handle{Name: "foo"}, false}, - {Handle{Type: "foobar"}, false}, - {Handle{Type: ConfigFile, Name: ""}, true}, - {Handle{Type: PackFile, Name: ""}, false}, - {Handle{Type: "", Name: "x"}, false}, - {Handle{Type: LockFile, Name: "010203040506"}, true}, + rtest "github.com/restic/restic/internal/test" +) + +func TestHandleString(t *testing.T) { + rtest.Equals(t, "", Handle{Type: PackFile, Name: "foobar"}.String()) + rtest.Equals(t, "", Handle{Type: LockFile, Name: "1"}.String()) } func TestHandleValid(t *testing.T) { + var handleTests = []struct { + h Handle + valid bool + }{ + {Handle{Name: "foo"}, false}, + {Handle{Type: 0}, false}, + {Handle{Type: ConfigFile, Name: ""}, true}, + {Handle{Type: PackFile, Name: ""}, false}, + {Handle{Type: LockFile, Name: "010203040506"}, true}, + } + for i, test := range handleTests { err := test.h.Valid() if err != nil && test.valid { From 9adae5521d3d148e8c1716e9e69cad227feefbdb Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Fri, 21 Oct 2022 14:32:06 +0200 Subject: [PATCH 4/4] cache: Call interface method once --- internal/cache/file.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/cache/file.go b/internal/cache/file.go index 490b2b311..8dafdfc56 100644 --- a/internal/cache/file.go +++ b/internal/cache/file.go @@ -59,13 +59,14 @@ func (c *Cache) load(h restic.Handle, length int, offset int64) (io.ReadCloser, return nil, errors.WithStack(err) } - if fi.Size() <= int64(crypto.CiphertextLength(0)) { + size := fi.Size() + if size <= int64(crypto.CiphertextLength(0)) { _ = f.Close() _ = c.remove(h) return nil, errors.Errorf("cached file %v is truncated, removing", h) } - if fi.Size() < offset+int64(length) { + if size < offset+int64(length) { _ = f.Close() _ = c.remove(h) return nil, errors.Errorf("cached file %v is too small, removing", h)