Date: Mon, 5 May 2014 00:17:36 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-standards@FreeBSD.org Subject: Re: standards/189353: POSIX sem_unlink does not actually unlink the semaphore in the process context Message-ID: <20140504221736.GA65318@stack.nl> In-Reply-To: <201405041830.s44IU1cI097827@freefall.freebsd.org> References: <201405041830.s44IU1cI097827@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@gmail.com>
> To: Joris Giovannangeli <joris@giovannangeli.fr>
> Cc: freebsd-gnats-submit@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.
> [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.
> - _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.
--
Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140504221736.GA65318>