From 4677b72a4c38944522b90e9db12eb410e1ec54d9 Mon Sep 17 00:00:00 2001
From: Chris Waldon <christopher.waldon.dev@gmail.com>
Date: Tue, 23 May 2023 09:51:53 -0400
Subject: [PATCH] text: fix over-reading on truncated EOF

When consuming text from an io.Reader, the shaper could hit an EOF when reading the
text, then still try to check whether it was done by calling ReadByte() followed by
UnreadByte(). The ReadByte() would still return EOF, but the UnreadByte() would then
walk the iterator cursor backwards to the final byte of the text. If and only if the
text was being truncated, this unexpected cursor position could cause the shaper to
conclude that there were additional runes that were truncated, and thus the returned
glyph stream would account for too many runes. This commit provides a test and a fix.

Many thanks to Jack Mordaunt for the excellent bug report leading to this fix.

Signed-off-by: Chris Waldon <christopher.waldon.dev@gmail.com>
---
 text/shaper.go      |  8 ++++--
 text/shaper_test.go | 69 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/text/shaper.go b/text/shaper.go
index 6f02701a..114b174e 100644
--- a/text/shaper.go
+++ b/text/shaper.go
@@ -247,9 +247,11 @@ func (l *Shaper) layoutText(params Parameters, txt io.Reader, str string) {
 					break
 				}
 			}
-			_, re := l.reader.ReadByte()
-			done = re != nil
-			_ = l.reader.UnreadByte()
+			if !done {
+				_, re := l.reader.ReadByte()
+				done = re != nil
+				_ = l.reader.UnreadByte()
+			}
 		} else {
 			idx := strings.IndexByte(str, '\n')
 			if idx == -1 {
diff --git a/text/shaper_test.go b/text/shaper_test.go
index b2926070..05cb694f 100644
--- a/text/shaper_test.go
+++ b/text/shaper_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	nsareg "eliasnaur.com/font/noto/sans/arabic/regular"
+	"gioui.org/font/gofont"
 	"gioui.org/font/opentype"
 	"gioui.org/io/system"
 	"golang.org/x/exp/slices"
@@ -431,3 +432,71 @@ func printLinePositioning(t *testing.T, lines []line, glyphs []Glyph) {
 		}
 	}
 }
+
+// TestShapeStringRuneAccounting tries shaping the same string/parameter combinations with both
+// shaping methods and ensures that the resulting glyph stream always has the right number of
+// runes accounted for.
+func TestShapeStringRuneAccounting(t *testing.T) {
+	type testcase struct {
+		name   string
+		input  string
+		params Parameters
+	}
+	type setup struct {
+		kind string
+		do   func(*Shaper, Parameters, string)
+	}
+	for _, tc := range []testcase{
+		{
+			name:  "simple truncated",
+			input: "abc",
+			params: Parameters{
+				PxPerEm:  fixed.Int26_6(16),
+				MaxWidth: 100,
+				MaxLines: 1,
+			},
+		},
+		{
+			name:  "simple",
+			input: "abc",
+			params: Parameters{
+				PxPerEm:  fixed.Int26_6(16),
+				MaxWidth: 100,
+			},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			for _, setup := range []setup{
+				{
+					kind: "LayoutString",
+					do: func(shaper *Shaper, params Parameters, input string) {
+						shaper.LayoutString(params, input)
+					},
+				},
+				{
+					kind: "Layout",
+					do: func(shaper *Shaper, params Parameters, input string) {
+						shaper.Layout(params, strings.NewReader(input))
+					},
+				},
+			} {
+				t.Run(setup.kind, func(t *testing.T) {
+					shaper := NewShaper(gofont.Collection())
+					setup.do(shaper, tc.params, tc.input)
+
+					glyphs := []Glyph{}
+					for g, ok := shaper.NextGlyph(); ok; g, ok = shaper.NextGlyph() {
+						glyphs = append(glyphs, g)
+					}
+					totalRunes := 0
+					for _, g := range glyphs {
+						totalRunes += g.Runes
+					}
+					if inputRunes := len([]rune(tc.input)); totalRunes != inputRunes {
+						t.Errorf("input contained %d runes, but glyphs contained %d", inputRunes, totalRunes)
+					}
+				})
+			}
+		})
+	}
+}