From 1cab18fd8f9e456bb9170e74428f7d0a7fe47da5 Mon Sep 17 00:00:00 2001
From: Paul Banks <pbanks@hashicorp.com>
Date: Mon, 23 Jan 2023 14:37:12 +0000
Subject: [PATCH] Fix logical race caused by read-buffer sharing

---
 codec.go          |  6 ++++--
 codec_test.go     | 33 +++++++++++++++++++++++++++++++++
 wal_stubs_test.go | 15 ++++++++++++++-
 wal_test.go       |  3 +++
 4 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/codec.go b/codec.go
index e591f0f..c3d0315 100644
--- a/codec.go
+++ b/codec.go
@@ -37,7 +37,8 @@ type Codec interface {
 
 	// Decode a log from the passed byte slice into the log entry pointed to. This
 	// allows the caller to manage allocation and re-use of the bytes and log
-	// entry.
+	// entry. The resulting raft.Log MUST NOT reference data in the input byte
+	// slice since the input byte slice may be re-used for
 	Decode([]byte, *raft.Log) error
 }
 
@@ -148,7 +149,8 @@ func (d *decoder) bytes() []byte {
 		d.err = io.ErrShortBuffer
 		return nil
 	}
-	bs := d.buf[:n]
+	bs := make([]byte, n)
+	copy(bs, d.buf[:n])
 	d.buf = d.buf[n:]
 	return bs
 }
diff --git a/codec_test.go b/codec_test.go
index 4e71e18..045b333 100644
--- a/codec_test.go
+++ b/codec_test.go
@@ -59,3 +59,36 @@ func TestBinaryCodecFuzz(t *testing.T) {
 		require.Equal(t, log, log2)
 	}
 }
+
+func TestBinaryCodecCopysOnDecode(t *testing.T) {
+	var in, out raft.Log
+
+	in.Index = 1234
+	in.Term = 2
+	in.Type = raft.LogCommand
+	in.Data = []byte("foo")
+	in.Extensions = []byte("ext")
+
+	c := BinaryCodec{}
+	var buf bytes.Buffer
+	require.NoError(t, c.Encode(&in, &buf))
+
+	rawBytes := buf.Bytes()
+
+	require.NoError(t, c.Decode(rawBytes, &out))
+
+	// Make sure the decoded data is the same
+	require.Equal(t, string(out.Data), "foo")
+	require.Equal(t, string(out.Extensions), "ext")
+
+	// Intentionally mangle the buffer contents
+	for i := 0; i < len(rawBytes); i++ {
+		rawBytes[i] = 'x'
+	}
+
+	// Make sure the decoded data is still the same (i.e. didn't refer to the
+	// underlying bytes)
+	require.Equal(t, string(out.Data), "foo")
+	require.Equal(t, string(out.Extensions), "ext")
+
+}
diff --git a/wal_stubs_test.go b/wal_stubs_test.go
index f94ec64..a20008e 100644
--- a/wal_stubs_test.go
+++ b/wal_stubs_test.go
@@ -513,8 +513,21 @@ func (s *testSegment) GetLog(idx uint64) (*types.PooledBuffer, error) {
 	if !ok {
 		return nil, ErrNotFound
 	}
+
+	// We make a pooled buffer with it's own copy of the log which we then
+	// invalidate when it's Closed to make sure that any test code that reads data
+	// from a buffer that was freed fails to read what it expected. This ensures
+	// our logic about returning slices to users that we might re-use is correct.
+	buf := make([]byte, len(log.Data))
+	copy(buf, log.Data)
 	pb := &types.PooledBuffer{
-		Bs: log.Data,
+		Bs: buf,
+		CloseFn: func() {
+			closed := []byte{'c', 'l', 'o', 's', 'e', 'd', ' ', 'b', 'u', 'f', 'f', 'e', 'r', '!'}
+			for i := 0; i < len(buf); i++ {
+				buf[i] = closed[i%len(closed)]
+			}
+		},
 	}
 	return pb, nil
 }
diff --git a/wal_test.go b/wal_test.go
index a3a20ae..3cd21d4 100644
--- a/wal_test.go
+++ b/wal_test.go
@@ -852,6 +852,9 @@ func TestConcurrentReadersAndWriter(t *testing.T) {
 				if log.Index != idx {
 					panic(fmt.Errorf("wrong %s log want=%d got=%d", name, idx, log.Index))
 				}
+				if string(log.Data) != fmt.Sprintf("Log entry %d", log.Index) {
+					panic(fmt.Errorf("wrong data in log %d: %s", log.Index, log.Data))
+				}
 				return 1
 			}