fix(screen): make escape buffer reuse safe under re-entrant Write

Detach combineBuf while the outer Write parses it so callbacks calling Write
cannot overwrite it, drop oversized escapeBuf on the combine path, and document
the Process/AddAssetOwned ownership contracts. Add re-entrancy and caller-buffer
aliasing tests.

commit: b0f008cb0c8cf5636a512b6867d66611e50aabe1
author: brice · 20d ago · 2026-10-02 09:46 UTC
parent: 178c6c5
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

5 files · +106 -7

M graphics/graphics.go +2 -1


@@ -111,7 +111,8 @@ const (
111	111		// scene or any snapshot. Format preserves the encoded pixel layout for the
112	112		// renderer. Width and Height describe the decoded pixel extent used for
113	113		// clipping and quota accounting. DecodedPixels, when non-zero, must equal
114		-	// Width*Height; accounting always uses the checked product.
	114	+	// Width*Height; accounting always uses the checked product. Scene.AddAssetOwned
	115	+	// and ReplaceAssetOwned instead take ownership of Encoded without copying it.
115	116		type AssetBlob struct {
116	117		Encoded []byte
117	118		Format AssetFormat

 // scene or any snapshot. Format preserves the encoded pixel layout for the
 // renderer. Width and Height describe the decoded pixel extent used for
 // clipping and quota accounting. DecodedPixels, when non-zero, must equal
-// Width*Height; accounting always uses the checked product.
+// Width*Height; accounting always uses the checked product. Scene.AddAssetOwned
+// and ReplaceAssetOwned instead take ownership of Encoded without copying it.
 type AssetBlob struct {
 	Encoded       []byte
 	Format        AssetFormat

M graphics/scene.go +3 -2


@@ -106,7 +106,8 @@ func (s *Scene) AddAsset(blob AssetBlob) (AssetID, error) {
106	106
107	107		// AddAssetOwned is AddAsset for callers that hand over blob.Encoded: the scene
108	108		// keeps the slice without copying, so the caller must not read or modify it
109		-	// afterwards. Snapshots and AssetView still return copies.
	109	+	// afterwards. Snapshots and AssetView still return copies. On error the scene
	110	+	// does not retain blob.Encoded, so the caller keeps ownership of it.
110	111		func (s *Scene) AddAssetOwned(blob AssetBlob) (AssetID, error) {
111	112		return s.addAsset(blob, false)
112	113		}

@@ -149,7 +150,7 @@ func (s *Scene) ReplaceAsset(id AssetID, blob AssetBlob) (AssetID, error) {
149	150		}
150	151
151	152		// ReplaceAssetOwned is ReplaceAsset for callers that hand over blob.Encoded;
152		-	// see AddAssetOwned.
	153	+	// see AddAssetOwned. On error blob.Encoded is not retained.
153	154		func (s *Scene) ReplaceAssetOwned(id AssetID, blob AssetBlob) (AssetID, error) {
154	155		return s.replaceAsset(id, blob, false)
155	156		}

 // AddAssetOwned is AddAsset for callers that hand over blob.Encoded: the scene
 // keeps the slice without copying, so the caller must not read or modify it
-// afterwards. Snapshots and AssetView still return copies.
+// afterwards. Snapshots and AssetView still return copies. On error the scene
+// does not retain blob.Encoded, so the caller keeps ownership of it.
 func (s *Scene) AddAssetOwned(blob AssetBlob) (AssetID, error) {
 	return s.addAsset(blob, false)
 }
 }
 // ReplaceAssetOwned is ReplaceAsset for callers that hand over blob.Encoded;
-// see AddAssetOwned.
+// see AddAssetOwned. On error blob.Encoded is not retained.
 func (s *Scene) ReplaceAssetOwned(id AssetID, blob AssetBlob) (AssetID, error) {
 	return s.replaceAsset(id, blob, false)
 }

M protocol/kittygraphics/session.go +6 -0


@@ -162,6 +162,12 @@ func (s *Session) Finish() (Result, error) {
162	162
163	163		// Process applies one already parsed command. It is useful for callers that
164	164		// have their own stream framing but want this package's strict adapter.
	165	+	//
	166	+	// Process never retains command.Payload (or the APC bytes it was parsed from)
	167	+	// beyond the call: whatever must outlive it, such as a pending chunked upload
	168	+	// or a committed asset, is copied or freshly decoded. Callers may therefore pass
	169	+	// a command from ParseAPCBorrowed and reuse or overwrite the underlying bytes
	170	+	// as soon as Process returns.
165	171		func (s *Session) Process(command Command) (Result, error) {
166	172		if s == nil {
167	173		return Result{}, ErrNoScene


@@ -162,6 +162,12 @@ func (s *Session) Finish() (Result, error) {
162		162
163	// Process applies one already parsed command. It is useful for callers that	163		// Process applies one already parsed command. It is useful for callers that
164	// have their own stream framing but want this package's strict adapter.	164		// have their own stream framing but want this package's strict adapter.
		165	+	//
		166	+	// Process never retains command.Payload (or the APC bytes it was parsed from)
		167	+	// beyond the call: whatever must outlive it, such as a pending chunked upload
		168	+	// or a committed asset, is copied or freshly decoded. Callers may therefore pass
		169	+	// a command from ParseAPCBorrowed and reuse or overwrite the underlying bytes
		170	+	// as soon as Process returns.
165	func (s *Session) Process(command Command) (Result, error) {	171		func (s *Session) Process(command Command) (Result, error) {
166	if s == nil {	172		if s == nil {
167	return Result{}, ErrNoScene	173		return Result{}, ErrNoScene

M screen.go +14 -4


@@ -188,18 +188,28 @@ func (s *Screen) Write(data []byte) {
188	188		if len(s.escapeBuf) > 0 {
189	189		// Reuse a bounded scratch buffer: data is consumed before Write returns
190	190		// and nothing retains slices of it (the partial-escape path copies).
	191	+	// Detach it while in use: callbacks invoked below may call Write
	192	+	// re-entrantly, and that inner Write must not overwrite bytes this
	193	+	// Write is still reading.
191	194		need := len(s.escapeBuf) + len(data)
192	195		combined := s.combineBuf[:0]
	196	+	s.combineBuf = nil
193	197		if cap(combined) < need {
194	198		combined = make([]byte, 0, need)
195	199		}
196	200		combined = append(combined, s.escapeBuf...)
197	201		combined = append(combined, data...)
198		-	if cap(combined) <= maxRetainedEscapeBufferCap {
199		-	s.combineBuf = combined
200		-	}
	202	+	defer func() {
	203	+	if s.combineBuf == nil && cap(combined) <= maxRetainedEscapeBufferCap {
	204	+	s.combineBuf = combined[:0]
	205	+	}
	206	+	}()
201	207		data = combined
202		-	s.escapeBuf = s.escapeBuf[:0]
	208	+	if cap(s.escapeBuf) > maxRetainedEscapeBufferCap {
	209	+	s.escapeBuf = nil
	210	+	} else {
	211	+	s.escapeBuf = s.escapeBuf[:0]
	212	+	}
203	213		}
204	214		if s.kittyDiscard {
205	215		consumed := s.consumeKittyDiscard(data)

 	if len(s.escapeBuf) > 0 {
 		// Reuse a bounded scratch buffer: data is consumed before Write returns
 		// and nothing retains slices of it (the partial-escape path copies).
+		// Detach it while in use: callbacks invoked below may call Write
+		// re-entrantly, and that inner Write must not overwrite bytes this
+		// Write is still reading.
 		need := len(s.escapeBuf) + len(data)
 		combined := s.combineBuf[:0]
+		s.combineBuf = nil
 		if cap(combined) < need {
 			combined = make([]byte, 0, need)
 		}
 		combined = append(combined, s.escapeBuf...)
 		combined = append(combined, data...)
-		if cap(combined) <= maxRetainedEscapeBufferCap {
+		defer func() {
-			s.combineBuf = combined
+			if s.combineBuf == nil && cap(combined) <= maxRetainedEscapeBufferCap {
-		}
+				s.combineBuf = combined[:0]
+			}
+		}()
 		data = combined
-		s.escapeBuf = s.escapeBuf[:0]
+		if cap(s.escapeBuf) > maxRetainedEscapeBufferCap {
+			s.escapeBuf = nil
+		} else {
+			s.escapeBuf = s.escapeBuf[:0]
+		}
 	}
 	if s.kittyDiscard {
 		consumed := s.consumeKittyDiscard(data)

M screen_graphics_test.go +81 -0


@@ -4,6 +4,7 @@ import (
4	4		"encoding/base64"
5	5		"math"
6	6		"strconv"
	7	+	"strings"
7	8		"testing"
8	9
9	10		"github.com/bnema/vev-vt/graphics"

@@ -356,3 +357,83 @@ func TestScreenKittyFragmentedAPCsDoNotAliasEscapeBuffer(t *testing.T) {
356	357		}
357	358		require.ElementsMatch(t, [][]byte{{1, 2, 3, 4}, {9, 9, 9, 9}}, got)
358	359		}
	360	+
	361	+	func screenAssetBytes(t testing.T, screen Screen) [][]byte {
	362	+	t.Helper()
	363	+	snapshot := screen.GraphicsSnapshot()
	364	+	require.NotNil(t, snapshot)
	365	+	var got [][]byte
	366	+	for _, asset := range snapshot.Assets() {
	367	+	got = append(got, asset.Bytes())
	368	+	}
	369	+	return got
	370	+	}
	371	+
	372	+	// A callback may call Write re-entrantly; the inner Write must not clobber the
	373	+	// reusable combine buffer the outer Write is still parsing.
	374	+	func TestScreenWriteReentrantFromCallbackKeepsOuterStreamIntact(t *testing.T) {
	375	+	t.Run("split CSI then DA reply writes text", func(t *testing.T) {
	376	+	screen := NewScreen(40, 3)
	377	+	fired := 0
	378	+	screen.OnResponse = func([]byte) {
	379	+	fired++
	380	+	// Leave a partial escape, then complete it: the second inner
	381	+	// Write takes the combine path while the outer Write is mid-parse.
	382	+	screen.Write([]byte("\x1b["))
	383	+	screen.Write([]byte("1mZZZZZZZZZZ"))
	384	+	}
	385	+	screen.Write([]byte("\x1b[1"))
	386	+	screen.Write([]byte("m\x1b[cBCDEFGH"))
	387	+	require.Equal(t, 1, fired)
	388	+	require.Equal(t, "ZZZZZZZZZZBCDEFGH", strings.TrimRight(rowText(screen.Snapshot().Row(0)), " "))
	389	+	})
	390	+	t.Run("split APC response writes upload", func(t *testing.T) {
	391	+	screen := NewScreen(40, 3)
	392	+	inner := false
	393	+	screen.OnResponse = func([]byte) {
	394	+	if inner {
	395	+	return
	396	+	}
	397	+	inner = true
	398	+	for _, b := range []byte("\x1b_Ga=t,f=32,s=1,v=1;CQkJCQ==\x1b\\") {
	399	+	screen.Write([]byte{b})
	400	+	}
	401	+	}
	402	+	screen.Write([]byte("\x1b_Ga=t,i=1,f=32,s=1,v=1;AQID"))
	403	+	screen.Write([]byte("BA==\x1b\\XYZ"))
	404	+	require.Equal(t, "XYZ", strings.TrimRight(rowText(screen.Snapshot().Row(0)), " "))
	405	+	require.ElementsMatch(t, [][]byte{{1, 2, 3, 4}, {9, 9, 9, 9}}, screenAssetBytes(t, screen))
	406	+	})
	407	+	}
	408	+
	409	+	// The Screen borrows the caller's Write buffer while parsing; nothing it
	410	+	// retains may alias that buffer once Write returns.
	411	+	func TestScreenKittyGraphicsDoesNotAliasCallerWriteBuffer(t *testing.T) {
	412	+	scribble := func(b []byte) {
	413	+	for i := range b {
	414	+	b[i] = 0xff
	415	+	}
	416	+	}
	417	+	t.Run("single-shot transmit and display", func(t *testing.T) {
	418	+	screen := NewScreen(16, 3)
	419	+	buf := []byte("\x1b_Ga=T,i=1,f=32,s=1,v=1;AQIDBA==\x1b\\")
	420	+	screen.Write(buf)
	421	+	scribble(buf)
	422	+	require.Equal(t, [][]byte{{1, 2, 3, 4}}, screenAssetBytes(t, screen))
	423	+	})
	424	+	t.Run("first and final chunk in one write", func(t *testing.T) {
	425	+	screen := NewScreen(16, 3)
	426	+	buf := []byte("\x1b_Ga=t,i=1,f=32,s=1,v=1,m=1;AQID\x1b\\\x1b_Gm=0;BA==\x1b\\")
	427	+	screen.Write(buf)
	428	+	scribble(buf)
	429	+	require.Equal(t, [][]byte{{1, 2, 3, 4}}, screenAssetBytes(t, screen))
	430	+	})
	431	+	t.Run("first chunk write then buffer reuse before final chunk", func(t *testing.T) {
	432	+	screen := NewScreen(16, 3)
	433	+	buf := []byte("\x1b_Ga=t,i=1,f=32,s=1,v=1,m=1;AQID\x1b\\")
	434	+	screen.Write(buf)
	435	+	scribble(buf)
	436	+	screen.Write([]byte("\x1b_Gm=0;BA==\x1b\\"))
	437	+	require.Equal(t, [][]byte{{1, 2, 3, 4}}, screenAssetBytes(t, screen))
	438	+	})
	439	+	}


@@ -4,6 +4,7 @@ import (
4	"encoding/base64"	4		"encoding/base64"
5	"math"	5		"math"
6	"strconv"	6		"strconv"
		7	+	"strings"
7	"testing"	8		"testing"
8		9
9	"github.com/bnema/vev-vt/graphics"	10		"github.com/bnema/vev-vt/graphics"

@@ -356,3 +357,83 @@ func TestScreenKittyFragmentedAPCsDoNotAliasEscapeBuffer(t *testing.T) {
356	}	357		}
357	require.ElementsMatch(t, [][]byte{{1, 2, 3, 4}, {9, 9, 9, 9}}, got)	358		require.ElementsMatch(t, [][]byte{{1, 2, 3, 4}, {9, 9, 9, 9}}, got)
358	}	359		}
		360	+
		361	+	func screenAssetBytes(t testing.T, screen Screen) [][]byte {
		362	+	t.Helper()
		363	+	snapshot := screen.GraphicsSnapshot()
		364	+	require.NotNil(t, snapshot)
		365	+	var got [][]byte
		366	+	for _, asset := range snapshot.Assets() {
		367	+	got = append(got, asset.Bytes())
		368	+	}
		369	+	return got
		370	+	}
		371	+
		372	+	// A callback may call Write re-entrantly; the inner Write must not clobber the
		373	+	// reusable combine buffer the outer Write is still parsing.
		374	+	func TestScreenWriteReentrantFromCallbackKeepsOuterStreamIntact(t *testing.T) {
		375	+	t.Run("split CSI then DA reply writes text", func(t *testing.T) {
		376	+	screen := NewScreen(40, 3)
		377	+	fired := 0
		378	+	screen.OnResponse = func([]byte) {
		379	+	fired++
		380	+	// Leave a partial escape, then complete it: the second inner
		381	+	// Write takes the combine path while the outer Write is mid-parse.
		382	+	screen.Write([]byte("\x1b["))
		383	+	screen.Write([]byte("1mZZZZZZZZZZ"))
		384	+	}
		385	+	screen.Write([]byte("\x1b[1"))
		386	+	screen.Write([]byte("m\x1b[cBCDEFGH"))
		387	+	require.Equal(t, 1, fired)
		388	+	require.Equal(t, "ZZZZZZZZZZBCDEFGH", strings.TrimRight(rowText(screen.Snapshot().Row(0)), " "))
		389	+	})
		390	+	t.Run("split APC response writes upload", func(t *testing.T) {
		391	+	screen := NewScreen(40, 3)
		392	+	inner := false
		393	+	screen.OnResponse = func([]byte) {
		394	+	if inner {
		395	+	return
		396	+	}
		397	+	inner = true
		398	+	for _, b := range []byte("\x1b_Ga=t,f=32,s=1,v=1;CQkJCQ==\x1b\\") {
		399	+	screen.Write([]byte{b})
		400	+	}
		401	+	}
		402	+	screen.Write([]byte("\x1b_Ga=t,i=1,f=32,s=1,v=1;AQID"))
		403	+	screen.Write([]byte("BA==\x1b\\XYZ"))
		404	+	require.Equal(t, "XYZ", strings.TrimRight(rowText(screen.Snapshot().Row(0)), " "))
		405	+	require.ElementsMatch(t, [][]byte{{1, 2, 3, 4}, {9, 9, 9, 9}}, screenAssetBytes(t, screen))
		406	+	})
		407	+	}
		408	+
		409	+	// The Screen borrows the caller's Write buffer while parsing; nothing it
		410	+	// retains may alias that buffer once Write returns.
		411	+	func TestScreenKittyGraphicsDoesNotAliasCallerWriteBuffer(t *testing.T) {
		412	+	scribble := func(b []byte) {
		413	+	for i := range b {
		414	+	b[i] = 0xff
		415	+	}
		416	+	}
		417	+	t.Run("single-shot transmit and display", func(t *testing.T) {
		418	+	screen := NewScreen(16, 3)
		419	+	buf := []byte("\x1b_Ga=T,i=1,f=32,s=1,v=1;AQIDBA==\x1b\\")
		420	+	screen.Write(buf)
		421	+	scribble(buf)
		422	+	require.Equal(t, [][]byte{{1, 2, 3, 4}}, screenAssetBytes(t, screen))
		423	+	})
		424	+	t.Run("first and final chunk in one write", func(t *testing.T) {
		425	+	screen := NewScreen(16, 3)
		426	+	buf := []byte("\x1b_Ga=t,i=1,f=32,s=1,v=1,m=1;AQID\x1b\\\x1b_Gm=0;BA==\x1b\\")
		427	+	screen.Write(buf)
		428	+	scribble(buf)
		429	+	require.Equal(t, [][]byte{{1, 2, 3, 4}}, screenAssetBytes(t, screen))
		430	+	})
		431	+	t.Run("first chunk write then buffer reuse before final chunk", func(t *testing.T) {
		432	+	screen := NewScreen(16, 3)
		433	+	buf := []byte("\x1b_Ga=t,i=1,f=32,s=1,v=1,m=1;AQID\x1b\\")
		434	+	screen.Write(buf)
		435	+	scribble(buf)
		436	+	screen.Write([]byte("\x1b_Gm=0;BA==\x1b\\"))
		437	+	require.Equal(t, [][]byte{{1, 2, 3, 4}}, screenAssetBytes(t, screen))
		438	+	})
		439	+	}