From c83b1a12aade948eb2df24b2f4bf957537eed3f1 Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Thu, 22 Jan 2026 18:06:28 -0600 Subject: [PATCH] roken: Use OFD locks, flock, or POSIX locking, same as MIT --- cf/roken-frag.m4 | 5 +- lib/krb5/fcache.c | 38 +++--- lib/roken/Makefile.am | 1 + lib/roken/flock.c | 283 +++++++++++++++++++++++++++++------------- lib/roken/roken.h.in | 13 +- 5 files changed, 228 insertions(+), 112 deletions(-) diff --git a/cf/roken-frag.m4 b/cf/roken-frag.m4 index ae9f58f58..a554dc8f0 100644 --- a/cf/roken-frag.m4 +++ b/cf/roken-frag.m4 @@ -334,7 +334,6 @@ AC_BROKEN([ \ err \ errx \ fchown \ - flock \ fnmatch \ freehostent \ getcwd \ @@ -402,6 +401,10 @@ AC_BROKEN([ \ rk_LIBOBJ(closefrom) +dnl Check for flock() - we always provide rk_flock() but need to know if +dnl the system has flock() for our fallback chain +AC_CHECK_FUNCS([flock]) + AM_CONDITIONAL(have_fnmatch_h, test "$ac_cv_header_fnmatch_h" = yes -a "$ac_cv_func_fnmatch" = yes) diff --git a/lib/krb5/fcache.c b/lib/krb5/fcache.c index 00a72e776..1bed403f8 100644 --- a/lib/krb5/fcache.c +++ b/lib/krb5/fcache.c @@ -90,20 +90,22 @@ _krb5_xlock(krb5_context context, int fd, krb5_boolean exclusive, const char *filename) { int ret; -#ifdef HAVE_FCNTL - struct flock l; - l.l_start = 0; - l.l_len = 0; - l.l_type = exclusive ? F_WRLCK : F_RDLCK; - l.l_whence = SEEK_SET; - ret = fcntl(fd, F_SETLKW, &l); -#else - ret = flock(fd, exclusive ? LOCK_EX : LOCK_SH); -#endif - if(ret < 0) + /* + * Use rk_flock(), which uses the first to work of these on Linux and + * Unix-like systems: + * + * - OFD fcntl() locks (works on NFS; thread-safe) + * - BSD flock() (does not work on NFS; thread-safe) + * - POSIX fcntl() locks (works on NFS; NOT thread-safe, because POSIX byte + * range file locking semantics are insane) + * + * On Windows it uses LockFileEx(). + */ + ret = rk_flock(fd, exclusive ? LOCK_EX : LOCK_SH); + if (ret < 0) ret = errno; - if(ret == EACCES) /* fcntl can return EACCES instead of EAGAIN */ + if (ret == EACCES) /* fcntl can return EACCES instead of EAGAIN */ ret = EAGAIN; switch (ret) { @@ -133,16 +135,8 @@ KRB5_LIB_FUNCTION int KRB5_LIB_CALL _krb5_xunlock(krb5_context context, int fd) { int ret; -#ifdef HAVE_FCNTL - struct flock l; - l.l_start = 0; - l.l_len = 0; - l.l_type = F_UNLCK; - l.l_whence = SEEK_SET; - ret = fcntl(fd, F_SETLKW, &l); -#else - ret = flock(fd, LOCK_UN); -#endif + + ret = rk_flock(fd, LOCK_UN); if (ret < 0) ret = errno; switch (ret) { diff --git a/lib/roken/Makefile.am b/lib/roken/Makefile.am index 0a2d850d5..115e759e8 100644 --- a/lib/roken/Makefile.am +++ b/lib/roken/Makefile.am @@ -139,6 +139,7 @@ libroken_la_SOURCES = \ eread.c \ esetenv.c \ ewrite.c \ + flock.c \ fseeko.c \ ftello.c \ getaddrinfo_hostspec.c \ diff --git a/lib/roken/flock.c b/lib/roken/flock.c index 068d09929..883a938d8 100644 --- a/lib/roken/flock.c +++ b/lib/roken/flock.c @@ -33,122 +33,233 @@ #include -#ifndef HAVE_FLOCK +#ifdef HAVE_SYS_FILE_H +#include +#endif #include "roken.h" +/* Undo the flock -> rk_flock redirection for this file */ +#undef flock + +/* + * Implement flock() semantics with the best available locking mechanism. + * + * We prefer OFD (Open File Description) locks when available because they: + * - Are associated with the file description, not the process + * - Work correctly with threads (each open() gets its own lock) + * - Are inherited across fork() with the file descriptor + * - Don't have the POSIX lock problem where close() on ANY fd to the + * same file releases all locks + * + * Fallback order: + * 1. OFD locks (F_OFD_SETLK/F_OFD_SETLKW) - Linux 3.15+, FreeBSD 13+ + * 2. BSD flock() - works on most local filesystems + * 3. POSIX fcntl() locks - most portable, but has issues + * + * We also handle EINVAL/ENOLCK from flock() by falling back to fcntl(), + * which helps on filesystems that don't support flock() (e.g., some NFS). + */ + +#ifndef LOCK_SH +#define LOCK_SH 1 /* Shared lock */ +#endif +#ifndef LOCK_EX +#define LOCK_EX 2 /* Exclusive lock */ +#endif +#ifndef LOCK_NB +#define LOCK_NB 4 /* Don't block when locking */ +#endif +#ifndef LOCK_UN +#define LOCK_UN 8 /* Unlock */ +#endif + #define OP_MASK (LOCK_SH | LOCK_EX | LOCK_UN) +#if defined(HAVE_FCNTL) && defined(F_SETLK) +static int +flock_fcntl(int fd, int operation, int ofd) +{ + struct flock l; + int cmd; + + l.l_whence = SEEK_SET; + l.l_start = 0; + l.l_len = 0; /* 0 means to EOF */ + + switch (operation & OP_MASK) { + case LOCK_UN: + l.l_type = F_UNLCK; +#ifdef F_OFD_SETLK + cmd = ofd ? F_OFD_SETLK : F_SETLK; +#else + cmd = F_SETLK; +#endif + break; + case LOCK_SH: + l.l_type = F_RDLCK; +#ifdef F_OFD_SETLK + if (ofd) + cmd = (operation & LOCK_NB) ? F_OFD_SETLK : F_OFD_SETLKW; + else +#endif + cmd = (operation & LOCK_NB) ? F_SETLK : F_SETLKW; + break; + case LOCK_EX: + l.l_type = F_WRLCK; +#ifdef F_OFD_SETLK + if (ofd) + cmd = (operation & LOCK_NB) ? F_OFD_SETLK : F_OFD_SETLKW; + else +#endif + cmd = (operation & LOCK_NB) ? F_SETLK : F_SETLKW; + break; + default: + errno = EINVAL; + return -1; + } + return fcntl(fd, cmd, &l); +} +#endif /* HAVE_FCNTL && F_SETLK */ ROKEN_LIB_FUNCTION int ROKEN_LIB_CALL rk_flock(int fd, int operation) { + int ret; + +#if defined(HAVE_FCNTL) && defined(F_OFD_SETLK) + /* + * Try OFD locks first -- the sane variant of POSIX byte range file + * locking, and it should work on NFS (it's the client that implements sane + * or insane POSIX semantics, not the server, and the protocol is the same + * either way). + * + * Note that even if F_OFD_SETLK is defined in headers, the kernel might + * not support it (e.g., old kernel with new userspace), so we have a + * run-time fallback for OFD locks. + */ + ret = flock_fcntl(fd, operation, 1); + if (ret == 0) + return 0; + if (errno != EINVAL) + return ret; +#endif + +#ifdef HAVE_FLOCK + { + int op; + + switch (operation & OP_MASK) { + case LOCK_UN: + op = LOCK_UN; + break; + case LOCK_SH: + op = LOCK_SH; + break; + case LOCK_EX: + op = LOCK_EX; + break; + default: + errno = EINVAL; + return -1; + } + if (operation & LOCK_NB) + op |= LOCK_NB; + + /* + * Note: we call the real flock() here, not rk_flock() recursively. + * The roken.h header renames flock to rk_flock, but that only + * affects code that includes roken.h. Since we're implementing + * rk_flock, we get the real system flock(). + */ + ret = flock(fd, op); + if (ret == 0) + return 0; + /* + * Some filesystems (e.g., NFS) don't support flock(). + * Fall back to POSIX locks. + */ + if (errno != EINVAL && errno != ENOLCK && errno != ENOTSUP) + return ret; + } +#endif /* HAVE_FLOCK */ + #if defined(HAVE_FCNTL) && defined(F_SETLK) - struct flock arg; - int code, cmd; - - arg.l_whence = SEEK_SET; - arg.l_start = 0; - arg.l_len = 0; /* means to EOF */ - - if (operation & LOCK_NB) - cmd = F_SETLK; - else - cmd = F_SETLKW; /* Blocking */ - - switch (operation & OP_MASK) { - case LOCK_UN: - arg.l_type = F_UNLCK; - code = fcntl(fd, F_SETLK, &arg); - break; - case LOCK_SH: - arg.l_type = F_RDLCK; - code = fcntl(fd, cmd, &arg); - break; - case LOCK_EX: - arg.l_type = F_WRLCK; - code = fcntl(fd, cmd, &arg); - break; - default: - errno = EINVAL; - code = -1; - break; - } - return code; + /* Fall back to POSIX locks */ + (void)ret; /* may be unused depending on #ifdefs above */ + return flock_fcntl(fd, operation, 0); #elif defined(_WIN32) - /* Windows */ + /* Windows */ #define FLOCK_OFFSET_LOW 0 #define FLOCK_OFFSET_HIGH 0 #define FLOCK_LENGTH_LOW 0x00000000 #define FLOCK_LENGTH_HIGH 0x80000000 - HANDLE hFile; - OVERLAPPED ov; - BOOL rv = FALSE; - DWORD f = 0; + HANDLE hFile; + OVERLAPPED ov; + BOOL rv = FALSE; + DWORD f = 0; - hFile = (HANDLE) _get_osfhandle(fd); - if (hFile == NULL || hFile == INVALID_HANDLE_VALUE) { - _set_errno(EBADF); - return -1; - } + hFile = (HANDLE) _get_osfhandle(fd); + if (hFile == NULL || hFile == INVALID_HANDLE_VALUE) { + _set_errno(EBADF); + return -1; + } - ZeroMemory(&ov, sizeof(ov)); - ov.hEvent = NULL; - ov.Offset = FLOCK_OFFSET_LOW; - ov.OffsetHigh = FLOCK_OFFSET_HIGH; + ZeroMemory(&ov, sizeof(ov)); + ov.hEvent = NULL; + ov.Offset = FLOCK_OFFSET_LOW; + ov.OffsetHigh = FLOCK_OFFSET_HIGH; - if (operation & LOCK_NB) - f = LOCKFILE_FAIL_IMMEDIATELY; + if (operation & LOCK_NB) + f = LOCKFILE_FAIL_IMMEDIATELY; - switch (operation & OP_MASK) { - case LOCK_UN: /* Unlock */ - rv = UnlockFileEx(hFile, 0, + switch (operation & OP_MASK) { + case LOCK_UN: /* Unlock */ + rv = UnlockFileEx(hFile, 0, + FLOCK_LENGTH_LOW, FLOCK_LENGTH_HIGH, &ov); + break; + + case LOCK_SH: /* Shared lock */ + rv = LockFileEx(hFile, f, 0, FLOCK_LENGTH_LOW, FLOCK_LENGTH_HIGH, &ov); - break; + break; - case LOCK_SH: /* Shared lock */ - rv = LockFileEx(hFile, f, 0, - FLOCK_LENGTH_LOW, FLOCK_LENGTH_HIGH, &ov); - break; + case LOCK_EX: /* Exclusive lock */ + rv = LockFileEx(hFile, f|LOCKFILE_EXCLUSIVE_LOCK, 0, + FLOCK_LENGTH_LOW, FLOCK_LENGTH_HIGH, + &ov); + break; - case LOCK_EX: /* Exclusive lock */ - rv = LockFileEx(hFile, f|LOCKFILE_EXCLUSIVE_LOCK, 0, - FLOCK_LENGTH_LOW, FLOCK_LENGTH_HIGH, - &ov); - break; + default: + _set_errno(EINVAL); + return -1; + } - default: - _set_errno(EINVAL); - return -1; - } + if (!rv) { + switch (GetLastError()) { + case ERROR_SHARING_VIOLATION: + case ERROR_LOCK_VIOLATION: + case ERROR_IO_PENDING: + _set_errno(EWOULDBLOCK); + break; - if (!rv) { - switch (GetLastError()) { - case ERROR_SHARING_VIOLATION: - case ERROR_LOCK_VIOLATION: - case ERROR_IO_PENDING: - _set_errno(EWOULDBLOCK); - break; + case ERROR_ACCESS_DENIED: + _set_errno(EACCES); + break; - case ERROR_ACCESS_DENIED: - _set_errno(EACCES); - break; + default: + _set_errno(ENOLCK); + } + return -1; + } - default: - _set_errno(ENOLCK); - } - return -1; - } - - return 0; + return 0; #else - return -1; + errno = ENOSYS; + return -1; #endif } - -#endif - diff --git a/lib/roken/roken.h.in b/lib/roken/roken.h.in index 31847ce8b..b64d98631 100644 --- a/lib/roken/roken.h.in +++ b/lib/roken/roken.h.in @@ -881,7 +881,15 @@ ROKEN_LIB_FUNCTION unsigned int ROKEN_LIB_CALL bswap32(unsigned int); ROKEN_LIB_FUNCTION unsigned short ROKEN_LIB_CALL bswap16(unsigned short); #endif -#ifndef HAVE_FLOCK +/* + * rk_flock() - portable flock() implementation + * + * Always use rk_flock() (via the flock macro) instead of calling system + * flock() directly. rk_flock() provides: + * - Prefer OFD locks (F_OFD_SETLK) when available for better thread safety + * - Fallback to system flock() if available + * - Fallback to POSIX fcntl() locks for NFS and other filesystems + */ #ifndef LOCK_SH #define LOCK_SH 1 /* Shared lock */ #endif @@ -895,9 +903,8 @@ ROKEN_LIB_FUNCTION unsigned short ROKEN_LIB_CALL bswap16(unsigned short); #define LOCK_UN 8 /* Unlock */ #endif +ROKEN_LIB_FUNCTION int ROKEN_LIB_CALL rk_flock(int fd, int operation); #define flock(_x,_y) rk_flock(_x,_y) -int rk_flock(int fd, int operation); -#endif /* HAVE_FLOCK */ #ifndef HAVE_DIRFD #ifdef HAVE_DIR_DD_FD