Date: Fri, 4 Jun 1999 07:14:20 +1000 From: Peter Jeremy <jeremyp@gsmx07.alcatel.com.au> To: FreeBSD-gnats-submit@FreeBSD.ORG Subject: kern/12014: Fix SysV Semaphore handling Message-ID: <99Jun4.065824est.40321@border.alcanet.com.au>
next in thread | raw e-mail | index | archive | help
>Number: 12014
>Category: kern
>Synopsis: Fix SysV Semaphore handling
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Thu Jun 3 14:20:01 PDT 1999
>Closed-Date:
>Last-Modified:
>Originator: Peter Jeremy
>Release: FreeBSD 4.0-CURRENT i386
>Organization:
Alcatel Australia Limited
>Environment:
Any FreeBSD {2,3,4}.x
>Description:
Following some code inspection and comparison with the sem*
man pages in Solaris 2.5, Digital UNIX 4.0D, Linux 0.99.13 and
the Open Group's `Single Unix Specification, version 2'
(http://www.opengroup.org/onlinepubs/7908799/xsh/semop.html),
I've noticed the following bugs in the FreeBSD semaphore
implementation:
1) The maximum allowed semaphore operations in semop() is checked
against MAX_SOPS instead of SEMOPM.
2) Within semop(), if an operation has sem_op == 0, the code
should check for read, rather than alter, permission.
3) Within semop(), if a sem_num is out of range, previously
commited semaphore changes are not rolled back before
returning EFBIG.
4) The code does not do range checking against SEMAEM or SEMVMX
5) The semaphore operation time is not set
6) The semaphore facility lock only works for semaphore
operations via semsys(). semget(), semop() and semconfig()
can be accessed via direct system calls as well as via
semsys().
7) semctl(..., SETALL,...) may update some semaphore elements
before returning an error.
The FreeBSD sem*(2) man pages also do not accurately reflect
the behaviour of the code.
>How-To-Repeat:
I haven't managed to build a test harness to reproduce the
problems found above. I'm happy to work with someone else
to validate the code.
>Fix:
The following fixes points 1 through 6 above. Point 7 is
not fixed, but is documented in semctl(2) under BUGS.
Note that the following fix allocates a static memory buffer
within semop() for the largest array of operations used to
date. Alternative possibilities would be to allocate and free
the memory within a single semop() call or globally within
machdep.c:cpu_startup() - as is done for sema and semu.
Index: src/sys/kern/sysv_sem.c
===================================================================
RCS file: /home/CVSROOT/./src/sys/kern/sysv_sem.c,v
retrieving revision 1.23
diff -u -r1.23 sysv_sem.c
--- sysv_sem.c 1999/04/27 12:21:09 1.23
+++ sysv_sem.c 1999/05/14 02:43:21
@@ -12,10 +12,13 @@
#include <sys/systm.h>
#include <sys/sysproto.h>
#include <sys/kernel.h>
+#include <sys/malloc.h>
#include <sys/proc.h>
#include <sys/sem.h>
#include <sys/sysent.h>
+static MALLOC_DEFINE(M_SEM, "sem", "SVID compatible semaphore data");
+
static void seminit __P((void *));
SYSINIT(sysv_sem, SI_SUB_SYSV_SEM, SI_ORDER_FIRST, seminit, NULL)
@@ -260,10 +263,12 @@
for (i = 0; i < suptr->un_cnt; i++, sunptr++) {
if (sunptr->un_id != semid || sunptr->un_num != semnum)
continue;
- if (adjval == 0)
- sunptr->un_adjval = 0;
- else
- sunptr->un_adjval += adjval;
+ if (adjval != 0) {
+ adjval += sunptr->un_adjval;
+ if (adjval > seminfo.semaem || adjval < -seminfo.semaem)
+ return (ERANGE);
+ }
+ sunptr->un_adjval = adjval;
if (sunptr->un_adjval == 0) {
suptr->un_cnt--;
if (i < suptr->un_cnt)
@@ -276,6 +281,8 @@
/* Didn't find the right entry - create it */
if (adjval == 0)
return(0);
+ if (adjval > seminfo.semaem || adjval < -seminfo.semaem)
+ return (ERANGE);
if (suptr->un_cnt != seminfo.semume) {
sunptr = &suptr->un_ent[suptr->un_cnt];
suptr->un_cnt++;
@@ -340,6 +347,7 @@
int i, rval, eval;
struct semid_ds sbuf;
register struct semid_ds *semaptr;
+ u_short usval;
#ifdef SEM_DEBUG
printf("call to semctl(%d, %d, %d, 0x%x)\n", semid, semnum, cmd, arg);
@@ -452,6 +460,8 @@
return(EINVAL);
if ((eval = copyin(arg, &real_arg, sizeof(real_arg))) != 0)
return(eval);
+ if (real_arg.val < 0 || real_arg.val > seminfo.semvmx)
+ return(ERANGE);
semaptr->sem_base[semnum].semval = real_arg.val;
semundo_clear(semid, semnum);
wakeup((caddr_t)semaptr);
@@ -464,10 +474,14 @@
return(eval);
for (i = 0; i < semaptr->sem_nsems; i++) {
eval = copyin(&real_arg.array[i],
- (caddr_t)&semaptr->sem_base[i].semval,
- sizeof(real_arg.array[0]));
+ (caddr_t)&usval, sizeof(real_arg.array[0]));
if (eval != 0)
break;
+ if (usval > seminfo.semvmx) {
+ eval = ERANGE;
+ break;
+ }
+ semaptr->sem_base[i].semval = usval;
}
semundo_clear(semid, -1);
wakeup((caddr_t)semaptr);
@@ -611,11 +625,12 @@
{
int semid = uap->semid;
int nsops = uap->nsops;
- struct sembuf sops[MAX_SOPS];
+ static struct sembuf *sops;
+ static int nsops_alloc;
register struct semid_ds *semaptr;
register struct sembuf *sopptr;
register struct sem *semptr;
- struct sem_undo *suptr = NULL;
+ struct sem_undo *suptr;
int i, j, eval;
int do_wakeup, do_undos;
@@ -634,29 +649,65 @@
if (semaptr->sem_perm.seq != IPCID_TO_SEQ(uap->semid))
return(EINVAL);
- if ((eval = ipcperm(p, &semaptr->sem_perm, IPC_W))) {
+ if (nsops > seminfo.semopm) {
#ifdef SEM_DEBUG
- printf("eval = %d from ipaccess\n", eval);
+ printf("too many sops (max=%d, nsops=%d)\n", seminfo.semopm,
+ nsops);
#endif
- return(eval);
+ return(E2BIG);
}
- if (nsops > MAX_SOPS) {
+ /* Allocate memory for sem_ops if necessary */
+ /*XXX should we use nsops or seminfo.semopm in the following? */
+ if (nsops > nsops_alloc) {
+ /* No kernel realloc, so free/malloc */
+ if (sops) {
+ free(sops, M_SEM);
+ sops = NULL;
+ nsops_alloc = 0;
+ }
+ sops = malloc(nsops * sizeof(sops[0]), M_SEM, M_WAITOK);
+ if (!sops)
+ panic("Failed to allocate %d sem_ops", nsops);
+ nsops_alloc = nsops;
#ifdef SEM_DEBUG
- printf("too many sops (max=%d, nsops=%d)\n", MAX_SOPS, nsops);
+ printf("allocated %d at %08x for sops\n", nsops,
+ nsops * sizeof(sops[0]));
#endif
- return(E2BIG);
}
- if ((eval = copyin(uap->sops, &sops, nsops * sizeof(sops[0]))) != 0) {
+ if ((eval = copyin(uap->sops, sops, nsops * sizeof(sops[0]))) != 0) {
#ifdef SEM_DEBUG
printf("eval = %d from copyin(%08x, %08x, %d)\n", eval,
- uap->sops, &sops, nsops * sizeof(sops[0]));
+ uap->sops, sops, nsops * sizeof(sops[0]));
#endif
return(eval);
}
/*
+ * Initial pass thru sops to see what permissions are needed.
+ * Also perform any checks that don't need repeating on each
+ * attempt to satisfy the request vector.
+ */
+ j = 0; /* permission needed */
+ do_undos = 0;
+ for (i = 0; i < nsops; i++) {
+ sopptr = &sops[i];
+ if (sopptr->sem_num >= semaptr->sem_nsems)
+ return(EFBIG);
+ if (sopptr->sem_flg & SEM_UNDO && sopptr->sem_op != 0)
+ do_undos = 1;
+ j |= (sopptr->sem_op == 0) ? SEM_R : SEM_A;
+ }
+
+ if ((eval = ipcperm(p->p_ucred, &semaptr->sem_perm, j))) {
+#ifdef SEM_DEBUG
+ printf("eval = %d from ipaccess\n", eval);
+#endif
+ return(eval);
+ }
+
+ /*
* Loop trying to satisfy the vector of requests.
* If we reach a point where we must wait, any requests already
* performed are rolled back and we go to sleep until some other
@@ -665,17 +716,13 @@
* This ensures that from the perspective of other tasks, a set
* of requests is atomic (never partially satisfied).
*/
- do_undos = 0;
-
for (;;) {
do_wakeup = 0;
+ eval = 0; /* error return if necessary */
for (i = 0; i < nsops; i++) {
sopptr = &sops[i];
- if (sopptr->sem_num >= semaptr->sem_nsems)
- return(EFBIG);
-
semptr = &semaptr->sem_base[sopptr->sem_num];
#ifdef SEM_DEBUG
@@ -697,21 +744,21 @@
semptr->semzcnt > 0)
do_wakeup = 1;
}
- if (sopptr->sem_flg & SEM_UNDO)
- do_undos = 1;
} else if (sopptr->sem_op == 0) {
- if (semptr->semval > 0) {
+ if (semptr->semval != 0) {
#ifdef SEM_DEBUG
printf("semop: not zero now\n");
#endif
break;
}
+ } else if (semptr->semval + sopptr->sem_op >
+ seminfo.semvmx) {
+ eval = ERANGE;
+ break;
} else {
if (semptr->semncnt > 0)
do_wakeup = 1;
semptr->semval += sopptr->sem_op;
- if (sopptr->sem_flg & SEM_UNDO)
- do_undos = 1;
}
}
@@ -731,6 +778,10 @@
semaptr->sem_base[sops[j].sem_num].semval -=
sops[j].sem_op;
+ /* If we detected an error, return it */
+ if (eval != 0)
+ return(eval);
+
/*
* If the request that we couldn't satisfy has the
* NOWAIT flag set then return with EAGAIN.
@@ -752,8 +803,6 @@
printf("semop: good morning (eval=%d)!\n", eval);
#endif
- suptr = NULL; /* sem_undo may have been reallocated */
-
if (eval != 0)
return(EINTR);
#ifdef SEM_DEBUG
@@ -764,15 +813,8 @@
* Make sure that the semaphore still exists
*/
if ((semaptr->sem_perm.mode & SEM_ALLOC) == 0 ||
- semaptr->sem_perm.seq != IPCID_TO_SEQ(uap->semid)) {
- /* The man page says to return EIDRM. */
- /* Unfortunately, BSD doesn't define that code! */
-#ifdef EIDRM
+ semaptr->sem_perm.seq != IPCID_TO_SEQ(uap->semid))
return(EIDRM);
-#else
- return(EINVAL);
-#endif
- }
/*
* The semaphore is still alive. Readjust the count of
@@ -789,6 +831,7 @@
* Process any SEM_UNDO requests.
*/
if (do_undos) {
+ suptr = NULL;
for (i = 0; i < nsops; i++) {
/*
* We only need to deal with SEM_UNDO's for non-zero
@@ -837,14 +880,15 @@
} /* loop through the sops */
} /* if (do_undos) */
- /* We're definitely done - set the sempid's */
- for (i = 0; i < nsops; i++) {
- sopptr = &sops[i];
- semptr = &semaptr->sem_base[sopptr->sem_num];
- semptr->sempid = p->p_pid;
- }
+ /* We're definitely done - set the sempid's and time */
+ for (i = 0; i < nsops; i++)
+ semaptr->sem_base[sops[i].sem_num].sempid = p->p_pid;
+ semaptr->sem_otime = time_second;
- /* Do a wakeup if any semaphore was up'd. */
+ /*
+ * Do a wakeup if any semaphore was up'd whilst something was
+ * sleeping on it.
+ */
if (do_wakeup) {
#ifdef SEM_DEBUG
printf("semop: doing wakeup\n");
@@ -875,7 +919,6 @@
{
register struct sem_undo *suptr;
register struct sem_undo **supptr;
- int did_something;
/*
* If somebody else is holding the global semaphore facility lock
@@ -887,8 +930,6 @@
#endif
(void) tsleep((caddr_t)&semlock_holder, (PZERO - 4), "semext", 0);
}
-
- did_something = 0;
/*
* Go through the chain of undo vectors looking for one
Index: src/sys/sys/sem.h
===================================================================
RCS file: /home/CVSROOT/./src/sys/sys/sem.h,v
retrieving revision 1.18
diff -u -r1.18 sem.h
--- sem.h 1998/12/14 21:34:55 1.18
+++ sem.h 1999/04/27 22:17:49
@@ -42,8 +42,6 @@
};
#define SEM_UNDO 010000
-#define MAX_SOPS 5 /* maximum # of sembuf's per semop call */
-
/*
* semctl's arg parameter structure
*/
@@ -67,8 +65,8 @@
/*
* Permissions
*/
-#define SEM_A 0200 /* alter permission */
-#define SEM_R 0400 /* read permission */
+#define SEM_A IPC_W /* alter permission */
+#define SEM_R IPC_R /* read permission */
#ifdef KERNEL
/*
Index: src/lib/libc/sys/semctl.2
===================================================================
RCS file: /home/CVSROOT/./src/lib/libc/sys/semctl.2,v
retrieving revision 1.8
diff -u -r1.8 semctl.2
--- semctl.2 1998/09/12 01:27:34 1.8
+++ semctl.2 1999/02/19 10:21:03
@@ -105,6 +105,8 @@
.Fa semnum
to
.Fa arg.val .
+Outstanding adjust on exit values for this semaphore in any process
+are cleared.
.It Dv GETPID
Return the pid of the last process to perform an operation on
semaphore number
@@ -125,6 +127,8 @@
Set the values of all of the semaphores in the set to the values
in the array pointed to by
.Fa arg.array .
+Outstanding adjust on exit values for all semaphores in this set,
+in any process are cleared.
.El
.Pp
The
@@ -150,7 +154,10 @@
.Sh RETURN VALUES
On success, when
.Fa cmd
-is one of GETVAL, GETNCNT, or GETZCNT,
+is one of
+.Dv GETVAL , GETNCNT
+or
+.Dv GETZCNT ,
.Fn semctl
returns the corresponding value; otherwise, 0 is returned.
On failure, -1 is returned, and
@@ -172,6 +179,15 @@
.It Bq Er EACCES
Permission denied due to mismatch between operation and mode of
semaphore set.
+.It Bq Er ERANGE
+.Dv SETVAL
+or
+.Dv SETALL
+attempted to set a semaphore outside the allowable range
+.Bq 0 .. Dv SEMVMX .
.Sh SEE ALSO
.Xr semget 2 ,
.Xr semop 2
+.Sh BUGS
+.Dv SETALL
+may update some semaphore elements before returning an error.
Index: src/lib/libc/sys/semop.2
===================================================================
RCS file: /home/CVSROOT/./src/lib/libc/sys/semop.2,v
retrieving revision 1.6
diff -u -r1.6 semop.2
--- semop.2 1997/03/18 23:57:27 1.6
+++ semop.2 1999/02/19 09:52:00
@@ -68,7 +68,11 @@
.Fa sem_flg
determine an operation to be performed on semaphore number
.Fa sem_num
-in the set. The values SEM_UNDO and IPC_NOWAIT may be
+in the set. The values
+.Dv SEM_UNDO
+and
+.Dv IPC_NOWAIT
+may be
.Em OR Ns 'ed
into the
.Fa sem_flg
@@ -78,16 +82,19 @@
.Fa sem_op :
.\"
.\" This section is based on the description of semop() in
-.\" Stevens, _Advanced Programming in the UNIX Environment_.
+.\" Stevens, _Advanced Programming in the UNIX Environment_,
+.\" and the semop(2) description in The Open Group Unix2 specification.
.\"
.Bl -bullet
.It
When
.Fa sem_op
-is positive, the semaphore's value is incremented by
+is positive and the process has alter permission,
+the semaphore's value is incremented by
.Fa sem_op Ns 's
-value. If SEM_UNDO is specified, the semaphore's adjust on exit
-value is decremented by
+value. If
+.Dv SEM_UNDO
+is specified, the semaphore's adjust on exit value is decremented by
.Fa sem_op Ns 's
value. A positive value for
.Fa sem_op
@@ -96,7 +103,8 @@
.It
The behavior when
.Fa sem_op
-is negative depends on the current value of the semaphore:
+is negative and the process has alter permission,
+depends on the current value of the semaphore:
.Bl -bullet
.It
If the current value of the semaphore is greater than or equal to
@@ -104,37 +112,54 @@
.Fa sem_op ,
then the value is decremented by the absolute value of
.Fa sem_op .
-If SEM_UNDO is specified, the semaphore's adjust on exit
+If
+.Dv SEM_UNDO
+is specified, the semaphore's adjust on exit
value is incremented by the absolute value of
.Fa sem_op .
.It
-If the current value of the semaphore is less than
-.Fa sem_op Ns 's
-value, one of the following happens:
+If the current value of the semaphore is less than the absolute value of
+.Fa sem_op ,
+one of the following happens:
.\" XXX a *second* sublist?
.Bl -bullet
.It
-If IPC_NOWAIT was specified, then
+If
+.Dv IPC_NOWAIT
+was specified, then
.Fn semop
-returns immediately with a return value of EAGAIN.
+returns immediately with a return value of
+.Er EAGAIN .
+.It
+Otherwise, the calling process is put to sleep until one of the following
+conditions is satisfied:
+.\" XXX We already have two sublists, why not a third?
+.Bl -bullet
.It
-If some other process has removed the semaphore with the IPC_RMID
+Some other process removes the semaphore with the
+.Dv IPC_RMID
option of
-.Fn semctl ,
-then
+.Fn semctl .
+In this case,
.Fn semop
-returns immediately with a return value of EINVAL.
+returns immediately with a return value of
+.Er EIDRM .
+.It
+The process receives a signal that is to be caught.
+In this case, the process will resume execution as defined by
+.Fn sigaction .
.It
-Otherwise, the calling process is put to sleep until the semaphore's
+The semaphore's
value is greater than or equal to the absolute value of
.Fa sem_op .
When this condition becomes true, the semaphore's value is decremented
by the absolute value of
.Fa sem_op ,
-and the semaphore's adjust on exit value is incremented by the
+the semaphore's adjust on exit value is incremented by the
absolute value of
.Fa sem_op .
.El
+.El
.Pp
A negative value for
.Fa sem_op
@@ -145,11 +170,42 @@
.It
When
.Fa sem_op
-is zero, the process waits for the semaphore's value to become zero.
-If it is already zero, the call to
+is zero and the process has read permission,
+one of the following will occur:
+.Bl -bullet
+.It
+If the current value of the semaphore is equal to zero
+then
+.Fn semop
+will return immediately.
+.It
+If
+.Dv IPC_NOWAIT
+was specified, then
.Fn semop
-can return immediately. Otherwise, the calling process is put to
-sleep until the semaphore's value becomes zero.
+returns immediately with a return value of
+.Er EAGAIN .
+.It
+Otherwise, the calling process is put to sleep until one of the following
+conditions is satisfied:
+.\" XXX Another nested sublists
+.Bl -bullet
+.It
+Some other process removes the semaphore with the
+.Dv IPC_RMID
+option of
+.Fn semctl .
+In this case,
+.Fn semop
+returns immediately with a return value of
+.Er EIDRM .
+.It
+The process receives a signal that is to be caught.
+In this case, the process will resume execution as defined by
+.Fn sigaction
+.It
+The semaphore's value becomes zero.
+.El
.El
.Pp
For each semaphore a process has in use, the kernel maintains an
@@ -170,16 +226,20 @@
.Bl -tag -width Er
.It Bq Er EINVAL
No semaphore set corresponds to
-.Fa semid .
+.Fa semid ,
+or the process would exceed the system-defined limit for the number of
+per-process SEM_UNDO structures.
.It Bq Er EACCES
Permission denied due to mismatch between operation and mode of
semaphore set.
.It Bq Er EAGAIN
-The semaphore's value was less than
-.Fa sem_op ,
-and IPC_NOWAIT was specified.
+The semaphore's value would have resulted in the process being put to sleep
+and
+.Dv IPC_NOWAIT
+was specified.
.It Bq Er E2BIG
Too many operations were specified.
+.Bq Dv SEMOPM
.It Bq Er EFBIG
.\"
.\" I'd have thought this would be EINVAL, but the source says
@@ -187,6 +247,29 @@
.\"
.Fa sem_num
was not in the range of valid semaphores for the set.
+.It Bq Er EIDRM
+The semaphore set was removed from the system.
+.It Bq Er EINTR
+The
+.Fn semop
+call was interrupted by a signal.
+.It Bq Er ENOSPC
+The system SEM_UNDO pool
+.Bq Dv SEMMNU
+is full.
+.It Bq Er ERANGE
+The requested operation would cause either
+the semaphore's current value
+.Bq Dv SEMVMX
+or its adjust on exit value
+.Bq Dv SEMAEM
+to exceed the system-imposed limits.
.Sh SEE ALSO
.Xr semctl 2 ,
-.Xr semget 2
+.Xr semget 2 ,
+.Xr sigaction 2
+.Sh BUGS
+.Fn Semop
+may block waiting for memory even if
+.Dv IPC_NOWAIT
+is specified.
>Release-Note:
>Audit-Trail:
>Unformatted:
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?99Jun4.065824est.40321>