fix: rewrite the read function to correctly handle out of bound issue

ref: longhorn/longhonr 6899

Signed-off-by: Jack Lin <jack.lin@suse.com>

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) {

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)