Date: Mon, 5 May 2014 21:27:02 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Jilles Tjoelker <jilles@stack.nl> Cc: freebsd-standards@FreeBSD.org Subject: Re: standards/189353: POSIX sem_unlink does not actually unlink the semaphore in the process context Message-ID: <20140505182702.GK74331@kib.kiev.ua> In-Reply-To: <20140504221736.GA65318@stack.nl> References: <201405041830.s44IU1cI097827@freefall.freebsd.org> <20140504221736.GA65318@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
--rnP2AJ7yb1j09OW/
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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.
>=20
> > 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 unlin=
k the
> > semaphore in the process context
> > Date: Sun, 4 May 2014 21:23:56 +0300
>=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
> 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.
>=20
> Instead, you could use st_birthtim; since there is no specified API to
> set timestamps on semaphores, it should never change.
>=20
> 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.
>=20
> > [snip]
>=20
> > 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) =3D3D=3D3D 0) {
> > + if (ni->name !=3D3D NULL && strcmp(name, ni->name) =3D3D=3D3D 0) {
> > + fd =3D3D _open(path, flags | O_RDWR | O_CLOEXEC |
> > + O_EXLOCK, mode);
> > + if (fd =3D3D=3D3D -1 || _fstat(fd, &sb) =3D3D=3D3D -1)
> > + goto error;
> > + if (ni->dev !=3D3D sb.st_dev ||
> > + ni->ino !=3D3D sb.st_ino ||
> > + ni->gen !=3D3D sb.st_gen) {
> > + ni->name =3D3D NULL;
> > + ni =3D3D NULL;
> > + break;
> > + }
> > if ((flags & (O_CREAT|O_EXCL)) =3D3D=3D3D (O_CREAT|O_EXCL)) {
>=20
> 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.
>=20
> > - _pthread_mutex_unlock(&sem_llock);
> > - errno =3D3D EEXIST;
> > - return (SEM_FAILED);
> > + goto error;
> > } else {
> > ni->open_count++;
> > sem =3D3D 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;
> > =3D20
> > if (name[0] !=3D3D '/') {
> > errno =3D3D ENOENT;
> > return -1;
> > }
> > name++;
> > -
> > strcpy(path, SEM_PREFIX);
> > if (strlcat(path, name, sizeof(path)) >=3D3D sizeof(path)) {
> > errno =3D3D 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 !=3D3D NULL && strcmp(name, ni->name) =3D3D=3D3D 0) {
> > + ni->name =3D3D NULL;
> > + break;
> > + }
> > + }
> > + error =3D3D unlink(path);
> > + _pthread_mutex_unlock(&sem_llock);
> > + return (error);
> > }
>=20
> 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)) >=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 ((flags & (O_CREAT|O_EXCL)) =3D=3D (O_CREAT|O_EXCL)) {
- _pthread_mutex_unlock(&sem_llock);
- errno =3D EEXIST;
- return (SEM_FAILED);
- } else {
- ni->open_count++;
- sem =3D ni->sem;
- _pthread_mutex_unlock(&sem_llock);
- return (sem);
+ 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 ((flags & (O_CREAT | O_EXCL)) =3D=3D (O_CREAT |
+ O_EXCL) || ni->dev !=3D sb.st_dev ||
+ ni->ino !=3D sb.st_ino) {
+ ni->name =3D NULL;
+ ni =3D NULL;
+ break;
}
+ 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 +203,11 @@ _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 || _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 +233,8 @@ _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;
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)) >=3D sizeof(path)) {
errno =3D ENAMETOOLONG;
return (-1);
}
- return unlink(path);
+
+ return (unlink(path));
}
=20
int
--rnP2AJ7yb1j09OW/
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (FreeBSD)
iQIcBAEBAgAGBQJTZ9f1AAoJEJDCuSvBvK1B5U8P/2aQEwO6xCg0reK1Nfy0zKOB
la2webOYgYZdlOq7vwKigkZXpRv3J9IgHNGBIcZqHqku4f405nLIQEsWOygZBbwA
CNfEbv1eFK4Q1idsBrx1BLvzt7S1lttELzRodMuNBk+lJw/pFWSd+/6TJWBNzaqO
nFc0C4d15bUigBohiNWTs1+syH6sDFcVJSkckl5eV9QOW2jBnhnxsnyaYIA2KyQ8
arpkkjFGMJktCotRDWnC3isnJeSN6FRcCoIOTYS7ylfvhkRAcwPKUQ29+DxDm5uT
yfSBz5Fhokx+PRZU1h5F/XKn2VwNIIzh3wo3fllgw93OE+lCyptm59t8kvUpQ4gr
wXj+P7Gcr3DI1YP9Zt52od2FGdlMrZ3S49xa3ZS8UUVbqtPG27WCqqKKVe++YjYs
oYi0ibNLEMJDCYHXgukPjybc1vi+Q0xpLiPiY6RUbmLuhcBHKZicClETPnsVKZJQ
AcdFn7otj84b0E0OllNZo3q73JwH5gzbYNEYpy+aPOFItC7DgWdm5/u+IuNle66X
dszKnouKGtokpXKKjDpSpbQfqbn3B4giqvIRbTlD0KmHB9Htc/vdUDrysAPcgCh+
N7Gxhnl88087jGQnDYwS8M3WwaXo3JQKmoiLHI/byRouZsH6LiMi7apSplh6x+ER
7xFX5me5j9VmEZ1sECKr
=4JPI
-----END PGP SIGNATURE-----
--rnP2AJ7yb1j09OW/--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140505182702.GK74331>