standards/189353: POSIX sem_unlink does not actually unlink the semaphore in the process context
Konstantin Belousov
kostikbel at gmail.com
Mon May 5 18:27:13 UTC 2014
On Mon, May 05, 2014 at 12:17:36AM +0200, Jilles Tjoelker wrote:
> On Sun, May 04, 2014 at 06:30:01PM +0000, Konstantin Belousov wrote:
> > The following reply was made to PR standards/189353; it has been noted
> > by GNATS.
>
> > From: Konstantin Belousov <kostikbel at gmail.com>
> > To: Joris Giovannangeli <joris at giovannangeli.fr>
> > Cc: freebsd-gnats-submit at FreeBSD.org
> > Subject: Re: standards/189353: POSIX sem_unlink does not actually unlink the
> > semaphore in the process context
> > Date: Sun, 4 May 2014 21:23:56 +0300
>
> > > It looks like glibc is using inodes numbers and dev number for that,
> > > which, while not being strictly correct (inodes can be reused), seems
> > > to work fine on linux.
> > Inode number + generation provides the same practical guarantee as the
> > file descriptor. You cannot have two inodes with the same (inum, gen)
> > simultaneously, generation would need some years to wrap.
>
> Unfortunately, st_gen is zeroed for users without PRIV_VFS_GENERATION
> (this is to make it harder to forge NFS file handles). As a result, it
> works less well for non-root users and may incorrectly not reuse a
> nameinfo if the process dropped root privileges between opens.
>
> Instead, you could use st_birthtim; since there is no specified API to
> set timestamps on semaphores, it should never change.
>
> On another note, I wonder whether the generation number or birth time is
> necessary at all. The old file is still open because it is mapped, so it
> will not be deleted completely. There can only be a problem if the
> filesystem's file identifier is larger than an ino_t and therefore the
> value in st_ino is truncated.
I see. I just removed the gen from the semaphore structure.
>
> > [snip]
>
> > diff --git a/lib/libc/gen/sem_new.c b/lib/libc/gen/sem_new.c
> > index 9a2ab27..ed58de3 100644
> > --- a/lib/libc/gen/sem_new.c
> > +++ b/lib/libc/gen/sem_new.c
> > [snip]
> > LIST_FOREACH(ni, &sem_list, next) {
> > - if (strcmp(name, ni->name) =3D=3D 0) {
> > + if (ni->name !=3D NULL && strcmp(name, ni->name) =3D=3D 0) {
> > + fd =3D _open(path, flags | O_RDWR | O_CLOEXEC |
> > + O_EXLOCK, mode);
> > + if (fd =3D=3D -1 || _fstat(fd, &sb) =3D=3D -1)
> > + goto error;
> > + if (ni->dev !=3D sb.st_dev ||
> > + ni->ino !=3D sb.st_ino ||
> > + ni->gen !=3D sb.st_gen) {
> > + ni->name =3D NULL;
> > + ni =3D NULL;
> > + break;
> > + }
> > if ((flags & (O_CREAT|O_EXCL)) =3D=3D (O_CREAT|O_EXCL)) {
>
> If dev/ino/gen are reused and open with O_CREAT | O_EXCL succeeded, the
> semaphore was apparently recreated, so this check should be moved to the
> previous if.
Agreed.
>
> > - _pthread_mutex_unlock(&sem_llock);
> > - errno =3D EEXIST;
> > - return (SEM_FAILED);
> > + goto error;
> > } else {
> > ni->open_count++;
> > sem =3D ni->sem;
> > _pthread_mutex_unlock(&sem_llock);
> > + _close(fd);
> > return (sem);
> > }
> > }
> > }
> > [snip]
> > @@ -287,20 +302,32 @@ _sem_close(sem_t *sem)
> > int
> > _sem_unlink(const char *name)
> > {
> > + struct sem_nameinfo *ni;
> > char path[PATH_MAX];
> > + int error;
> > =20
> > if (name[0] !=3D '/') {
> > errno =3D ENOENT;
> > return -1;
> > }
> > name++;
> > -
> > strcpy(path, SEM_PREFIX);
> > if (strlcat(path, name, sizeof(path)) >=3D sizeof(path)) {
> > errno =3D ENAMETOOLONG;
> > return (-1);
> > }
> > - return unlink(path);
> > +
> > + _pthread_once(&once, sem_module_init);
> > + _pthread_mutex_lock(&sem_llock);
> > + LIST_FOREACH(ni, &sem_list, next) {
> > + if (ni->name !=3D NULL && strcmp(name, ni->name) =3D=3D 0) {
> > + ni->name =3D NULL;
> > + break;
> > + }
> > + }
> > + error =3D unlink(path);
> > + _pthread_mutex_unlock(&sem_llock);
> > + return (error);
> > }
>
> For efficiency and reduction of code paths, perhaps the loop over
> sem_list should be removed. It solves the reuse issue for sem_unlink()
> by the same process, but the fstat()-based code in sem_open() solves it
> for sem_unlink() by any process.
Agreed again. What is left from the chunk is actually a style change,
that will be committed separately.
Updated patch below, thank you for the notes.
diff --git a/lib/libc/gen/sem_new.c b/lib/libc/gen/sem_new.c
index 9a2ab27..ec1a2fd 100644
--- a/lib/libc/gen/sem_new.c
+++ b/lib/libc/gen/sem_new.c
@@ -66,6 +66,8 @@ __weak_reference(_sem_wait, sem_wait);
struct sem_nameinfo {
int open_count;
char *name;
+ dev_t dev;
+ ino_t ino;
sem_t *sem;
LIST_ENTRY(sem_nameinfo) next;
};
@@ -151,37 +153,46 @@ _sem_open(const char *name, int flags, ...)
return (SEM_FAILED);
}
name++;
-
+ strcpy(path, SEM_PREFIX);
+ if (strlcat(path, name, sizeof(path)) >= sizeof(path)) {
+ errno = ENAMETOOLONG;
+ return (SEM_FAILED);
+ }
if (flags & ~(O_CREAT|O_EXCL)) {
errno = EINVAL;
return (SEM_FAILED);
}
-
+ if ((flags & O_CREAT) != 0) {
+ va_start(ap, flags);
+ mode = va_arg(ap, int);
+ value = va_arg(ap, int);
+ va_end(ap);
+ }
+ fd = -1;
_pthread_once(&once, sem_module_init);
_pthread_mutex_lock(&sem_llock);
LIST_FOREACH(ni, &sem_list, next) {
- if (strcmp(name, ni->name) == 0) {
- if ((flags & (O_CREAT|O_EXCL)) == (O_CREAT|O_EXCL)) {
- _pthread_mutex_unlock(&sem_llock);
- errno = EEXIST;
- return (SEM_FAILED);
- } else {
- ni->open_count++;
- sem = ni->sem;
- _pthread_mutex_unlock(&sem_llock);
- return (sem);
+ if (ni->name != NULL && strcmp(name, ni->name) == 0) {
+ fd = _open(path, flags | O_RDWR | O_CLOEXEC |
+ O_EXLOCK, mode);
+ if (fd == -1 || _fstat(fd, &sb) == -1)
+ goto error;
+ if ((flags & (O_CREAT | O_EXCL)) == (O_CREAT |
+ O_EXCL) || ni->dev != sb.st_dev ||
+ ni->ino != sb.st_ino) {
+ ni->name = NULL;
+ ni = NULL;
+ break;
}
+ ni->open_count++;
+ sem = ni->sem;
+ _pthread_mutex_unlock(&sem_llock);
+ _close(fd);
+ return (sem);
}
}
- if (flags & O_CREAT) {
- va_start(ap, flags);
- mode = va_arg(ap, int);
- value = va_arg(ap, int);
- va_end(ap);
- }
-
len = sizeof(*ni) + strlen(name) + 1;
ni = (struct sem_nameinfo *)malloc(len);
if (ni == NULL) {
@@ -192,17 +203,11 @@ _sem_open(const char *name, int flags, ...)
ni->name = (char *)(ni+1);
strcpy(ni->name, name);
- strcpy(path, SEM_PREFIX);
- if (strlcat(path, name, sizeof(path)) >= sizeof(path)) {
- errno = ENAMETOOLONG;
- goto error;
+ if (fd == -1) {
+ fd = _open(path, flags | O_RDWR | O_CLOEXEC | O_EXLOCK, mode);
+ if (fd == -1 || _fstat(fd, &sb) == -1)
+ goto error;
}
-
- fd = _open(path, flags|O_RDWR|O_CLOEXEC|O_EXLOCK, mode);
- if (fd == -1)
- goto error;
- if (_fstat(fd, &sb))
- goto error;
if (sb.st_size < sizeof(sem_t)) {
sem_t tmp;
@@ -228,6 +233,8 @@ _sem_open(const char *name, int flags, ...)
}
ni->open_count = 1;
ni->sem = sem;
+ ni->dev = sb.st_dev;
+ ni->ino = sb.st_ino;
LIST_INSERT_HEAD(&sem_list, ni, next);
_close(fd);
_pthread_mutex_unlock(&sem_llock);
@@ -294,13 +301,13 @@ _sem_unlink(const char *name)
return -1;
}
name++;
-
strcpy(path, SEM_PREFIX);
if (strlcat(path, name, sizeof(path)) >= sizeof(path)) {
errno = ENAMETOOLONG;
return (-1);
}
- return unlink(path);
+
+ return (unlink(path));
}
int
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-standards/attachments/20140505/ac51b707/attachment.sig>
More information about the freebsd-standards
mailing list