From 97a307df1ab111e9307e7eb5d341b5032b36b842 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Thu, 9 May 2024 18:21:53 +0200 Subject: [PATCH] cache: Always use cached file if it exists A file is always cached whole. Thus, any out of bounds access will also fail when directed at the backend. To handle case in which the cached file is broken, then caller must call Cache.Forget(h) for the file in question. --- internal/cache/backend.go | 7 ++++--- internal/cache/file.go | 22 ++++++++++++---------- internal/cache/file_test.go | 24 +++++++++++++----------- 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/internal/cache/backend.go b/internal/cache/backend.go index 27b37e9d9..b0edfcbe5 100644 --- a/internal/cache/backend.go +++ b/internal/cache/backend.go @@ -40,7 +40,8 @@ func (b *Backend) Remove(ctx context.Context, h backend.Handle) error { return err } - return b.Cache.remove(h) + err = b.Cache.remove(h) + return err } func autoCacheTypes(h backend.Handle) bool { @@ -133,9 +134,9 @@ func (b *Backend) cacheFile(ctx context.Context, h backend.Handle) error { // loadFromCache will try to load the file from the cache. func (b *Backend) loadFromCache(h backend.Handle, length int, offset int64, consumer func(rd io.Reader) error) (bool, error) { - rd, err := b.Cache.load(h, length, offset) + rd, inCache, err := b.Cache.load(h, length, offset) if err != nil { - return false, err + return inCache, err } err = consumer(rd) diff --git a/internal/cache/file.go b/internal/cache/file.go index 921add24c..b54bd806d 100644 --- a/internal/cache/file.go +++ b/internal/cache/file.go @@ -34,46 +34,48 @@ func (c *Cache) canBeCached(t backend.FileType) bool { // load returns a reader that yields the contents of the file with the // given handle. rd must be closed after use. If an error is returned, the -// ReadCloser is nil. -func (c *Cache) load(h backend.Handle, length int, offset int64) (io.ReadCloser, error) { +// ReadCloser is nil. The bool return value indicates whether the requested +// file exists in the cache. It can be true even when no reader is returned +// because length or offset are out of bounds +func (c *Cache) load(h backend.Handle, length int, offset int64) (io.ReadCloser, bool, error) { debug.Log("Load(%v, %v, %v) from cache", h, length, offset) if !c.canBeCached(h.Type) { - return nil, errors.New("cannot be cached") + return nil, false, errors.New("cannot be cached") } f, err := fs.Open(c.filename(h)) if err != nil { - return nil, errors.WithStack(err) + return nil, false, errors.WithStack(err) } fi, err := f.Stat() if err != nil { _ = f.Close() - return nil, errors.WithStack(err) + return nil, true, errors.WithStack(err) } size := fi.Size() if size <= int64(crypto.CiphertextLength(0)) { _ = f.Close() - return nil, errors.Errorf("cached file %v is truncated", h) + return nil, true, errors.Errorf("cached file %v is truncated", h) } if size < offset+int64(length) { _ = f.Close() - return nil, errors.Errorf("cached file %v is too short", h) + return nil, true, errors.Errorf("cached file %v is too short", h) } if offset > 0 { if _, err = f.Seek(offset, io.SeekStart); err != nil { _ = f.Close() - return nil, err + return nil, true, err } } if length <= 0 { - return f, nil + return f, true, nil } - return util.LimitReadCloser(f, int64(length)), nil + return util.LimitReadCloser(f, int64(length)), true, nil } // save saves a file in the cache. diff --git a/internal/cache/file_test.go b/internal/cache/file_test.go index 2ce238888..331e3251d 100644 --- a/internal/cache/file_test.go +++ b/internal/cache/file_test.go @@ -14,7 +14,7 @@ import ( "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/fs" "github.com/restic/restic/internal/restic" - "github.com/restic/restic/internal/test" + rtest "github.com/restic/restic/internal/test" "golang.org/x/sync/errgroup" ) @@ -22,7 +22,7 @@ import ( func generateRandomFiles(t testing.TB, tpe backend.FileType, c *Cache) restic.IDSet { ids := restic.NewIDSet() for i := 0; i < rand.Intn(15)+10; i++ { - buf := test.Random(rand.Int(), 1<<19) + buf := rtest.Random(rand.Int(), 1<<19) id := restic.Hash(buf) h := backend.Handle{Type: tpe, Name: id.String()} @@ -48,10 +48,11 @@ func randomID(s restic.IDSet) restic.ID { } func load(t testing.TB, c *Cache, h backend.Handle) []byte { - rd, err := c.load(h, 0, 0) + rd, inCache, err := c.load(h, 0, 0) if err != nil { t.Fatal(err) } + rtest.Equals(t, true, inCache, "expected inCache flag to be true") if rd == nil { t.Fatalf("load() returned nil reader") @@ -144,7 +145,7 @@ func TestFileLoad(t *testing.T) { c := TestNewCache(t) // save about 5 MiB of data in the cache - data := test.Random(rand.Int(), 5234142) + data := rtest.Random(rand.Int(), 5234142) id := restic.ID{} copy(id[:], data) h := backend.Handle{ @@ -169,10 +170,11 @@ func TestFileLoad(t *testing.T) { for _, test := range tests { t.Run(fmt.Sprintf("%v/%v", test.length, test.offset), func(t *testing.T) { - rd, err := c.load(h, test.length, test.offset) + rd, inCache, err := c.load(h, test.length, test.offset) if err != nil { t.Fatal(err) } + rtest.Equals(t, true, inCache, "expected inCache flag to be true") buf, err := io.ReadAll(rd) if err != nil { @@ -225,7 +227,7 @@ func TestFileSaveConcurrent(t *testing.T) { var ( c = TestNewCache(t) - data = test.Random(1, 10000) + data = rtest.Random(1, 10000) g errgroup.Group id restic.ID ) @@ -245,7 +247,7 @@ func TestFileSaveConcurrent(t *testing.T) { // ensure is ENOENT or nil error. time.Sleep(time.Duration(100+rand.Intn(200)) * time.Millisecond) - f, err := c.load(h, 0, 0) + f, _, err := c.load(h, 0, 0) t.Logf("Load error: %v", err) switch { case err == nil: @@ -264,17 +266,17 @@ func TestFileSaveConcurrent(t *testing.T) { }) } - test.OK(t, g.Wait()) + rtest.OK(t, g.Wait()) saved := load(t, c, h) - test.Equals(t, data, saved) + rtest.Equals(t, data, saved) } func TestFileSaveAfterDamage(t *testing.T) { c := TestNewCache(t) - test.OK(t, fs.RemoveAll(c.path)) + rtest.OK(t, fs.RemoveAll(c.path)) // save a few bytes of data in the cache - data := test.Random(123456789, 42) + data := rtest.Random(123456789, 42) id := restic.Hash(data) h := backend.Handle{ Type: restic.PackFile,