On Mon, 14 Oct 2024 15:36:02 +0200
Christian Franke wrote:
> Two possibly independent bugs found by 'stress-ng --lockf ...':
>
> 1) lockf() may abort the process with api_fatal() if the new range
> partly overlaps with two ranges previously locked by the same process.
>
> 2) lockf() prints a warning on too many locks and returns success. It
> should not print a warning and fail with ENOLCK instead.
>
> Testcase for both:
>
> $ uname -r
> 3.5.4-1.x86_64
>
> $ cat locktest.c
> #include <fcntl.h>
> #include <stdio.h>
> #include <unistd.h>
>
> static int lock_at(int fd, int off, int size)
> {
> if (lseek(fd, off, SEEK_SET) < 0) {
> perror("lseek"); return -1;
> }
> printf("\rlock %d-%d\n", off, off + size - 1); fflush(stdout);
> if (lockf(fd, F_LOCK, size) < 0) {
> perror("lock"); return -1;
> }
> return 0;
> }
>
> int main(int argc, char **argv)
> {
> int fd = open("locktest.tmp", O_RDWR|O_CREAT, 0666);
> if (fd < 0) {
> perror("open"); return 1;
> }
>
> if (argc == 1) {
> lock_at(fd, 0, 2);
> lock_at(fd, 2, 2);
> lock_at(fd, 1, 2);
> }
> else {
> for (int i = 0; i < 914; i++)
> if (lock_at(fd, i, 1))
> return 1;
> }
> printf("\rdone\n");
> return 0;
> }
>
> $ gcc -o locktest locktest.c
>
> $ ./locktest
> lock 0-1
> lock 2-3
> lock 1-2
> 1 [main] locktest 44864 C:\cygwin64\tmp\locktest.exe: \
> *** fatal error - NtCreateEvent(lock): 0xC0000035\
> Hangup
>
> $ ./locktest loop
> lock 0-0
> lock 1-1
> lock 2-2
> lock 3-3
> ...
> lock 909-909
> lock 910-910
> lock 911-911
> 0 [main] locktest 44865 inode_t::get_all_locks_list: \
> Warning, can't handle more than 910 locks per file.
> lock 912-912
> 727 [main] locktest 44865 inode_t::get_all_locks_list: \
> Warning, can't handle more than 910 locks per file.
> lock 913-913
> 1329 [main] locktest 44865 inode_t::get_all_locks_list: \
> Warning, can't handle more than 910 locks per file.
> done
>
> There is possibly also an off-by-one error as the 912'th lockf() prints
> the first warning.
Thanks for the report.
I looked into the problems, and considered how to fix them.
Could you please try the experimental patch attached?
--
Takashi Yano <takashi.y...@nifty.ne.jp>
diff --git a/winsup/cygwin/flock.cc b/winsup/cygwin/flock.cc
index 0f1efa01d..f99b7f538 100644
--- a/winsup/cygwin/flock.cc
+++ b/winsup/cygwin/flock.cc
@@ -297,6 +297,7 @@ class inode_t
HANDLE i_dir;
HANDLE i_mtx;
uint32_t i_cnt; /* # of threads referencing this instance. */
+ uint32_t lock_cnt;
public:
inode_t (dev_t dev, ino_t ino);
@@ -321,6 +322,7 @@ class inode_t
void unlock_and_remove_if_unused ();
lockf_t *get_all_locks_list ();
+ uint32_t get_lock_count () { return lock_cnt; };
bool del_my_locks (long long id, HANDLE fhdl);
};
@@ -503,7 +505,8 @@ 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 (NULL), i_dev (dev), i_ino (ino), i_cnt (0L),
+ lock_cnt (0)
{
HANDLE parent_dir;
WCHAR name[48];
@@ -586,6 +589,7 @@ inode_t::get_all_locks_list ()
BOOLEAN restart = TRUE;
bool last_run = false;
lockf_t newlock, *lock = i_all_lf;
+ int cnt = 0;
PDIRECTORY_BASIC_INFORMATION dbi_buf = (PDIRECTORY_BASIC_INFORMATION)
tp.w_get ();
@@ -610,17 +614,16 @@ inode_t::get_all_locks_list ()
dbi->ObjectName.Buffer[LOCK_OBJ_NAME_LEN] = L'\0';
if (!newlock.from_obj_name (this, &i_all_lf, dbi->ObjectName.Buffer))
continue;
- if (lock - i_all_lf >= MAX_LOCKF_CNT)
- {
- system_printf ("Warning, can't handle more than %d locks per
file.",
- MAX_LOCKF_CNT);
- break;
- }
+ if (lock - i_all_lf > MAX_LOCKF_CNT)
+ api_fatal ("Can't handle more than %d locks per file.",
+ MAX_LOCKF_CNT);
if (lock > i_all_lf)
lock[-1].lf_next = lock;
new (lock++) lockf_t (newlock);
+ cnt++;
}
}
+ lock_cnt = cnt;
/* If no lock has been found, return NULL. */
if (lock == i_all_lf)
return NULL;
@@ -1349,6 +1352,10 @@ lf_setlock (lockf_t *lock, inode_t *node, lockf_t
**clean, HANDLE fhdl)
prev = head;
block = *head;
needtolink = 1;
+
+ if (node->get_lock_count () + 2 > MAX_LOCKF_CNT)
+ return ENOLCK;
+
for (;;)
{
ovcase = lf_findoverlap (block, lock, SELF, &prev, &overlap);
@@ -1446,23 +1453,22 @@ lf_setlock (lockf_t *lock, inode_t *node, lockf_t
**clean, HANDLE fhdl)
prev = &lock->lf_next;
lf_wakelock (overlap, fhdl);
overlap->create_lock_obj ();
- lock->create_lock_obj ();
- needtolink = 0;
continue;
case 5: /* overlap ends after lock */
/*
* Add the new lock before overlap.
*/
- if (needtolink) {
+ if (needtolink)
+ {
*prev = lock;
lock->lf_next = overlap;
- }
+ needtolink = 0;
+ }
overlap->lf_start = lock->lf_end + 1;
lf_wakelock (overlap, fhdl);
- lock->create_lock_obj ();
overlap->create_lock_obj ();
- break;
+ continue;
}
break;
}
--
Problem reports: https://cygwin.com/problems.html
FAQ: https://cygwin.com/faq/
Documentation: https://cygwin.com/docs.html
Unsubscribe info: https://cygwin.com/ml/#unsubscribe-simple
