Re: sem_wait and EINTR

[Rich Felker <dalias@libc.org>]

On Wed, Dec 05, 2018 at 07:16:05PM +0000, Orivej Desh wrote:
> Hi,
> 
> musl differs from glibc in that it does not return from sem_wait() on EINTR.
> This mail [1] explains that this is useful to safeguard the software that does
> not check sem_wait() return code. However, since glibc does return EINTR, such
> bugs in the open source software seem to be eventually noticed and fixed.
> 
> The musl behaviour has a disadvantage in that it makes sem_wait() difficult to
> interrupt (and delays the return from sem_timedwait() until the timeout), which
> is relied upon in particular by multithreaded fuse for breaking out of the
> main thread waiting loop [2]. IMHO the fuse implementation is sensible, since it
> looks better than the alternatives I could imagine, and I'm inclined to patch
> musl like this [3] to meet its expectations.
> 
> Am I missing some implications? Would you reconsider returning from sem_wait()
> on EINTR? Could you suggest a good fix for fuse that does not change musl?
> 
> [1] https://www.openwall.com/lists/musl/2018/02/24/3
> [2] https://github.com/libfuse/libfuse/blob/fuse-3.3.0/lib/fuse_loop_mt.c#L332
> [3] https://github.com/orivej/musl/commit/c4c38aaab4fc55c23669f7b81386b615609cc3e1
> 
> diff --git a/src/thread/sem_timedwait.c b/src/thread/sem_timedwait.c
> index 8132eb1b..58d3ebfe 100644
> --- a/src/thread/sem_timedwait.c
> +++ b/src/thread/sem_timedwait.c
> @@ -22,7 +22,7 @@ int sem_timedwait(sem_t *restrict sem, const struct timespec *restrict at)
>  		pthread_cleanup_push(cleanup, (void *)(sem->__val+1));
>  		r = __timedwait_cp(sem->__val, -1, CLOCK_REALTIME, at, sem->__val[2]);
>  		pthread_cleanup_pop(1);
> -		if (r && r != EINTR) {
> +		if (r) {
>  			errno = r;
>  			return -1;
>  		}
> diff --git a/src/thread/synccall.c b/src/thread/synccall.c
> index cc66bd24..d9ab40cb 100644
> --- a/src/thread/synccall.c
> +++ b/src/thread/synccall.c
> @@ -37,10 +37,10 @@ static void handler(int sig)
>  	if (a_cas(&target_tid, ch.tid, 0) == (ch.tid | 0x80000000))
>  		__syscall(SYS_futex, &target_tid, FUTEX_UNLOCK_PI|FUTEX_PRIVATE);
>  
> -	sem_wait(&ch.target_sem);
> +	while (sem_wait(&ch.target_sem) && errno != EINTR);
>  	callback(context);
>  	sem_post(&ch.caller_sem);
> -	sem_wait(&ch.target_sem);
> +	while (sem_wait(&ch.target_sem) && errno != EINTR);
>  
>  	errno = old_errno;
>  }
> @@ -153,7 +153,7 @@ void __synccall(void (*func)(void *), void *ctx)
>  	/* Serialize execution of callback in caught threads. */
>  	for (cp=head; cp; cp=cp->next) {
>  		sem_post(&cp->target_sem);
> -		sem_wait(&cp->caller_sem);
> +		while (sem_wait(&cp->caller_sem) && errno != EINTR);
>  	}
>  
>  	sa.sa_handler = SIG_IGN;

I think the changes to __synccall are unnecessary noise. It
necessarily runs with all signals, even implementation-internal ones,
blocked. Did you just miss this or do you think there's a reason the
checks need to be added?

Rich