svn commit: r249566 - in head: lib/libc/gen sys/kern

[Jilles Tjoelker]

On Tue, Apr 16, 2013 at 08:26:31PM +0000, John Baldwin wrote:
> Author: jhb
> Date: Tue Apr 16 20:26:31 2013
> New Revision: 249566
> URL: http://svnweb.freebsd.org/changeset/base/249566

> Log:
>   - Document that sem_wait() can fail with EINTR if it is interrupted by a
>     signal.
>   - Fix the old ksem implementation for POSIX semaphores to not restart
>     sem_wait() or sem_timedwait() if interrupted by a signal.

>   MFC after:	1 week

> Modified:
>   head/lib/libc/gen/sem_wait.3
>   head/sys/kern/uipc_sem.c

> Modified: head/lib/libc/gen/sem_wait.3
> ==============================================================================
> --- head/lib/libc/gen/sem_wait.3	Tue Apr 16 20:21:02 2013	(r249565)
> +++ head/lib/libc/gen/sem_wait.3	Tue Apr 16 20:26:31 2013	(r249566)
> @@ -27,7 +27,7 @@
>  .\"
>  .\" $FreeBSD$
>  .\"
> -.Dd February 15, 2000
> +.Dd April 16, 2013
>  .Dt SEM_WAIT 3
>  .Os
>  .Sh NAME
> @@ -75,6 +75,14 @@ points to an invalid semaphore.
>  .El
>  .Pp
>  Additionally,
> +.Fn sem_wait
> +will fail if:
> +.Bl -tag -width Er
> +.Pp
> +.It Bq Er EINTR
> +A signal interrupted this function.
> +.El
> +Additionally,
>  .Fn sem_trywait
>  will fail if:
>  .Bl -tag -width Er
> 
> Modified: head/sys/kern/uipc_sem.c
> ==============================================================================
> --- head/sys/kern/uipc_sem.c	Tue Apr 16 20:21:02 2013	(r249565)
> +++ head/sys/kern/uipc_sem.c	Tue Apr 16 20:26:31 2013	(r249566)
> @@ -846,6 +846,8 @@ kern_sem_wait(struct thread *td, semid_t
>  err:
>  	mtx_unlock(&sem_lock);
>  	fdrop(fp, td);
> +	if (error == ERESTART)
> +		error = EINTR;
>  	DP(("<<< kern_sem_wait leaving, pid=%d, error = %d\n",
>  	    (int)td->td_proc->p_pid, error));
>  	return (error);

It would normally be expected (and required by POSIX) that a signal with
SA_RESTART set does not cause a function to fail with [EINTR], so more
rationale is needed here.

Also, the timeouts passed to these functions are absolute and also are
at the system call level except UMTX_OP_SEM_WAIT which can take a
relative or an absolute timeout but libc passed it an absolute timeout.
So there is no need for a caller to see [EINTR] to avoid overlong
timeouts.

Given the number of times people call sem_wait() without checking the
return value, call sem_wait() and assume any error is fatal or call
sem_timedwait() and assume any error is a timeout, I think it may be
better to go the other way and never fail with [EINTR], just like the
pthread functions. POSIX explicitly permits this: the [EINTR] error for
sem_wait(), sem_trywait() and sem_timedwait() is "may fail" rather than
the usual "shall fail" (note that SA_RESTART overrides such a "shall
fail" unless there is a relative timeout or it is explicitly excluded
with the function).

I realize that this reduces functionality, but relying on [EINTR] here
introduces a race condition anyway (because the signal handler may be
called just before sem_wait() blocks). I suggest pthread cancellation,
not returning from the signal handler or leaving the default
action in place (if it is termination). The racy functionality can be
kept by leaving [EINTR] and [ERESTART] from the sleep function as they
are except for changing [ERESTART] to [EINTR] when UMTX_OP_SEM_WAIT was
passed a relative timeout. Breakage in programs that check sem_wait()'s
return value incorrectly will then be less frequent and only present
when POSIX permits it.

-- 
Jilles Tjoelker