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); }
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
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
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
> 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.
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"
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"
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"
State Changed From-To: open->open add misfiled PRs.
Responsible Changed From-To: freebsd-standards->freebsd-standards
It seems that this bug still exists when /tmp is using tmpfs. tmpfs generates inode numbers with alloc_unr so they get reused.
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
^Triage: it seems the workaround was committed back in 2018.