Date: Sun, 4 May 2014 18:30:01 GMT From: Konstantin Belousov <kostikbel@gmail.com> To: freebsd-standards@FreeBSD.org Subject: Re: standards/189353: POSIX sem_unlink does not actually unlink the semaphore in the process context Message-ID: <201405041830.s44IU1cI097827@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
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 --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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201405041830.s44IU1cI097827>