internal/poll, os: loop on EINTR

Historically we've assumed that we can install all signal handlers
with the SA_RESTART flag set, and let the system restart slow functions
if a signal is received. Therefore, we don't have to worry about EINTR.

This is only partially true, and we've added EINTR checks already for
connect, and open/read on Darwin, and sendfile on Solaris.

Other cases have turned up in #36644, #38033, and #38836.

Also, #20400 points out that when Go code is included in a C program,
the C program may install its own signal handlers without SA_RESTART.
In that case, Go code will see EINTR no matter what it does.

So, go ahead and check for EINTR. We don't check in the syscall package;
people using syscalls directly may want to check for EINTR themselves.
But we do check for EINTR in the higher level APIs in os and net,
and retry the system call if we see it.

This change looks safe, but of course we may be missing some cases
where we need to check for EINTR. As such cases turn up, we can add
tests to runtime/testdata/testprogcgo/eintr.go, and fix the code.
If there are any such cases, their handling after this change will be
no worse than it is today.

For #22838
Fixes #20400
Fixes #36644
Fixes #38033
Fixes #38836

Change-Id: I7e46ca8cafed0429c7a2386cc9edc9d9d47a6896
Reviewed-on: https://go-review.googlesource.com/c/go/+/232862
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Bryan C. Mills <[email protected]>

Author: ianlancetaylor

src/internal/poll/copy_file_range_linux.go

@@ -88,6 +88,12 @@ func copyFileRange(dst, src *FD, max int) (written int64, err error) {
 		return 0, err
 	}
 	defer src.readUnlock()
-	n, err := unix.CopyFileRange(src.Sysfd, nil, dst.Sysfd, nil, max, 0)
+	var n int
+	for {
+		n, err = unix.CopyFileRange(src.Sysfd, nil, dst.Sysfd, nil, max, 0)
+		if err != syscall.EINTR {
+			break
+		}
+	}
 	return int64(n), err
 }

src/internal/poll/fd_unix.go

@@ -8,7 +8,6 @@ package poll
 
 import (
 	"io"
-	"runtime"
 	"sync/atomic"
 	"syscall"
 )
@@ -153,20 +152,14 @@ func (fd *FD) Read(p []byte) (int, error) {
 		p = p[:maxRW]
 	}
 	for {
-		n, err := syscall.Read(fd.Sysfd, p)
+		n, err := ignoringEINTR(syscall.Read, fd.Sysfd, p)
 		if err != nil {
 			n = 0
 			if err == syscall.EAGAIN && fd.pd.pollable() {
 				if err = fd.pd.waitRead(fd.isFile); err == nil {
 					continue
 				}
 			}
-
-			// On MacOS we can see EINTR here if the user
-			// pressed ^Z.  See issue #22838.
-			if runtime.GOOS == "darwin" && err == syscall.EINTR {
-				continue
-			}
 		}
 		err = fd.eofError(n, err)
 		return n, err
@@ -184,7 +177,16 @@ func (fd *FD) Pread(p []byte, off int64) (int, error) {
 	if fd.IsStream && len(p) > maxRW {
 		p = p[:maxRW]
 	}
-	n, err := syscall.Pread(fd.Sysfd, p, off)
+	var (
+		n   int
+		err error
+	)
+	for {
+		n, err = syscall.Pread(fd.Sysfd, p, off)
+		if err != syscall.EINTR {
+			break
+		}
+	}
 	if err != nil {
 		n = 0
 	}
@@ -205,6 +207,9 @@ func (fd *FD) ReadFrom(p []byte) (int, syscall.Sockaddr, error) {
 	for {
 		n, sa, err := syscall.Recvfrom(fd.Sysfd, p, 0)
 		if err != nil {
+			if err == syscall.EINTR {
+				continue
+			}
 			n = 0
 			if err == syscall.EAGAIN && fd.pd.pollable() {
 				if err = fd.pd.waitRead(fd.isFile); err == nil {
@@ -229,6 +234,9 @@ func (fd *FD) ReadMsg(p []byte, oob []byte) (int, int, int, syscall.Sockaddr, er
 	for {
 		n, oobn, flags, sa, err := syscall.Recvmsg(fd.Sysfd, p, oob, 0)
 		if err != nil {
+			if err == syscall.EINTR {
+				continue
+			}
 			// TODO(dfc) should n and oobn be set to 0
 			if err == syscall.EAGAIN && fd.pd.pollable() {
 				if err = fd.pd.waitRead(fd.isFile); err == nil {
@@ -256,7 +264,7 @@ func (fd *FD) Write(p []byte) (int, error) {
 		if fd.IsStream && max-nn > maxRW {
 			max = nn + maxRW
 		}
-		n, err := syscall.Write(fd.Sysfd, p[nn:max])
+		n, err := ignoringEINTR(syscall.Write, fd.Sysfd, p[nn:max])
 		if n > 0 {
 			nn += n
 		}
@@ -293,6 +301,9 @@ func (fd *FD) Pwrite(p []byte, off int64) (int, error) {
 			max = nn + maxRW
 		}
 		n, err := syscall.Pwrite(fd.Sysfd, p[nn:max], off+int64(nn))
+		if err == syscall.EINTR {
+			continue
+		}
 		if n > 0 {
 			nn += n
 		}
@@ -319,6 +330,9 @@ func (fd *FD) WriteTo(p []byte, sa syscall.Sockaddr) (int, error) {
 	}
 	for {
 		err := syscall.Sendto(fd.Sysfd, p, 0, sa)
+		if err == syscall.EINTR {
+			continue
+		}
 		if err == syscall.EAGAIN && fd.pd.pollable() {
 			if err = fd.pd.waitWrite(fd.isFile); err == nil {
 				continue
@@ -342,6 +356,9 @@ func (fd *FD) WriteMsg(p []byte, oob []byte, sa syscall.Sockaddr) (int, int, err
 	}
 	for {
 		n, err := syscall.SendmsgN(fd.Sysfd, p, oob, sa, 0)
+		if err == syscall.EINTR {
+			continue
+		}
 		if err == syscall.EAGAIN && fd.pd.pollable() {
 			if err = fd.pd.waitWrite(fd.isFile); err == nil {
 				continue
@@ -370,6 +387,8 @@ func (fd *FD) Accept() (int, syscall.Sockaddr, string, error) {
 			return s, rsa, "", err
 		}
 		switch err {
+		case syscall.EINTR:
+			continue
 		case syscall.EAGAIN:
 			if fd.pd.pollable() {
 				if err = fd.pd.waitRead(fd.isFile); err == nil {
@@ -404,7 +423,7 @@ func (fd *FD) ReadDirent(buf []byte) (int, error) {
 	}
 	defer fd.decref()
 	for {
-		n, err := syscall.ReadDirent(fd.Sysfd, buf)
+		n, err := ignoringEINTR(syscall.ReadDirent, fd.Sysfd, buf)
 		if err != nil {
 			n = 0
 			if err == syscall.EAGAIN && fd.pd.pollable() {
@@ -495,7 +514,7 @@ func (fd *FD) WriteOnce(p []byte) (int, error) {
 		return 0, err
 	}
 	defer fd.writeUnlock()
-	return syscall.Write(fd.Sysfd, p)
+	return ignoringEINTR(syscall.Write, fd.Sysfd, p)
 }
 
 // RawRead invokes the user-defined function f for a read operation.
@@ -535,3 +554,19 @@ func (fd *FD) RawWrite(f func(uintptr) bool) error {
 		}
 	}
 }
+
+// ignoringEINTR makes a function call and repeats it if it returns
+// an EINTR error. This appears to be required even though we install
+// all signal handlers with SA_RESTART: see #22838, #38033, #38836.
+// Also #20400 and #36644 are issues in which a signal handler is
+// installed without setting SA_RESTART. None of these are the common case,
+// but there are enough of them that it seems that we can't avoid
+// an EINTR loop.
+func ignoringEINTR(fn func(fd int, p []byte) (int, error), fd int, p []byte) (int, error) {
+	for {
+		n, err := fn(fd, p)
+		if err != syscall.EINTR {
+			return n, err
+		}
+	}
+}

src/internal/poll/fd_writev_unix.go

@@ -12,9 +12,18 @@ import (
 )
 
 func writev(fd int, iovecs []syscall.Iovec) (uintptr, error) {
-	r, _, e := syscall.Syscall(syscall.SYS_WRITEV, uintptr(fd), uintptr(unsafe.Pointer(&iovecs[0])), uintptr(len(iovecs)))
+	var (
+		r uintptr
+		e syscall.Errno
+	)
+	for {
+		r, _, e = syscall.Syscall(syscall.SYS_WRITEV, uintptr(fd), uintptr(unsafe.Pointer(&iovecs[0])), uintptr(len(iovecs)))
+		if e != syscall.EINTR {
+			break
+		}
+	}
 	if e != 0 {
-		return r, syscall.Errno(e)
+		return r, e
 	}
 	return r, nil
 }

src/internal/poll/sendfile_bsd.go

@@ -35,6 +35,9 @@ func SendFile(dstFD *FD, src int, pos, remain int64) (int64, error) {
 		} else if n == 0 && err1 == nil {
 			break
 		}
+		if err1 == syscall.EINTR {
+			continue
+		}
 		if err1 == syscall.EAGAIN {
 			if err1 = dstFD.pd.waitWrite(dstFD.isFile); err1 == nil {
 				continue

src/internal/poll/sendfile_linux.go

@@ -32,6 +32,9 @@ func SendFile(dstFD *FD, src int, remain int64) (int64, error) {
 		} else if n == 0 && err1 == nil {
 			break
 		}
+		if err1 == syscall.EINTR {
+			continue
+		}
 		if err1 == syscall.EAGAIN {
 			if err1 = dstFD.pd.waitWrite(dstFD.isFile); err1 == nil {
 				continue

src/internal/poll/splice_linux.go

@@ -87,6 +87,9 @@ func spliceDrain(pipefd int, sock *FD, max int) (int, error) {
 	}
 	for {
 		n, err := splice(pipefd, sock.Sysfd, max, spliceNonblock)
+		if err == syscall.EINTR {
+			continue
+		}
 		if err != syscall.EAGAIN {
 			return n, err
 		}

src/internal/poll/writev.go

@@ -68,7 +68,10 @@ func (fd *FD) Writev(v *[][]byte) (int64, error) {
 			iovecs[i] = syscall.Iovec{}
 		}
 		if err != nil {
-			if err.(syscall.Errno) == syscall.EAGAIN {
+			if err == syscall.EINTR {
+				continue
+			}
+			if err == syscall.EAGAIN {
 				if err = fd.pd.waitWrite(fd.isFile); err == nil {
 					continue
 				}

src/os/exec_unix.go

@@ -33,9 +33,18 @@ func (p *Process) wait() (ps *ProcessState, err error) {
 		p.sigMu.Unlock()
 	}
 
-	var status syscall.WaitStatus
-	var rusage syscall.Rusage
-	pid1, e := syscall.Wait4(p.Pid, &status, 0, &rusage)
+	var (
+		status syscall.WaitStatus
+		rusage syscall.Rusage
+		pid1   int
+		e      error
+	)
+	for {
+		pid1, e = syscall.Wait4(p.Pid, &status, 0, &rusage)
+		if e != syscall.EINTR {
+			break
+		}
+	}
 	if e != nil {
 		return nil, NewSyscallError("wait", e)
 	}

src/os/wait_wait6.go

@@ -18,15 +18,20 @@ const _P_PID = 0
 // It does not actually call p.Wait.
 func (p *Process) blockUntilWaitable() (bool, error) {
 	var errno syscall.Errno
-	// The arguments on 32-bit FreeBSD look like the following:
-	// - freebsd32_wait6_args{ idtype, id1, id2, status, options, wrusage, info } or
-	// - freebsd32_wait6_args{ idtype, pad, id1, id2, status, options, wrusage, info } when PAD64_REQUIRED=1 on ARM, MIPS or PowerPC
-	if runtime.GOARCH == "386" {
-		_, _, errno = syscall.Syscall9(syscall.SYS_WAIT6, _P_PID, uintptr(p.Pid), 0, 0, syscall.WEXITED|syscall.WNOWAIT, 0, 0, 0, 0)
-	} else if runtime.GOARCH == "arm" {
-		_, _, errno = syscall.Syscall9(syscall.SYS_WAIT6, _P_PID, 0, uintptr(p.Pid), 0, 0, syscall.WEXITED|syscall.WNOWAIT, 0, 0, 0)
-	} else {
-		_, _, errno = syscall.Syscall6(syscall.SYS_WAIT6, _P_PID, uintptr(p.Pid), 0, syscall.WEXITED|syscall.WNOWAIT, 0, 0)
+	for {
+		// The arguments on 32-bit FreeBSD look like the following:
+		// - freebsd32_wait6_args{ idtype, id1, id2, status, options, wrusage, info } or
+		// - freebsd32_wait6_args{ idtype, pad, id1, id2, status, options, wrusage, info } when PAD64_REQUIRED=1 on ARM, MIPS or PowerPC
+		if runtime.GOARCH == "386" {
+			_, _, errno = syscall.Syscall9(syscall.SYS_WAIT6, _P_PID, uintptr(p.Pid), 0, 0, syscall.WEXITED|syscall.WNOWAIT, 0, 0, 0, 0)
+		} else if runtime.GOARCH == "arm" {
+			_, _, errno = syscall.Syscall9(syscall.SYS_WAIT6, _P_PID, 0, uintptr(p.Pid), 0, 0, syscall.WEXITED|syscall.WNOWAIT, 0, 0, 0)
+		} else {
+			_, _, errno = syscall.Syscall6(syscall.SYS_WAIT6, _P_PID, uintptr(p.Pid), 0, syscall.WEXITED|syscall.WNOWAIT, 0, 0)
+		}
+		if errno != syscall.EINTR {
+			break
+		}
 	}
 	runtime.KeepAlive(p)
 	if errno != 0 {

src/os/wait_waitid.go

@@ -27,7 +27,13 @@ func (p *Process) blockUntilWaitable() (bool, error) {
 	// We don't care about the values it returns.
 	var siginfo [16]uint64
 	psig := &siginfo[0]
-	_, _, e := syscall.Syscall6(syscall.SYS_WAITID, _P_PID, uintptr(p.Pid), uintptr(unsafe.Pointer(psig)), syscall.WEXITED|syscall.WNOWAIT, 0, 0)
+	var e syscall.Errno
+	for {
+		_, _, e = syscall.Syscall6(syscall.SYS_WAITID, _P_PID, uintptr(p.Pid), uintptr(unsafe.Pointer(psig)), syscall.WEXITED|syscall.WNOWAIT, 0, 0)
+		if e != syscall.EINTR {
+			break
+		}
+	}
 	runtime.KeepAlive(p)
 	if e != 0 {
 		// waitid has been available since Linux 2.6.9, but

src/runtime/crash_cgo_test.go

@@ -573,3 +573,30 @@ func TestSegv(t *testing.T) {
 		})
 	}
 }
+
+// TestEINTR tests that we handle EINTR correctly.
+// See issue #20400 and friends.
+func TestEINTR(t *testing.T) {
+	switch runtime.GOOS {
+	case "plan9", "windows":
+		t.Skipf("no EINTR on %s", runtime.GOOS)
+	case "linux":
+		if runtime.GOARCH == "386" {
+			// On linux-386 the Go signal handler sets
+			// a restorer function that is not preserved
+			// by the C sigaction call in the test,
+			// causing the signal handler to crash when
+			// returning the normal code. The test is not
+			// architecture-specific, so just skip on 386
+			// rather than doing a complicated workaround.
+			t.Skip("skipping on linux-386; C sigaction does not preserve Go restorer")
+		}
+	}
+
+	t.Parallel()
+	output := runTestProg(t, "testprogcgo", "EINTR")
+	want := "OK\n"
+	if output != want {
+		t.Fatalf("want %s, got %s\n", want, output)
+	}
+}