From 82529becaee6f5050cb3ebb4aaa7a798357c1cf1 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Wed, 3 Apr 2019 12:59:27 -0700 Subject: 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 --- pkg/abi/linux/tty.go | 20 +++++++------ pkg/sentry/fs/tty/line_discipline.go | 55 ++++++++++++++++-------------------- 2 files changed, 36 insertions(+), 39 deletions(-) (limited to 'pkg') 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 } -- cgit v1.2.3