summaryrefslogtreecommitdiffhomepage
path: root/pkg/sentry/fsimpl/devpts
diff options
context:
space:
mode:
authorEtienne Perot <eperot@google.com>2021-07-08 12:01:47 -0700
committergVisor bot <gvisor-bot@google.com>2021-07-08 12:04:56 -0700
commit07f2c8b56b5948759b3df6587a8fcea13fbcc82b (patch)
tree7dd45402c07749ed81dae5172913a1a911002256 /pkg/sentry/fsimpl/devpts
parent1fc7a9eac2f27053ed4ca138c6568b14ef520648 (diff)
devpts: Notify of echo'd input queue bytes only after locks have been released.
PiperOrigin-RevId: 383684320
Diffstat (limited to 'pkg/sentry/fsimpl/devpts')
-rw-r--r--pkg/sentry/fsimpl/devpts/BUILD1
-rw-r--r--pkg/sentry/fsimpl/devpts/devpts_test.go34
-rw-r--r--pkg/sentry/fsimpl/devpts/line_discipline.go57
-rw-r--r--pkg/sentry/fsimpl/devpts/queue.go42
4 files changed, 99 insertions, 35 deletions
diff --git a/pkg/sentry/fsimpl/devpts/BUILD b/pkg/sentry/fsimpl/devpts/BUILD
index 50b4c02ef..f981ff296 100644
--- a/pkg/sentry/fsimpl/devpts/BUILD
+++ b/pkg/sentry/fsimpl/devpts/BUILD
@@ -60,5 +60,6 @@ go_test(
"//pkg/abi/linux",
"//pkg/sentry/contexttest",
"//pkg/usermem",
+ "//pkg/waiter",
],
)
diff --git a/pkg/sentry/fsimpl/devpts/devpts_test.go b/pkg/sentry/fsimpl/devpts/devpts_test.go
index 448390cfe..1ef07d702 100644
--- a/pkg/sentry/fsimpl/devpts/devpts_test.go
+++ b/pkg/sentry/fsimpl/devpts/devpts_test.go
@@ -20,6 +20,7 @@ import (
"gvisor.dev/gvisor/pkg/abi/linux"
"gvisor.dev/gvisor/pkg/sentry/contexttest"
"gvisor.dev/gvisor/pkg/usermem"
+ "gvisor.dev/gvisor/pkg/waiter"
)
func TestSimpleMasterToReplica(t *testing.T) {
@@ -54,3 +55,36 @@ func TestSimpleMasterToReplica(t *testing.T) {
t.Fatalf("written and read strings do not match: got %q, want %q", outStr, inStr)
}
}
+
+type callback func(*waiter.Entry, waiter.EventMask)
+
+func (cb callback) Callback(entry *waiter.Entry, mask waiter.EventMask) {
+ cb(entry, mask)
+}
+
+func TestEchoDeadlock(t *testing.T) {
+ ctx := contexttest.Context(t)
+ termios := linux.DefaultReplicaTermios
+ termios.LocalFlags |= linux.ECHO
+ ld := newLineDiscipline(termios)
+ outBytes := make([]byte, 32)
+ dst := usermem.BytesIOSequence(outBytes)
+ entry := &waiter.Entry{Callback: callback(func(*waiter.Entry, waiter.EventMask) {
+ ld.inputQueueRead(ctx, dst)
+ })}
+ ld.masterWaiter.EventRegister(entry, waiter.ReadableEvents)
+ defer ld.masterWaiter.EventUnregister(entry)
+ inBytes := []byte("hello, tty\n")
+ n, err := ld.inputQueueWrite(ctx, usermem.BytesIOSequence(inBytes))
+ if err != nil {
+ t.Fatalf("inputQueueWrite: %v", err)
+ }
+ if int(n) != len(inBytes) {
+ t.Fatalf("read wrong length: got %d, want %d", n, len(inBytes))
+ }
+ outStr := string(outBytes[:n])
+ inStr := string(inBytes)
+ if outStr != inStr {
+ t.Fatalf("written and read strings do not match: got %q, want %q", outStr, inStr)
+ }
+}
diff --git a/pkg/sentry/fsimpl/devpts/line_discipline.go b/pkg/sentry/fsimpl/devpts/line_discipline.go
index e94a5bac3..9cb21e83b 100644
--- a/pkg/sentry/fsimpl/devpts/line_discipline.go
+++ b/pkg/sentry/fsimpl/devpts/line_discipline.go
@@ -70,6 +70,10 @@ const (
// +------------------------| output queue |<--------------------------+
// (outputQueueRead) +--------------+ (outputQueueWrite)
//
+// There is special handling for the ECHO option, where bytes written to the
+// input queue are also output back to the terminal by being written to
+// l.outQueue by the input queue transformer.
+//
// Lock order:
// termiosMu
// inQueue.mu
@@ -126,7 +130,6 @@ func (l *lineDiscipline) getTermios(task *kernel.Task, args arch.SyscallArgument
// setTermios sets a linux.Termios for the tty.
func (l *lineDiscipline) setTermios(task *kernel.Task, args arch.SyscallArguments) (uintptr, error) {
l.termiosMu.Lock()
- defer l.termiosMu.Unlock()
oldCanonEnabled := l.termios.LEnabled(linux.ICANON)
// We must copy a Termios struct, not KernelTermios.
var t linux.Termios
@@ -141,7 +144,10 @@ func (l *lineDiscipline) setTermios(task *kernel.Task, args arch.SyscallArgument
l.inQueue.pushWaitBufLocked(l)
l.inQueue.readable = true
l.inQueue.mu.Unlock()
+ l.termiosMu.Unlock()
l.replicaWaiter.Notify(waiter.ReadableEvents)
+ } else {
+ l.termiosMu.Unlock()
}
return 0, err
@@ -179,28 +185,37 @@ func (l *lineDiscipline) inputQueueReadSize(t *kernel.Task, io usermem.IO, args
func (l *lineDiscipline) inputQueueRead(ctx context.Context, dst usermem.IOSequence) (int64, error) {
l.termiosMu.RLock()
- defer l.termiosMu.RUnlock()
- n, pushed, err := l.inQueue.read(ctx, dst, l)
+ n, pushed, notifyEcho, err := l.inQueue.read(ctx, dst, l)
+ l.termiosMu.RUnlock()
if err != nil {
return 0, err
}
if n > 0 {
- l.masterWaiter.Notify(waiter.WritableEvents)
+ if notifyEcho {
+ l.masterWaiter.Notify(waiter.ReadableEvents | waiter.WritableEvents)
+ } else {
+ l.masterWaiter.Notify(waiter.WritableEvents)
+ }
if pushed {
l.replicaWaiter.Notify(waiter.ReadableEvents)
}
return n, nil
+ } else if notifyEcho {
+ l.masterWaiter.Notify(waiter.ReadableEvents)
}
return 0, syserror.ErrWouldBlock
}
func (l *lineDiscipline) inputQueueWrite(ctx context.Context, src usermem.IOSequence) (int64, error) {
l.termiosMu.RLock()
- defer l.termiosMu.RUnlock()
- n, err := l.inQueue.write(ctx, src, l)
+ n, notifyEcho, err := l.inQueue.write(ctx, src, l)
+ l.termiosMu.RUnlock()
if err != nil {
return 0, err
}
+ if notifyEcho {
+ l.masterWaiter.Notify(waiter.ReadableEvents)
+ }
if n > 0 {
l.replicaWaiter.Notify(waiter.ReadableEvents)
return n, nil
@@ -214,8 +229,9 @@ func (l *lineDiscipline) outputQueueReadSize(t *kernel.Task, io usermem.IO, args
func (l *lineDiscipline) outputQueueRead(ctx context.Context, dst usermem.IOSequence) (int64, error) {
l.termiosMu.RLock()
- defer l.termiosMu.RUnlock()
- n, pushed, err := l.outQueue.read(ctx, dst, l)
+ // Ignore notifyEcho, as it cannot happen when reading from the output queue.
+ n, pushed, _, err := l.outQueue.read(ctx, dst, l)
+ l.termiosMu.RUnlock()
if err != nil {
return 0, err
}
@@ -231,8 +247,9 @@ func (l *lineDiscipline) outputQueueRead(ctx context.Context, dst usermem.IOSequ
func (l *lineDiscipline) outputQueueWrite(ctx context.Context, src usermem.IOSequence) (int64, error) {
l.termiosMu.RLock()
- defer l.termiosMu.RUnlock()
- n, err := l.outQueue.write(ctx, src, l)
+ // Ignore notifyEcho, as it cannot happen when writing to the output queue.
+ n, _, err := l.outQueue.write(ctx, src, l)
+ l.termiosMu.RUnlock()
if err != nil {
return 0, err
}
@@ -246,7 +263,8 @@ func (l *lineDiscipline) outputQueueWrite(ctx context.Context, src usermem.IOSeq
// transformer is a helper interface to make it easier to stateify queue.
type transformer interface {
// transform functions require queue's mutex to be held.
- transform(*lineDiscipline, *queue, []byte) int
+ // The boolean indicates whether there was any echoed bytes.
+ transform(*lineDiscipline, *queue, []byte) (int, bool)
}
// outputQueueTransformer implements transformer. It performs line discipline
@@ -261,7 +279,7 @@ type outputQueueTransformer struct{}
// Preconditions:
// * l.termiosMu must be held for reading.
// * q.mu must be held.
-func (*outputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte) int {
+func (*outputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte) (int, bool) {
// transformOutput is effectively always in noncanonical mode, as the
// master termios never has ICANON set.
@@ -270,7 +288,7 @@ func (*outputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte
if len(q.readBuf) > 0 {
q.readable = true
}
- return len(buf)
+ return len(buf), false
}
var ret int
@@ -321,7 +339,7 @@ func (*outputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte
if len(q.readBuf) > 0 {
q.readable = true
}
- return ret
+ return ret, false
}
// inputQueueTransformer implements transformer. It performs line discipline
@@ -334,15 +352,17 @@ type inputQueueTransformer struct{}
// transformed according to flags set in the termios struct. See
// drivers/tty/n_tty.c:n_tty_receive_char_special for an analogous kernel
// function.
+// It returns an extra boolean indicating whether any characters need to be
+// echoed, in which case we need to notify readers.
//
// Preconditions:
// * l.termiosMu must be held for reading.
// * q.mu must be held.
-func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte) int {
+func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte) (int, bool) {
// If there's a line waiting to be read in canonical mode, don't write
// anything else to the read buffer.
if l.termios.LEnabled(linux.ICANON) && q.readable {
- return 0
+ return 0, false
}
maxBytes := nonCanonMaxBytes
@@ -351,6 +371,7 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte)
}
var ret int
+ var notifyEcho bool
for len(buf) > 0 && len(q.readBuf) < canonMaxBytes {
size := l.peek(buf)
cBytes := append([]byte{}, buf[:size]...)
@@ -397,7 +418,7 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte)
// Anything written to the readBuf will have to be echoed.
if l.termios.LEnabled(linux.ECHO) {
l.outQueue.writeBytes(cBytes, l)
- l.masterWaiter.Notify(waiter.ReadableEvents)
+ notifyEcho = true
}
// If we finish a line, make it available for reading.
@@ -412,7 +433,7 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte)
q.readable = true
}
- return ret
+ return ret, notifyEcho
}
// shouldDiscard returns whether c should be discarded. In canonical mode, if
diff --git a/pkg/sentry/fsimpl/devpts/queue.go b/pkg/sentry/fsimpl/devpts/queue.go
index 47b0f1599..ff1d89955 100644
--- a/pkg/sentry/fsimpl/devpts/queue.go
+++ b/pkg/sentry/fsimpl/devpts/queue.go
@@ -98,17 +98,19 @@ func (q *queue) readableSize(t *kernel.Task, io usermem.IO, args arch.SyscallArg
}
-// read reads from q to userspace. It returns the number of bytes read as well
-// as whether the read caused more readable data to become available (whether
+// read reads from q to userspace. It returns:
+// - The number of bytes read
+// - Whether the read caused more readable data to become available (whether
// data was pushed from the wait buffer to the read buffer).
+// - Whether any data was echoed back (need to notify readers).
//
// Preconditions: l.termiosMu must be held for reading.
-func (q *queue) read(ctx context.Context, dst usermem.IOSequence, l *lineDiscipline) (int64, bool, error) {
+func (q *queue) read(ctx context.Context, dst usermem.IOSequence, l *lineDiscipline) (int64, bool, bool, error) {
q.mu.Lock()
defer q.mu.Unlock()
if !q.readable {
- return 0, false, syserror.ErrWouldBlock
+ return 0, false, false, syserror.ErrWouldBlock
}
if dst.NumBytes() > canonMaxBytes {
@@ -131,19 +133,20 @@ func (q *queue) read(ctx context.Context, dst usermem.IOSequence, l *lineDiscipl
return n, nil
}))
if err != nil {
- return 0, false, err
+ return 0, false, false, err
}
// Move data from the queue's wait buffer to its read buffer.
- nPushed := q.pushWaitBufLocked(l)
+ nPushed, notifyEcho := q.pushWaitBufLocked(l)
- return int64(n), nPushed > 0, nil
+ return int64(n), nPushed > 0, notifyEcho, nil
}
// write writes to q from userspace.
+// The returned boolean indicates whether any data was echoed back.
//
// Preconditions: l.termiosMu must be held for reading.
-func (q *queue) write(ctx context.Context, src usermem.IOSequence, l *lineDiscipline) (int64, error) {
+func (q *queue) write(ctx context.Context, src usermem.IOSequence, l *lineDiscipline) (int64, bool, error) {
q.mu.Lock()
defer q.mu.Unlock()
@@ -173,44 +176,49 @@ func (q *queue) write(ctx context.Context, src usermem.IOSequence, l *lineDiscip
return n, nil
}))
if err != nil {
- return 0, err
+ return 0, false, err
}
// Push data from the wait to the read buffer.
- q.pushWaitBufLocked(l)
+ _, notifyEcho := q.pushWaitBufLocked(l)
- return n, nil
+ return n, notifyEcho, nil
}
// writeBytes writes to q from b.
+// The returned boolean indicates whether any data was echoed back.
//
// Preconditions: l.termiosMu must be held for reading.
-func (q *queue) writeBytes(b []byte, l *lineDiscipline) {
+func (q *queue) writeBytes(b []byte, l *lineDiscipline) bool {
q.mu.Lock()
defer q.mu.Unlock()
// Write to the wait buffer.
q.waitBufAppend(b)
- q.pushWaitBufLocked(l)
+ _, notifyEcho := q.pushWaitBufLocked(l)
+ return notifyEcho
}
// pushWaitBufLocked fills the queue's read buffer with data from the wait
// buffer.
+// The returned boolean indicates whether any data was echoed back.
//
// Preconditions:
// * l.termiosMu must be held for reading.
// * q.mu must be locked.
-func (q *queue) pushWaitBufLocked(l *lineDiscipline) int {
+func (q *queue) pushWaitBufLocked(l *lineDiscipline) (int, bool) {
if q.waitBufLen == 0 {
- return 0
+ return 0, false
}
// Move data from the wait to the read buffer.
var total int
var i int
+ var notifyEcho bool
for i = 0; i < len(q.waitBuf); i++ {
- n := q.transform(l, q, q.waitBuf[i])
+ n, echo := q.transform(l, q, q.waitBuf[i])
total += n
+ notifyEcho = notifyEcho || echo
if n != len(q.waitBuf[i]) {
// The read buffer filled up without consuming the
// entire buffer.
@@ -223,7 +231,7 @@ func (q *queue) pushWaitBufLocked(l *lineDiscipline) int {
q.waitBuf = q.waitBuf[i:]
q.waitBufLen -= uint64(total)
- return total
+ return total, notifyEcho
}
// Precondition: q.mu must be locked.