[PATCH] sigtimedwait: allow failing with EINTR

[PATCH] sigtimedwait: allow failing with EINTR

[Julien Ramseier]

According to POSIX, sigtimedwait(2) is allowed to fail
with EINTR, while sigwait(3) is not, so move the retry loop there.
---

diff --git a/src/signal/sigtimedwait.c b/src/signal/sigtimedwait.c
index 0739986b..97a526da 100644
--- a/src/signal/sigtimedwait.c
+++ b/src/signal/sigtimedwait.c
@@ -1,13 +1,8 @@
 #include <signal.h>
-#include <errno.h>
 #include "syscall.h"
 #include "libc.h"
 
 int sigtimedwait(const sigset_t *restrict mask, siginfo_t *restrict si, const struct timespec *restrict timeout)
 {
-	int ret;
-	do ret = syscall_cp(SYS_rt_sigtimedwait, mask,
-		si, timeout, _NSIG/8);
-	while (ret<0 && errno==EINTR);
-	return ret;
+	return syscall_cp(SYS_rt_sigtimedwait, mask, si, timeout, _NSIG/8);
 }
diff --git a/src/signal/sigwait.c b/src/signal/sigwait.c
index c8822eea..53d04803 100644
--- a/src/signal/sigwait.c
+++ b/src/signal/sigwait.c
@@ -1,10 +1,13 @@
+#include <errno.h>
 #include <signal.h>
 
 int sigwait(const sigset_t *restrict mask, int *restrict sig)
 {
+	int ret;
 	siginfo_t si;
-	if (sigtimedwait(mask, &si, 0) < 0)
-		return -1;
+	do ret = sigtimedwait(mask, &si, 0);
+	while (ret<0 && errno==EINTR);
+	if (ret<0) return -1;
 	*sig = si.si_signo;
 	return 0;
 }

Re: [PATCH] sigtimedwait: allow failing with EINTR

[Rich Felker]

On Fri, Feb 23, 2018 at 01:09:35PM +0100, Julien Ramseier wrote:
> According to POSIX, sigtimedwait(2) is allowed to fail
> with EINTR, while sigwait(3) is not, so move the retry loop there.
> ---

This is a "may fail", not a "shall fail". Generally we prefer not to
support EINTR in cases where it's optional, since getting rid of them
with retry loops makes it safe to run on old kernels or
pseudo-linux-compat systems where SA_RESTART semantics were/are not
actually conforming. Is there a reason you want it to fail with EINTR?

Rich


> diff --git a/src/signal/sigtimedwait.c b/src/signal/sigtimedwait.c
> index 0739986b..97a526da 100644
> --- a/src/signal/sigtimedwait.c
> +++ b/src/signal/sigtimedwait.c
> @@ -1,13 +1,8 @@
>  #include <signal.h>
> -#include <errno.h>
>  #include "syscall.h"
>  #include "libc.h"
>  
>  int sigtimedwait(const sigset_t *restrict mask, siginfo_t *restrict si, const struct timespec *restrict timeout)
>  {
> -	int ret;
> -	do ret = syscall_cp(SYS_rt_sigtimedwait, mask,
> -		si, timeout, _NSIG/8);
> -	while (ret<0 && errno==EINTR);
> -	return ret;
> +	return syscall_cp(SYS_rt_sigtimedwait, mask, si, timeout, _NSIG/8);
>  }
> diff --git a/src/signal/sigwait.c b/src/signal/sigwait.c
> index c8822eea..53d04803 100644
> --- a/src/signal/sigwait.c
> +++ b/src/signal/sigwait.c
> @@ -1,10 +1,13 @@
> +#include <errno.h>
>  #include <signal.h>
>  
>  int sigwait(const sigset_t *restrict mask, int *restrict sig)
>  {
> +	int ret;
>  	siginfo_t si;
> -	if (sigtimedwait(mask, &si, 0) < 0)
> -		return -1;
> +	do ret = sigtimedwait(mask, &si, 0);
> +	while (ret<0 && errno==EINTR);
> +	if (ret<0) return -1;
>  	*sig = si.si_signo;
>  	return 0;
>  }

Re: [PATCH] sigtimedwait: allow failing with EINTR

[Julien Ramseier]

> Le 23 févr. 2018 à 22:45, Rich Felker <dalias@libc.org> a écrit :
> 
> On Fri, Feb 23, 2018 at 01:09:35PM +0100, Julien Ramseier wrote:
>> According to POSIX, sigtimedwait(2) is allowed to fail
>> with EINTR, while sigwait(3) is not, so move the retry loop there.
>> ---
> 
> This is a "may fail", not a "shall fail". Generally we prefer not to
> support EINTR in cases where it's optional, since getting rid of them
> with retry loops makes it safe to run on old kernels or
> pseudo-linux-compat systems where SA_RESTART semantics were/are not
> actually conforming. Is there a reason you want it to fail with EINTR?
> 
> Rich

Some apps seem to rely on this, notably Python where a test case expects
sigtimedwait to be interrupted when a SIGALRM is fired.

I cannot find another case in musl where EINTR is inhibited
(while being allowed by POSIX), are there any?

- Julien

Re: [PATCH] sigtimedwait: allow failing with EINTR

[Rich Felker]

On Sat, Feb 24, 2018 at 11:45:16AM +0100, Julien Ramseier wrote:
> 
> 
> > Le 23 févr. 2018 à 22:45, Rich Felker <dalias@libc.org> a écrit :
> > 
> > On Fri, Feb 23, 2018 at 01:09:35PM +0100, Julien Ramseier wrote:
> >> According to POSIX, sigtimedwait(2) is allowed to fail
> >> with EINTR, while sigwait(3) is not, so move the retry loop there.
> >> ---
> > 
> > This is a "may fail", not a "shall fail". Generally we prefer not to
> > support EINTR in cases where it's optional, since getting rid of them
> > with retry loops makes it safe to run on old kernels or
> > pseudo-linux-compat systems where SA_RESTART semantics were/are not
> > actually conforming. Is there a reason you want it to fail with EINTR?
> > 
> > Rich
> 
> Some apps seem to rely on this, notably Python where a test case expects
> sigtimedwait to be interrupted when a SIGALRM is fired.
> 
> I cannot find another case in musl where EINTR is inhibited
> (while being allowed by POSIX), are there any?

sem_wait (via sem_timedwait). This is a case where I know the kernel
bug (SYS_futex returning EINTR when it shouldn't) wasn't fixed until
relatively late, and it's particularly dangerous because callers of
sem_wait generally don't expect to return without having obtained the
semaphore, and might not even be checking for errors (since there are
no reasonable errors that could happen unless you installed
interrupting signal handlers).

Note that the timed variant always has a reasonable error you have to
check for (ETIMEDOUT) so it may make sense to move the retry there. In
the case of sem_timedwait there may be other implementation
considerations that make it difficult though; I remember something
came up discussing Alexander Monakov's ideas for new semaphore design.
Analogous behavior might be appropriate here (but see also sigwaitinfo
which is the non-timed version of sigtimedwait; sigwait is just the
legacy version).

Rich