Bug 189353

Summary:

POSIX sem_unlink does not actually unlink the semaphore in the process context

Product:

Base System

Reporter:

joris

Component:

standards

Assignee:

Antoine Brodin <antoine>

Status:

Closed FIXED

Severity:

Affects Only Me

Priority:

Normal

Version:

Unspecified

Hardware:

Any

OS:

Any

Attachments:

Description	Flags
sem_test.c	none
sem_test.c	none

Description joris 2014-05-04 12:20:00 UTC

When a posix sempahore is open, it is cached in the process address space. A call to sem_unlink does not invalidate the caching, and trying to reopen the semaphore with O_EXCL after calling sem_unlink fails with EEXIST.

How-To-Repeat: Here is a test case :

#include <fcntl.h>
#include <sys/stat.h>
#include <sys/wait.h>
#include <sys/types.h>
#include <semaphore.h>

#include <err.h>


int
main(int argc, char *argv[])
{
	sem_t *sem1, *sem2;
	int error;

	sem1 = sem_open("/test-sem1", O_CREAT | O_EXCL, S_IRWXU, 1);
	if (sem1 == SEM_FAILED)
		err(1,"Cannot create test /test-sem1 semaphore");

	error = sem_unlink("/test-sem1");

	if (error)
		err(1, "Cannot unlink semaphore /test-sem1");


	sem2 = sem_open("/test-sem1", O_CREAT | O_EXCL, S_IRWXU, 2);

	if (sem2 == SEM_FAILED)
		err(1, "Cannot re-create semaphore /test-sem1");

	error = sem_unlink("/test-sem1");

	if (error)
		err(1, "Cannot unlink semaphore /test-sem1");

	return(0);
}

Comment 1 Kostik Belousov 2014-05-04 13:37:43 UTC

Yes, I think you are right.  Please try the following patch, which worked
for me, but I only lightly tested it.

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, ...)
 
 	_pthread_mutex_lock(&sem_llock);
 	LIST_FOREACH(ni, &sem_list, next) {
-		if (strcmp(name, ni->name) == 0) {
+		if (ni->name != NULL && strcmp(name, ni->name) == 0) {
 			if ((flags & (O_CREAT|O_EXCL)) == (O_CREAT|O_EXCL)) {
 				_pthread_mutex_unlock(&sem_llock);
 				errno = EEXIST;
@@ -287,20 +287,32 @@ _sem_close(sem_t *sem)
 int
 _sem_unlink(const char *name)
 {
+	struct sem_nameinfo *ni;
 	char path[PATH_MAX];
+	int error;
 
 	if (name[0] != '/') {
 		errno = ENOENT;
 		return -1;
 	}
 	name++;
-
 	strcpy(path, SEM_PREFIX);
 	if (strlcat(path, name, sizeof(path)) >= sizeof(path)) {
 		errno = 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 != NULL && strcmp(name, ni->name) == 0) {
+			ni->name = NULL;
+			break;
+		}
+	}
+	error = unlink(path);
+	_pthread_mutex_unlock(&sem_llock);
+	return (error);
 }
 
 int

Comment 2 joris 2014-05-04 13:46:23 UTC

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.

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.

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.

This solution at least should be strictly correct, but the inode
solution could be fine in practice.

Regards,
joris

Comment 3 joris 2014-05-04 13:46:43 UTC

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.

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.

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.

This solution at least should be strictly correct, but the inode
solution could be fine in practice.

Regards,
joris

Comment 4 Kostik Belousov 2014-05-04 19:23:56 UTC

On Sun, May 04, 2014 at 02:46:23PM +0200, Joris Giovannangeli wrote:
> 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 ?

> 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.

> 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.

> 
> 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)) >= 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 (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 (ni->dev != sb.st_dev ||
+			    ni->ino != sb.st_ino ||
+			    ni->gen != sb.st_gen) {
+				ni->name = NULL;
+				ni = NULL;
+				break;
+			}
 			if ((flags & (O_CREAT|O_EXCL)) == (O_CREAT|O_EXCL)) {
-				_pthread_mutex_unlock(&sem_llock);
-				errno = EEXIST;
-				return (SEM_FAILED);
+				goto error;
 			} else {
 				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 +208,13 @@ _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)
+			goto error;
+		if (_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 +240,9 @@ _sem_open(const char *name, int flags, ...)
 	}
 	ni->open_count = 1;
 	ni->sem = sem;
+	ni->dev = sb.st_dev;
+	ni->ino = sb.st_ino;
+	ni->gen = 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;
 
 	if (name[0] != '/') {
 		errno = ENOENT;
 		return -1;
 	}
 	name++;
-
 	strcpy(path, SEM_PREFIX);
 	if (strlcat(path, name, sizeof(path)) >= sizeof(path)) {
 		errno = 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 != NULL && strcmp(name, ni->name) == 0) {
+			ni->name = NULL;
+			break;
+		}
+	}
+	error = unlink(path);
+	_pthread_mutex_unlock(&sem_llock);
+	return (error);
 }
 
 int

Comment 5 joris 2014-05-04 19:30:33 UTC

> Why didn't you provided both tests in the first report ?

Because i wrote the second one after...

> 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.

This looks fine. I don't have any other tests, but you can use the posix
test suite which has coverage for this API.

Comment 6 dfilter service freebsd_committer

2014-05-10 20:08:13 UTC

Author: kib
Date: Sat May 10 19:08:07 2014
New Revision: 265847
URL: http://svnweb.freebsd.org/changeset/base/265847

Log:
  Invalidate the cache for the named posix semaphore when opened and
  actual file storing the semaphore object is different from the file
  created on the first open.  Store the file st_dev and st_ino members
  of the struct stat in the semaphore structure on open, and compare
  them with the attributes of the opened file to detect unlink and
  re-creation.
  
  This fixes an issue of sem_unlink(3) failing to flush the named entry
  in the semaphore list for the current or remote process, making
  sem_unlink(3) not correctly operating if the unlinked semaphore is
  still opened.
  
  Reported by:	Joris Giovannangeli <joris@giovannangeli.fr>
  PR:	standards/189353
  Reviewed by:	jilles (previous version)
  Sponsored by:	The FreeBSD Foundation
  MFC after:	1 week

Modified:
  head/lib/libc/gen/sem_new.c

Modified: head/lib/libc/gen/sem_new.c
==============================================================================
--- head/lib/libc/gen/sem_new.c	Sat May 10 19:06:36 2014	(r265846)
+++ head/lib/libc/gen/sem_new.c	Sat May 10 19:08:07 2014	(r265847)
@@ -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);
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 7 dfilter service freebsd_committer

2014-05-17 12:43:17 UTC

Author: kib
Date: Sat May 17 11:43:14 2014
New Revision: 266306
URL: http://svnweb.freebsd.org/changeset/base/266306

Log:
  MFC r265847:
  Fix sem_unlink(3) to properly invalidate the semaphores name cache.
  
  PR:	standards/189353

Modified:
  stable/10/lib/libc/gen/sem_new.c
Directory Properties:
  stable/10/   (props changed)

Modified: stable/10/lib/libc/gen/sem_new.c
==============================================================================
--- stable/10/lib/libc/gen/sem_new.c	Sat May 17 11:38:26 2014	(r266305)
+++ stable/10/lib/libc/gen/sem_new.c	Sat May 17 11:43:14 2014	(r266306)
@@ -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);
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 8 dfilter service freebsd_committer

2014-05-17 17:29:43 UTC

Author: kib
Date: Sat May 17 16:29:39 2014
New Revision: 266327
URL: http://svnweb.freebsd.org/changeset/base/266327

Log:
  MFC r265847:
  Fix sem_unlink(3) to properly invalidate the semaphores name cache.
  
  PR:	standards/189353

Modified:
  stable/9/lib/libc/gen/sem_new.c
Directory Properties:
  stable/9/lib/libc/   (props changed)

Modified: stable/9/lib/libc/gen/sem_new.c
==============================================================================
--- stable/9/lib/libc/gen/sem_new.c	Sat May 17 16:28:29 2014	(r266326)
+++ stable/9/lib/libc/gen/sem_new.c	Sat May 17 16:29:39 2014	(r266327)
@@ -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);
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 9 Mark Linimon freebsd_committer

2014-05-17 17:45:09 UTC

State Changed
From-To: open->open

add misfiled PRs.

Comment 10 Mark Linimon freebsd_committer

2014-05-17 17:45:09 UTC

Responsible Changed
From-To: freebsd-standards->freebsd-standards

Comment 11 Antoine Brodin freebsd_committer

2017-12-31 22:35:55 UTC

It seems that this bug still exists when /tmp is using tmpfs.

tmpfs generates inode numbers with alloc_unr so they get reused.

Comment 12 commit-hook freebsd_committer

2018-01-13 09:54:05 UTC

A commit references this bug:

Author: antoine
Date: Sat Jan 13 09:53:15 UTC 2018
New revision: 458923
URL: https://svnweb.freebsd.org/changeset/ports/458923

Log:
  Work around sem_unlink bug on FreeBSD when /tmp is using tmpfs

  PR:		189353

Changes:
  head/security/yara/Makefile
  head/security/yara/files/
  head/security/yara/files/patch-threading.c

Comment 13 Mark Linimon freebsd_committer

2024-01-10 07:16:28 UTC

^Triage: it seems the workaround was committed back in 2018.