From ac47489509bd5298c127fe88655d6a71630dee09 Mon Sep 17 00:00:00 2001 From: Jack Lin Date: Wed, 17 Jan 2024 10:59:22 +0800 Subject: [PATCH] fix: rewrite the read function to correctly handle out of bound issue ref: longhorn/longhonr 6899 Signed-off-by: Jack Lin --- pkg/replica/diff_disk.go | 44 ++++++++++------- pkg/replica/replica_test.go | 98 +++++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 17 deletions(-) diff --git a/pkg/replica/diff_disk.go b/pkg/replica/diff_disk.go index 64405ebff..8318f1668 100644 --- a/pkg/replica/diff_disk.go +++ b/pkg/replica/diff_disk.go @@ -220,32 +220,42 @@ func (d *diffDisk) fullReadAt(buf []byte, offset int64) (int, error) { return count, nil } -func (d *diffDisk) read(target byte, buf []byte, startOffset int64, startSector int64, sectors int64) (int, error) { +// read called by fullReadAt reads the data from the target disk into buf +// offset is the offset of the whole volume +// startSector indicates the sector in the buffer from which the data should be read from the target disk. +// sectors indicates how many sectors we are going to read +func (d *diffDisk) read(target byte, buf []byte, offset int64, startSector int64, sectors int64) (int, error) { bufStart := startSector * d.sectorSize bufLength := sectors * d.sectorSize - offset := startOffset + bufStart - size, err := d.files[target].Size() + diskReadStartOffset := offset + bufStart + diskReadEndOffset := diskReadStartOffset + bufLength + diskSize, err := d.files[target].Size() if err != nil { return 0, err } + volumeSize := d.size - // Reading the out-of-bound part is not allowed - if bufLength > d.size-offset { - logrus.Warn("Trying to read the out-of-bound part") - return 0, io.ErrUnexpectedEOF - } + count := 0 - // May read the expanded part - if offset >= size { - return 0, nil + if diskReadStartOffset < diskSize { + newBuf := buf[bufStart : bufStart+min(bufLength, diskSize-diskReadStartOffset)] + n, err := d.files[target].ReadAt(newBuf, diskReadStartOffset) + if err != nil { + return n, err + } + count += n + } + if diskReadEndOffset <= diskSize { + return count, nil } - var newBuf []byte - if bufLength > size-offset { - newBuf = buf[bufStart : bufStart+size-offset] - } else { - newBuf = buf[bufStart : bufStart+bufLength] + + // Read out of disk boundary but still in Volume boundary, we should remain the valid range with 0 and return the correct count + if diskReadEndOffset <= volumeSize { + return count + int(diskReadEndOffset-max(diskSize, diskReadStartOffset)), nil } - return d.files[target].ReadAt(newBuf, offset) + + // Read out of Volume boundary, we should remain the valid range with 0 and return the correct count with EOF error + return count + int(volumeSize-max(diskSize, diskReadStartOffset)), io.ErrUnexpectedEOF } func (d *diffDisk) lookup(sector int64) (byte, error) { diff --git a/pkg/replica/replica_test.go b/pkg/replica/replica_test.go index 34f5534e8..44d68ff0d 100644 --- a/pkg/replica/replica_test.go +++ b/pkg/replica/replica_test.go @@ -3,6 +3,7 @@ package replica import ( "crypto/md5" "fmt" + "io" "os" "path" "strconv" @@ -693,6 +694,103 @@ func (s *TestSuite) TestRead(c *C) { byteEquals(c, buf, make([]byte, 3*b)) } +func (s *TestSuite) TestReadBackingFileSmallerThanVolume(c *C) { + dir, err := os.MkdirTemp("", "replica") + c.Assert(err, IsNil) + defer os.RemoveAll(dir) + + // Test read from BackingImage which is smaller than Volume + // BackinImage: [0:3b], Volume: [0:5b] + buf := make([]byte, 3*b) + fill(buf, 3) + + f, err := NewTestBackingFile(path.Join(dir, "backing")) + c.Assert(err, IsNil) + defer f.Close() + _, err = f.Write(buf) + c.Assert(err, IsNil) + + backing := &backingfile.BackingFile{ + Path: "backing", + Disk: f, + } + + r, err := New(5*b, b, dir, backing, false, false, 250, 0) + c.Assert(err, IsNil) + defer r.Close() + + // Test Case 1: read within BackingImage boundary (read [1b:3b]) + expectedData := make([]byte, 2*b) + fill(expectedData, 3) + + data := make([]byte, 2*b) + n, err := r.ReadAt(data, 1*b) + c.Assert(err, IsNil) + c.Assert(n, Equals, 2*b) + byteEquals(c, expectedData, data) + + // Test Case 2: read half within BackingImage boundary (read [2b:4b]) + expectedData = make([]byte, 2*b) + fill(expectedData[0:b], 3) + + data = make([]byte, 2*b) + n, err = r.ReadAt(data, 2*b) + c.Assert(err, IsNil) + c.Assert(n, Equals, 2*b) + byteEquals(c, expectedData, data) + + // Test Case 3: read out of BackingImage boundary but within Volume boundary (read [3b:5b]) + expectedData = make([]byte, 2*b) + fill(expectedData, 0) + + data = make([]byte, 2*b) + n, err = r.ReadAt(data, 3*b) + c.Assert(err, IsNil) + c.Assert(n, Equals, 2*b) + byteEquals(c, expectedData, data) + + // Test Case 4: read half within Volume boundary (read [4b:6b]) + expectedData = make([]byte, 2*b) + fill(expectedData, 0) + + data = make([]byte, 2*b) + n, err = r.ReadAt(data, 4*b) + c.Assert(err, Equals, io.ErrUnexpectedEOF) + c.Assert(n, Equals, b) + byteEquals(c, expectedData, data) + + // Test Case 5: read out of Volume boundary (read [5b:7b]) + expectedData = make([]byte, 2*b) + fill(expectedData, 0) + + data = make([]byte, 2*b) + n, err = r.ReadAt(data, 5*b) + c.Assert(err, Equals, io.ErrUnexpectedEOF) + c.Assert(n, Equals, 0) + byteEquals(c, expectedData, data) + + // Test Case 6: read [2b:6b] + expectedData = make([]byte, 4*b) + fill(expectedData[0:b], 3) + fill(expectedData[b:4*b], 0) + + data = make([]byte, 4*b) + n, err = r.ReadAt(data, 2*b) + c.Assert(err, Equals, io.ErrUnexpectedEOF) + c.Assert(n, Equals, 3*b) + byteEquals(c, expectedData, data) + + // Test Case 6: read [3b:6b] + expectedData = make([]byte, 3*b) + fill(expectedData[b:3*b], 0) + + data = make([]byte, 3*b) + n, err = r.ReadAt(data, 3*b) + c.Assert(err, Equals, io.ErrUnexpectedEOF) + c.Assert(n, Equals, 2*b) + byteEquals(c, expectedData, data) +} + func (s *TestSuite) TestWrite(c *C) { dir, err := os.MkdirTemp("", "replica") c.Assert(err, IsNil)