interp: test and fix read with regular files as stdin

This worked on v3.8.0, but was broken by v3.9.0 starting to use the
cancelreader library to support cancellable reads from standard input.
The use of cancelreader works OK for stdin files being OS pipes,
but it does not work for stdin files which are regular files on Linux,
as regular files on Linux are always ready for reading and do not
support polling or cancelling in any way.

As such, if cancelreader fails to create a cancellable reader,
fall back to reading directly from the file without cancellation.
This approach is not ideal, so leave a TODO to improve it with
some form of new API proposed upstream.

Thanks to Andrew Imeson for reporting the bug, providing multiple
test cases which reproduced it, and doing a git bisect as well.

Fixes #1099.

Author: mvdan

interp/builtin.go

@@ -10,6 +10,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -942,34 +943,43 @@ func (r *Runner) readLine(ctx context.Context, raw bool) ([]byte, error) {
 	var line []byte
 	esc := false
 
-	cr, err := cancelreader.NewReader(r.stdin)
-	if err != nil {
-		return nil, err
+	stdin := io.Reader(r.stdin)
+	// [cancelreader.NewReader] may fail under some circumstances, such as r.stdin being
+	// a regular file on Linux, in which case epoll returns an "operation not permitted" error
+	// given that regular files can always be read immediately. Polling them makes no sense.
+	// As such, if cancelreader fails, fall back to no cancellation, meaning this is best-effort.
+	//
+	// TODO: it would be nice if the cancelreader library classified errors so that we could
+	// safely handle "this file does not need polling" by skipping the polling as we do below
+	// but still fail on other errors, which may be unexpected or hide bugs.
+	// See the upstream issue: https://github.com/muesli/cancelreader/issues/23
+	if cr, err := cancelreader.NewReader(r.stdin); err == nil {
+		done := make(chan struct{})
+		var wg sync.WaitGroup
+		wg.Add(1)
+		go func() {
+			select {
+			case <-ctx.Done():
+				cr.Cancel()
+			case <-done:
+			}
+			wg.Done()
+		}()
+		defer func() {
+			close(done)
+			wg.Wait()
+			// Could put the Close in the above goroutine, but if "read" is
+			// immediately called again, the Close might overlap with creating a
+			// new cancelreader. Want this cancelreader to be completely closed
+			// by the time readLine returns.
+			cr.Close()
+		}()
+		stdin = cr
 	}
-	done := make(chan struct{})
-	var wg sync.WaitGroup
-	wg.Add(1)
-	go func() {
-		select {
-		case <-ctx.Done():
-			cr.Cancel()
-		case <-done:
-		}
-		wg.Done()
-	}()
-	defer func() {
-		close(done)
-		wg.Wait()
-		// Could put the Close in the above goroutine, but if "read" is
-		// immediately called again, the Close might overlap with creating a
-		// new cancelreader. Want this cancelreader to be completely closed
-		// by the time readLine returns.
-		cr.Close()
-	}()
 
 	for {
 		var buf [1]byte
-		n, err := cr.Read(buf[:])
+		n, err := stdin.Read(buf[:])
 		if n > 0 {
 			b := buf[0]
 			switch {

interp/interp_test.go

@@ -2905,6 +2905,10 @@ done <<< 2`,
 		"while read a; do echo $a; GOSH_CMD=exec_ok $GOSH_PROG; done <<EOF\na\nb\nc\nEOF",
 		"a\nexec ok\nb\nexec ok\nc\nexec ok\n",
 	},
+	{
+		"echo file1 >f; echo file2 >>f; while read a; do echo $a; done <f",
+		"file1\nfile2\n",
+	},
 	// TODO: our final exit status here isn't right.
 	// {
 	// 	"while read a; do echo $a; GOSH_CMD=exec_fail $GOSH_PROG; done <<< 'a\nb\nc'",