[PATCH v2] Fix EINTR handling when reading from a pipe.

[PATCH v2] Fix EINTR handling when reading from a pipe.

[Ignacy Gawędzki]

When using tee to peek at stdin pipe, consider forwarding EINTR
without attempting to read, otherwise pending signals don't get
handled properly.

Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
---
 src/input.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/input.c b/src/input.c
index 71282fb..fd5c36f 100644
--- a/src/input.c
+++ b/src/input.c
@@ -323,11 +323,11 @@ retry:
 
 	if (!fd && !stdin_bufferable()) {
 		nr = stdin_tee(buf, nr);
-		fd = stdin_state.pip[0];
-		if (nr <= 0) {
+		if (nr == 0 || (nr < 0 && errno != EINTR)) {
 			fd = 0;
 			nr = 1;
-		}
+		} else
+			fd = stdin_state.pip[0];
 	}
 
 	if (nr >= 0)
-- 
2.51.0

[v3 PATCH] input: Fix EINTR handling when reading from a pipe

[Herbert Xu]

Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr> wrote:
> When using tee to peek at stdin pipe, consider forwarding EINTR
> without attempting to read, otherwise pending signals don't get
> handled properly.
> 
> Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
> ---
> src/input.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)

Thanks for the patch.  I think we need to restrict the fallback
path even further.  In fact, only EINVAL should enter the fallback
path.

---8<---
Errors from tee(2) should not lead to the fallback path in every
case.  In fact, only EINVAL should trigger an attempt to call
read(2).

Every other error (and zero == EOF) returned by tee(2) should be
treated as if it came from read(2).

Reported-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/configure.ac b/configure.ac
index c37eefe..9a55ea0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -92,7 +92,7 @@ dnl Checks for library functions.
 AC_CHECK_FUNCS(bsearch faccessat getpwnam getrlimit isalpha killpg \
 	       memfd_create memrchr mempcpy \
 	       sigsetmask stpcpy strchrnul strsignal strtod strtoimax \
-	       strtoumax sysconf tee wait3)
+	       strtoumax sysconf wait3)
 
 dnl Check whether it's worth working around FreeBSD PR kern/125009.
 dnl The traditional behavior of access/faccessat is crazy, but
@@ -135,6 +135,10 @@ if test "$use_fnmatch" = yes && test "$enable_glob" = yes; then
 fi
 
 if test "$enable_tee" != no; then
+	AC_CHECK_FUNCS(tee, use_tee=yes)
+fi
+
+if test "$use_tee" = yes; then
 	AC_DEFINE([USE_TEE], [1], [Non-zero if tee(2) should be used])
 else
 	AC_DEFINE([USE_TEE], [0], [Non-zero if tee(2) should be used])
diff --git a/src/input.c b/src/input.c
index 71282fb..4e30010 100644
--- a/src/input.c
+++ b/src/input.c
@@ -183,7 +183,12 @@ static int stdin_tee(void *buf, int nr)
 
 	flush_tee(buf, nr, stdin_state.pending);
 
-	err = USE_TEE ? tee(0, stdin_state.pip[1], nr, 0) : -1;
+	if (USE_TEE)
+		err = tee(0, stdin_state.pip[1], nr, 0);
+	else {
+		errno = EINVAL;
+		err = -1;
+	}
 	stdin_state.pending = err;
 	return err;
 }
@@ -324,13 +329,13 @@ retry:
 	if (!fd && !stdin_bufferable()) {
 		nr = stdin_tee(buf, nr);
 		fd = stdin_state.pip[0];
-		if (nr <= 0) {
+		if (nr < 0 && errno == EINVAL) {
 			fd = 0;
 			nr = 1;
 		}
 	}
 
-	if (nr >= 0)
+	if (nr > 0)
 		nr = read(fd, buf, nr);
 
 	if (nr < 0) {
diff --git a/src/system.h b/src/system.h
index 8cb4726..2138989 100644
--- a/src/system.h
+++ b/src/system.h
@@ -119,13 +119,6 @@ long sysconf(int) __attribute__((__noreturn__));
 int isblank(int c);
 #endif
 
-#ifndef HAVE_TEE
-static inline ssize_t tee(int fd_in, int fd_out, size_t len, unsigned int flags)
-{
-	return -1;
-}
-#endif
-
 #ifndef HAVE_FNMATCH
 static inline int fnmatch(const char *pattern, const char *string, int flags)
 {
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[v4 PATCH] input: Fix EINTR handling when reading from a pipe

[Herbert Xu]

Restore the fallback tee definition so that it actually compiles.

---8<---
Errors from tee(2) should not lead to the fallback path in every
case.  In fact, only EINVAL should trigger an attempt to call
read(2).

Every other error (and zero == EOF) returned by tee(2) should be
treated as if it came from read(2).

Reported-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/configure.ac b/configure.ac
index c37eefe..d749440 100644
--- a/configure.ac
+++ b/configure.ac
@@ -135,6 +135,10 @@ if test "$use_fnmatch" = yes && test "$enable_glob" = yes; then
 fi
 
 if test "$enable_tee" != no; then
+	AC_CHECK_FUNCS(tee, use_tee=yes)
+fi
+
+if test "$use_tee" = yes; then
 	AC_DEFINE([USE_TEE], [1], [Non-zero if tee(2) should be used])
 else
 	AC_DEFINE([USE_TEE], [0], [Non-zero if tee(2) should be used])
diff --git a/src/input.c b/src/input.c
index 71282fb..4e30010 100644
--- a/src/input.c
+++ b/src/input.c
@@ -183,7 +183,12 @@ static int stdin_tee(void *buf, int nr)
 
 	flush_tee(buf, nr, stdin_state.pending);
 
-	err = USE_TEE ? tee(0, stdin_state.pip[1], nr, 0) : -1;
+	if (USE_TEE)
+		err = tee(0, stdin_state.pip[1], nr, 0);
+	else {
+		errno = EINVAL;
+		err = -1;
+	}
 	stdin_state.pending = err;
 	return err;
 }
@@ -324,13 +329,13 @@ retry:
 	if (!fd && !stdin_bufferable()) {
 		nr = stdin_tee(buf, nr);
 		fd = stdin_state.pip[0];
-		if (nr <= 0) {
+		if (nr < 0 && errno == EINVAL) {
 			fd = 0;
 			nr = 1;
 		}
 	}
 
-	if (nr >= 0)
+	if (nr > 0)
 		nr = read(fd, buf, nr);
 
 	if (nr < 0) {
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt