https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=8f551af4fcd195f28c1bd33fb476787f6b17da15
commit 8f551af4fcd195f28c1bd33fb476787f6b17da15 Author: Takashi Yano <[email protected]> AuthorDate: Tue Nov 25 15:43:11 2025 +0100 Commit: Corinna Vinschen <[email protected]> CommitDate: Wed Nov 26 12:04:37 2025 +0100 Cygwin: flock: allocate i_all_lf as static array Previously, variable i_all_lf was allocated and released in several functions: lf_setlock(), lf_clearlock(), and lf_getlock(), and was used only temporarily as noted in flock.cc. This pattern easily leads to bugs like those that occurred in flock.cc, such as: lf_setlock() lf_clearlock() | . i_all_lf = tp.w_get() . | . +---------------------->+ | i_all_lf = tp.wget() | do something | (release i_all_lf implicitly) | +<----------------------+ | . accessing i_all_lf (may destroy tmp_pathbuf area) | . The similar problem also happens in multi-thread case. Fix and prevent the bugs by converting i_all_lf to a pointer pointing to a static array in the inode_t class. Given inodes are allocated on the cygheap, reduce MAX_LOCKF_CNT by one so an inode_t fits into a 64K chunk. Avoid accessing the all locks list outside of locking and move get_all_locks_list() call in lf_clearlock() to fhandler_base::lock() to avoid calling the function twice. Addresses: https://cygwin.com/pipermail/cygwin/2025-October/258914.html Fixes: ae181b0ff122 ("Cygwin: lockf: Make lockf() return ENOLCK when too many locks") Reported-by: Nahor <[email protected]> Co-authored-by: Corinna Vinschen <[email protected]> Signed-off-by: Takashi Yano <[email protected]> Signed-off-by: Corinna Vinschen <[email protected]> Diff: --- winsup/cygwin/flock.cc | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/winsup/cygwin/flock.cc b/winsup/cygwin/flock.cc index e03caba27a8e..5c60f628b397 100644 --- a/winsup/cygwin/flock.cc +++ b/winsup/cygwin/flock.cc @@ -264,7 +264,6 @@ class lockf_t /* Used to store own lock list in the cygheap. */ void *operator new (size_t size) { return cmalloc (HEAP_FHANDLER, sizeof (lockf_t)); } - /* Never call on node->i_all_lf! */ void operator delete (void *p) { cfree (p); } @@ -277,6 +276,10 @@ class lockf_t void del_lock_obj (HANDLE fhdl, bool signal = false); }; +/* Number of lockf_t structs which fit in the temporary buffer. */ +#define MAX_LOCKF_CNT ((intptr_t)((NT_MAX_PATH * sizeof (WCHAR)) \ + / sizeof (lockf_t)) - 1) + /* Per inode_t class */ class inode_t { @@ -285,7 +288,8 @@ class inode_t public: LIST_ENTRY (inode_t) i_next; lockf_t *i_lockf; /* List of locks of this process. */ - lockf_t *i_all_lf; /* Temp list of all locks for this file. */ + /* list of all locks for this file. */ + lockf_t *i_all_lf; dev_t i_dev; /* Device ID */ ino_t i_ino; /* inode number */ @@ -295,6 +299,7 @@ class inode_t HANDLE i_mtx; uint32_t i_cnt; /* # of threads referencing this instance. */ uint32_t i_lock_cnt; /* # of locks for this file */ + lockf_t i_all[MAX_LOCKF_CNT]; public: inode_t (dev_t dev, ino_t ino); @@ -503,7 +508,7 @@ inode_t::get (dev_t dev, ino_t ino, bool create_if_missing, bool lock) } inode_t::inode_t (dev_t dev, ino_t ino) -: i_lockf (NULL), i_all_lf (NULL), i_dev (dev), i_ino (ino), i_cnt (0L), +: i_lockf (NULL), i_all_lf (i_all), i_dev (dev), i_ino (ino), i_cnt (0L), i_lock_cnt (0) { HANDLE parent_dir; @@ -535,10 +540,6 @@ inode_t::inode_t (dev_t dev, ino_t ino) list in the i_all_lf member. This list is searched in lf_getblock for locks which potentially block our lock request. */ -/* Number of lockf_t structs which fit in the temporary buffer. */ -#define MAX_LOCKF_CNT ((intptr_t)((NT_MAX_PATH * sizeof (WCHAR)) \ - / sizeof (lockf_t))) - bool lockf_t::from_obj_name (inode_t *node, lockf_t **head, const wchar_t *name) { @@ -1157,6 +1158,11 @@ restart: /* Entry point after a restartable signal came in. */ break; case F_UNLCK: + /* lf_clearlock() is called from here as well as from lf_setlock(). + lf_setlock() already calls get_all_locks_list(), so we don't call it + from lf_clearlock() but rather here to avoid calling the (potentially + timeconsuming) function twice if called through lf_setlock(). */ + node->get_all_locks_list (); error = lf_clearlock (lock, &clean, get_handle ()); lock->lf_next = clean; clean = lock; @@ -1218,7 +1224,6 @@ lf_setlock (lockf_t *lock, inode_t *node, lockf_t **clean, HANDLE fhdl) lockf_t **head = lock->lf_head; lockf_t **prev, *overlap; int ovcase, priority, old_prio, needtolink; - tmp_pathbuf tp; /* * Set the priority @@ -1229,8 +1234,6 @@ lf_setlock (lockf_t *lock, inode_t *node, lockf_t **clean, HANDLE fhdl) /* * Scan lock list for this file looking for locks that would block us. */ - /* Create temporary space for the all locks list. */ - node->i_all_lf = (lockf_t *) (void *) tp.w_get (); while ((block = lf_getblock(lock, node))) { HANDLE obj = block->lf_obj; @@ -1282,6 +1285,10 @@ lf_setlock (lockf_t *lock, inode_t *node, lockf_t **clean, HANDLE fhdl) lock->lf_type = F_WRLCK; } + /* Copy Windows PID to local var so we don't access the all locks + list outside of locking. */ + DWORD lf_wid = block->lf_wid; + /* * Add our lock to the blocked list and sleep until we're free. * Remember who blocked us (for deadlock detection). @@ -1298,7 +1305,7 @@ lf_setlock (lockf_t *lock, inode_t *node, lockf_t **clean, HANDLE fhdl) HANDLE proc = NULL; if (lock->lf_flags & F_POSIX) { - proc = OpenProcess (SYNCHRONIZE, FALSE, block->lf_wid); + proc = OpenProcess (SYNCHRONIZE, FALSE, lf_wid); if (!proc) timeout = 0L; else @@ -1543,9 +1550,6 @@ lf_clearlock (lockf_t *unlock, lockf_t **clean, HANDLE fhdl) return 0; inode_t *node = lf->lf_inode; - tmp_pathbuf tp; - node->i_all_lf = (lockf_t *) tp.w_get (); - node->get_all_locks_list (); /* Update lock count */ uint32_t lock_cnt = node->get_lock_count (); bool first_loop = true; @@ -1631,10 +1635,7 @@ static int lf_getlock (lockf_t *lock, inode_t *node, struct flock *fl) { lockf_t *block; - tmp_pathbuf tp; - /* Create temporary space for the all locks list. */ - node->i_all_lf = (lockf_t *) (void * ) tp.w_get (); if ((block = lf_getblock (lock, node))) { if (block->lf_obj)
