From 10f7a166dd236a7db142e67a2121fe8465014394 Mon Sep 17 00:00:00 2001
From: Ben Luddy <bluddy@redhat.com>
Date: Thu, 29 Feb 2024 19:21:16 -0500
Subject: [PATCH] Remove shared ref to underlying array of JSONFrameReader's
 Read arg.

When JSONFrameReader read a JSON object longer than the length of the destination slice, but not
larger than the capacity of the destination slice, it would retain a reference to a slice sharing
the same underlying array as the destination slice. If the underlying array is modified between
calls to Read, corrupt frame data could be returned.

This is also called out in the io.Reader contract: "Implementations must not retain p."
---
 .../apimachinery/pkg/util/framer/framer.go     | 18 ++++++++++++------
 .../pkg/util/framer/framer_test.go             | 15 +++++++++++++++
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/staging/src/k8s.io/apimachinery/pkg/util/framer/framer.go b/staging/src/k8s.io/apimachinery/pkg/util/framer/framer.go
index 9b3c9c8d5ac..1ab8fd396ed 100644
--- a/staging/src/k8s.io/apimachinery/pkg/util/framer/framer.go
+++ b/staging/src/k8s.io/apimachinery/pkg/util/framer/framer.go
@@ -147,7 +147,6 @@ func (r *jsonFrameReader) Read(data []byte) (int, error) {
 
 	// RawMessage#Unmarshal appends to data - we reset the slice down to 0 and will either see
 	// data written to data, or be larger than data and a different array.
-	n := len(data)
 	m := json.RawMessage(data[:0])
 	if err := r.decoder.Decode(&m); err != nil {
 		return 0, err
@@ -156,12 +155,19 @@ func (r *jsonFrameReader) Read(data []byte) (int, error) {
 	// If capacity of data is less than length of the message, decoder will allocate a new slice
 	// and set m to it, which means we need to copy the partial result back into data and preserve
 	// the remaining result for subsequent reads.
-	if len(m) > n {
-		//nolint:staticcheck // SA4006,SA4010 underlying array of data is modified here.
-		data = append(data[0:0], m[:n]...)
-		r.remaining = m[n:]
-		return n, io.ErrShortBuffer
+	if len(m) > cap(data) {
+		copy(data, m)
+		r.remaining = m[len(data):]
+		return len(data), io.ErrShortBuffer
 	}
+
+	if len(m) > len(data) {
+		// The bytes beyond len(data) were stored in data's underlying array, which we do
+		// not own after this function returns.
+		r.remaining = append([]byte(nil), m[len(data):]...)
+		return len(data), io.ErrShortBuffer
+	}
+
 	return len(m), nil
 }
 
diff --git a/staging/src/k8s.io/apimachinery/pkg/util/framer/framer_test.go b/staging/src/k8s.io/apimachinery/pkg/util/framer/framer_test.go
index 5a1fca2cc88..7275796ab44 100644
--- a/staging/src/k8s.io/apimachinery/pkg/util/framer/framer_test.go
+++ b/staging/src/k8s.io/apimachinery/pkg/util/framer/framer_test.go
@@ -18,6 +18,7 @@ package framer
 
 import (
 	"bytes"
+	"errors"
 	"io"
 	"testing"
 )
@@ -173,3 +174,17 @@ func TestJSONFrameReaderShortBuffer(t *testing.T) {
 		t.Fatalf("unexpected: %v %d %q", err, n, buf)
 	}
 }
+
+func TestJSONFrameReaderShortBufferNoUnderlyingArrayReuse(t *testing.T) {
+	b := bytes.NewBufferString("{}")
+	r := NewJSONFramedReader(io.NopCloser(b))
+	buf := make([]byte, 1, 2) // cap(buf) > len(buf) && cap(buf) <= len("{}")
+
+	if n, err := r.Read(buf); !errors.Is(err, io.ErrShortBuffer) || n != 1 || string(buf[:n]) != "{" {
+		t.Fatalf("unexpected: %v %d %q", err, n, buf)
+	}
+	buf = append(buf, make([]byte, 1)...) // stomps the second byte of the backing array
+	if n, err := r.Read(buf[1:]); err != nil || n != 1 || string(buf[1:1+n]) != "}" {
+		t.Fatalf("unexpected: %v %d %q", err, n, buf)
+	}
+}