Fix index out of bounds in tty implementation.

The previous implementation revolved around runes instead of bytes, which caused
weird behavior when converting between the two. For example, peekRune would read
the byte 0xff from a buffer, convert it to a rune, then return it. As rune is an
alias of int32, 0xff was 0-padded to int32(255), which is the hex code point for
?. However, peekRune also returned the length of the byte (1). When calling
utf8.EncodeRune, we only allocated 1 byte, but tried the write the 2-byte
character ?.

tl;dr: I apparently didn't understand runes when I wrote this.

PiperOrigin-RevId: 241789081
Change-Id: I14c788af4d9754973137801500ef6af7ab8a8727

3 files changed, 42 insertions, 39 deletions

diff --git a/pkg/abi/linux/tty.go b/pkg/abi/linux/tty.go
index e6f7c5b2a..bff882d89 100644
--- a/pkg/abi/linux/tty.go
+++ b/pkg/abi/linux/tty.go
@@ -14,10 +14,6 @@
 
 package linux
 
-import (
-	"unicode/utf8"
-)
-
 const (
 	// NumControlCharacters is the number of control characters in Termios.
 	NumControlCharacters = 19
@@ -104,11 +100,19 @@ func (t *KernelTermios) FromTermios(term Termios) {
 }
 
 // IsTerminating returns whether c is a line terminating character.
-func (t *KernelTermios) IsTerminating(c rune) bool {
+func (t *KernelTermios) IsTerminating(cBytes []byte) bool {
+	// All terminating characters are 1 byte.
+	if len(cBytes) != 1 {
+		return false
+	}
+	c := cBytes[0]
+
+	// Is this the user-set EOF character?
 	if t.IsEOF(c) {
 		return true
 	}
-	switch byte(c) {
+
+	switch c {
 	case disabledChar:
 		return false
 	case '\n', t.ControlCharacters[VEOL]:
@@ -120,8 +124,8 @@ func (t *KernelTermios) IsTerminating(c rune) bool {
 }
 
 // IsEOF returns whether c is the EOF character.
-func (t *KernelTermios) IsEOF(c rune) bool {
-	return utf8.RuneLen(c) == 1 && byte(c) == t.ControlCharacters[VEOF] && t.ControlCharacters[VEOF] != disabledChar
+func (t *KernelTermios) IsEOF(c byte) bool {
+	return c == t.ControlCharacters[VEOF] && t.ControlCharacters[VEOF] != disabledChar
 }
 
 // Input flags.
diff --git a/pkg/sentry/fs/tty/line_discipline.go b/pkg/sentry/fs/tty/line_discipline.go
index 31b6344f2..c4a364edb 100644
--- a/pkg/sentry/fs/tty/line_discipline.go
+++ b/pkg/sentry/fs/tty/line_discipline.go
@@ -280,10 +280,12 @@ func (*outputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte
 
 	var ret int
 	for len(buf) > 0 {
-		c, size := l.peekRune(buf)
+		size := l.peek(buf)
+		cBytes := append([]byte{}, buf[:size]...)
 		ret += size
 		buf = buf[size:]
-		switch c {
+		// We're guaranteed that cBytes has at least one element.
+		switch cBytes[0] {
 		case '\n':
 			if l.termios.OEnabled(linux.ONLRET) {
 				l.column = 0
@@ -297,7 +299,7 @@ func (*outputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte
 				continue
 			}
 			if l.termios.OEnabled(linux.OCRNL) {
-				c = '\n'
+				cBytes[0] = '\n'
 				if l.termios.OEnabled(linux.ONLRET) {
 					l.column = 0
 				}
@@ -319,10 +321,7 @@ func (*outputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte
 		default:
 			l.column++
 		}
-		// The compiler optimizes this by growing readBuf without
-		// creating the intermediate slice.
-		q.readBuf = append(q.readBuf, make([]byte, size)...)
-		utf8.EncodeRune(q.readBuf[len(q.readBuf)-size:], c)
+		q.readBuf = append(q.readBuf, cBytes...)
 	}
 	if len(q.readBuf) > 0 {
 		q.readable = true
@@ -358,8 +357,10 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte)
 
 	var ret int
 	for len(buf) > 0 && len(q.readBuf) < canonMaxBytes {
-		c, size := l.peekRune(buf)
-		switch c {
+		size := l.peek(buf)
+		cBytes := append([]byte{}, buf[:size]...)
+		// We're guaranteed that cBytes has at least one element.
+		switch cBytes[0] {
 		case '\r':
 			if l.termios.IEnabled(linux.IGNCR) {
 				buf = buf[size:]
@@ -367,17 +368,17 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte)
 				continue
 			}
 			if l.termios.IEnabled(linux.ICRNL) {
-				c = '\n'
+				cBytes[0] = '\n'
 			}
 		case '\n':
 			if l.termios.IEnabled(linux.INLCR) {
-				c = '\r'
+				cBytes[0] = '\r'
 			}
 		}
 
 		// In canonical mode, we discard non-terminating characters
 		// after the first 4095.
-		if l.shouldDiscard(q, c) {
+		if l.shouldDiscard(q, cBytes) {
 			buf = buf[size:]
 			ret += size
 			continue
@@ -387,20 +388,16 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte)
 		if len(q.readBuf)+size > maxBytes {
 			break
 		}
-		cBytes := buf[:size]
 		buf = buf[size:]
 		ret += size
 
 		// If we get EOF, make the buffer available for reading.
-		if l.termios.LEnabled(linux.ICANON) && l.termios.IsEOF(c) {
+		if l.termios.LEnabled(linux.ICANON) && l.termios.IsEOF(cBytes[0]) {
 			q.readable = true
 			break
 		}
 
-		// The compiler optimizes this by growing readBuf without
-		// creating the intermediate slice.
-		q.readBuf = append(q.readBuf, make([]byte, size)...)
-		utf8.EncodeRune(q.readBuf[len(q.readBuf)-size:], c)
+		q.readBuf = append(q.readBuf, cBytes...)
 
 		// Anything written to the readBuf will have to be echoed.
 		if l.termios.LEnabled(linux.ECHO) {
@@ -409,7 +406,7 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte)
 		}
 
 		// If we finish a line, make it available for reading.
-		if l.termios.LEnabled(linux.ICANON) && l.termios.IsTerminating(c) {
+		if l.termios.LEnabled(linux.ICANON) && l.termios.IsTerminating(cBytes) {
 			q.readable = true
 			break
 		}
@@ -430,21 +427,17 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte)
 // Precondition:
 // * l.termiosMu must be held for reading.
 // * q.mu must be held.
-func (l *lineDiscipline) shouldDiscard(q *queue, c rune) bool {
-	return l.termios.LEnabled(linux.ICANON) && len(q.readBuf)+utf8.RuneLen(c) >= canonMaxBytes && !l.termios.IsTerminating(c)
+func (l *lineDiscipline) shouldDiscard(q *queue, cBytes []byte) bool {
+	return l.termios.LEnabled(linux.ICANON) && len(q.readBuf)+len(cBytes) >= canonMaxBytes && !l.termios.IsTerminating(cBytes)
 }
 
-// peekRune returns the first rune from the byte array depending on whether
-// UTF8 is enabled.
-func (l *lineDiscipline) peekRune(b []byte) (rune, int) {
-	var c rune
-	var size int
+// peek returns the size in bytes of the next character to process. As long as
+// b isn't empty, peek returns a value of at least 1.
+func (l *lineDiscipline) peek(b []byte) int {
+	size := 1
 	// If UTF-8 support is enabled, runes might be multiple bytes.
 	if l.termios.IEnabled(linux.IUTF8) {
-		c, size = utf8.DecodeRune(b)
-	} else {
-		c = rune(b[0])
-		size = 1
+		_, size = utf8.DecodeRune(b)
 	}
-	return c, size
+	return size
 }
diff --git a/test/syscalls/linux/pty.cc b/test/syscalls/linux/pty.cc
index 253aa26ba..5b2dc9ccb 100644
--- a/test/syscalls/linux/pty.cc
+++ b/test/syscalls/linux/pty.cc
@@ -568,6 +568,12 @@ TEST_F(PtyTest, WriteSlaveToMaster) {
   EXPECT_EQ(memcmp(buf, kExpected, sizeof(kExpected)), 0);
 }
 
+TEST_F(PtyTest, WriteInvalidUTF8) {
+  char c = 0xff;
+  ASSERT_THAT(syscall(__NR_write, master_.get(), &c, sizeof(c)),
+              SyscallSucceedsWithValue(sizeof(c)));
+}
+
 // Both the master and slave report the standard default termios settings.
 //
 // Note that TCGETS on the master actually redirects to the slave (see comment