standards/189353: POSIX sem_unlink does not actually unlink the semaphore in the process context
Konstantin Belousov
kostikbel at gmail.com
Sun May 4 18:30:01 UTC 2014
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
--h22Fi9ANawrtbNPX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Sun, May 04, 2014 at 02:46:23PM +0200, Joris Giovannangeli wrote:
> On 04/05/2014 14:37, Konstantin Belousov wrote:
>=20
> > diff --git a/lib/libc/gen/sem_new.c b/lib/libc/gen/sem_new.c index
> > 9a2ab27..8e4a91d 100644 --- a/lib/libc/gen/sem_new.c +++
> > b/lib/libc/gen/sem_new.c @@ -161,7 +161,7 @@ _sem_open(const char
> > *name, int flags, ...)
> >=20
> > _pthread_mutex_lock(&sem_llock); 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) { if ((flags & (O_CREAT|O_EXCL)) =3D=3D
> > (O_CREAT|O_EXCL)) { _pthread_mutex_unlock(&sem_llock); errno =3D
> > EEXIST; @@ -287,20 +287,32 @@ _sem_close(sem_t *sem) int=20
> > _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++; -=20
> > 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); }
> >=20
> > int
> >=20
>=20
> I've found this issue while implementing semaphores for dragonflyBSD,
> and at first i came with the same solution than yours. But it won't
> work when you do the same in two processes : unlink will only remove
> the caching on the process calling unlink, not all of them. See
> attached test case.
Why didn't you provided both tests in the first report ?
>=20
> 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.
>=20
> For dragonfly, i'm currently trying to keep filedescriptors opened in
> the nameinfo structure. This adds quite a bit of overhead, but this
> way you can check link count with fstat during a sem_open to see if
> the semaphore file still exist, and return the cached mapping only if
> st_nlink > 0.
Yes, keeping filedescriptors for the semaphores is too heavy-weight.
If keeping fd, we could just use fifos to implement the functionality,
I think.
>=20
> This solution at least should be strictly correct, but the inode
> solution could be fine in practice.
Do you have any further tests ? The patch below passes yours' two
and tools/regression/posixsem2. I tried to be modest with open(2)
syscalls, reusing the fd in sem_open(). This also should prevent
a race.
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
@@ -66,6 +66,9 @@ __weak_reference(_sem_wait, sem_wait);
struct sem_nameinfo {
int open_count;
char *name;
+ dev_t dev;
+ ino_t ino;
+ uint32_t gen;
sem_t *sem;
LIST_ENTRY(sem_nameinfo) next;
};
@@ -151,37 +154,50 @@ _sem_open(const char *name, int flags, ...)
return (SEM_FAILED);
}
name++;
-
+ strcpy(path, SEM_PREFIX);
+ if (strlcat(path, name, sizeof(path)) >=3D sizeof(path)) {
+ errno =3D ENAMETOOLONG;
+ return (SEM_FAILED);
+ }
if (flags & ~(O_CREAT|O_EXCL)) {
errno =3D EINVAL;
return (SEM_FAILED);
}
-
+ if ((flags & O_CREAT) !=3D 0) {
+ va_start(ap, flags);
+ mode =3D va_arg(ap, int);
+ value =3D va_arg(ap, int);
+ va_end(ap);
+ }
+ fd =3D -1;
_pthread_once(&once, sem_module_init);
=20
_pthread_mutex_lock(&sem_llock);
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)) {
- _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);
}
}
}
=20
- if (flags & O_CREAT) {
- va_start(ap, flags);
- mode =3D va_arg(ap, int);
- value =3D va_arg(ap, int);
- va_end(ap);
- }
-
len =3D sizeof(*ni) + strlen(name) + 1;
ni =3D (struct sem_nameinfo *)malloc(len);
if (ni =3D=3D NULL) {
@@ -192,17 +208,13 @@ _sem_open(const char *name, int flags, ...)
ni->name =3D (char *)(ni+1);
strcpy(ni->name, name);
=20
- strcpy(path, SEM_PREFIX);
- if (strlcat(path, name, sizeof(path)) >=3D sizeof(path)) {
- errno =3D ENAMETOOLONG;
- goto error;
+ if (fd =3D=3D -1) {
+ fd =3D _open(path, flags | O_RDWR | O_CLOEXEC | O_EXLOCK, mode);
+ if (fd =3D=3D -1)
+ goto error;
+ if (_fstat(fd, &sb) =3D=3D -1)
+ goto error;
}
-
- fd =3D _open(path, flags|O_RDWR|O_CLOEXEC|O_EXLOCK, mode);
- if (fd =3D=3D -1)
- goto error;
- if (_fstat(fd, &sb))
- goto error;
if (sb.st_size < sizeof(sem_t)) {
sem_t tmp;
=20
@@ -228,6 +240,9 @@ _sem_open(const char *name, int flags, ...)
}
ni->open_count =3D 1;
ni->sem =3D sem;
+ ni->dev =3D sb.st_dev;
+ ni->ino =3D sb.st_ino;
+ ni->gen =3D sb.st_gen;
LIST_INSERT_HEAD(&sem_list, ni, next);
_close(fd);
_pthread_mutex_unlock(&sem_llock);
@@ -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);
}
=20
int
--h22Fi9ANawrtbNPX
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (FreeBSD)
iQIcBAEBAgAGBQJTZoW8AAoJEJDCuSvBvK1BilYQAJlijmMXvxNhADYUMx+xsPJf
OYiNqlp+1u+fdluJIb+CsSntAAucO2bXPwIkE4wUFmJ7f2XABvfnASRrUeOYuvjT
TQkQLb4bY1773a4KDxIapLs3EowJYi9MOxuo8lAjh/6veuplVI5OlGyVnSLDR54v
FLue4+IvBnP7sp1B1OTKJ8sQEs+ePVA6lwZG8TtTUrNt1KUVkwJ37ztJvWlE9rtx
pb+gdCMSiMWNpMinIqWeejAX4wjoQu6401t4wmQCoW7xi1Ia/IGujLamWDgRgcB+
Cl6LJNZIXRMPM5mKl+TRBAbvTvUFI0/i/WmMR/Q1OyokU3cPynwecBUk8n+cmPKg
+dOfsfCSMgl5YFevMCea5J8Kd5OhxCPjPlJxhg4nF7k+7uGrUZ4MQvsp8MOzrJWz
4AoDzMtw30vXQyJuR9KnYG0YjBbpi7fHuk7bK0VvuxQj2KGsTuqsgG43p8PTRvsz
+drvGuYmSt2CxiALlX6pHR3ku2/IuaAoKdtR87MLXxNwS9HqmfXhryKYsyz1p8bh
XxMUi33Ga3fnaL6OTi1nq7dJA/MEM6w1gnBDAiL0QuFiCIX7EktH/xldYz7BAJ4r
ln1GLkNBJzBS85AKcnbJrOo5uloqFfg6+xSQrBdbKWqmyKy+VXrn7w7Mlhtg6X+/
/byUWeJfgSvnV2hX6p2Q
=fwim
-----END PGP SIGNATURE-----
--h22Fi9ANawrtbNPX--
More information about the freebsd-standards
mailing list