Hi Qian,
sorry for the late reply, somehow I missed this mail. On Sep 5 04:22, Qian Hong wrote: > Dear list, > > When testing Cygwin/MSYS2 on Wine, I found randomly failure of flock(): > https://bugs.wine-staging.com/show_bug.cgi?id=466#c13 > > I ran MSYS2 with Wine+Valgrind, and found warnings like below when > calling flock(): I'd still be glad to stick to upstream Cygwin, but, oh well. > 7 ==19315== Conditional jump or move depends on uninitialised value(s) > 8 ==19315== at 0x7BC82750: RtlGetOwnerSecurityDescriptor (sec.c:740) > 9 ==19315== by 0x7BC9222A: NTDLL_create_struct_sd (sync.c:96) > 10 ==19315== by 0x7BC92CE4: NtCreateEvent (sync.c:294) > 11 ==19315== by 0x6107B687: ??? > 12 ==19315== by 0x612FC347: ??? > > Then I read Cygwin/MSYS2 source code, and found: > > --- snip --- > extern PSECURITY_DESCRIPTOR _everyone_sd (void *buf, ACCESS_MASK access); > #define everyone_sd(access) (_everyone_sd (alloca (SD_MIN_SIZE), (access))) > --- snip --- > > man alloca says: > The alloca() function allocates size bytes of space in the > stack frame of the caller. This temporary space is automatically > freed when > the function that called alloca() returns to its caller. > > However, Cygwin/MSYS2 seems passed a freed pointer to NtCreateEvent: > https://cygwin.com/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/flock.cc;h=2332f5467e37d124acfd12c0f85a30281f10a952;hb=HEAD#l773 > > 638 POBJECT_ATTRIBUTES > 639 lockf_t::create_lock_obj_attr (lockfattr_t *attr, ULONG flags) > 640 { > 641 __small_swprintf (attr->name, LOCK_OBJ_NAME_FMT, > 642 lf_flags & (F_POSIX | F_FLOCK), lf_type, > lf_start, lf_end, > 643 lf_id, lf_wid, lf_ver); > 644 RtlInitCountedUnicodeString (&attr->uname, attr->name, > 645 LOCK_OBJ_NAME_LEN * sizeof (WCHAR)); > 646 InitializeObjectAttributes (&attr->attr, &attr->uname, flags, > lf_inode->i_dir, > 647 everyone_sd (FLOCK_EVENT_ACCESS)); > 648 return &attr->attr; > 649 } > > 772 status = NtCreateEvent (&lf_obj, CYG_EVENT_ACCESS, > 773 create_lock_obj_attr (&attr, OBJ_INHERIT), > 774 NotificationEvent, FALSE); Yuk! You're right. This is certainly a bug. > It seems flock() works very stable on Windows according to my previous > testing, however, I have feeling that as a kernel function, > NtCreateEvent on Windows doesn't have terrible affects to the user > space stack of the process, while Wine implements NtCreateEvent as a > user space function, so the old stack was easier to be destroyed. > > I write a hack as attachment 0001-cygwin-flock-user-static-buffer.txt > and recompile MSYS2, then the bug seems go away. Right, but you can't use a static buffer here. > Could someone confirm whether there is a potential Cygwin bug? If true > I'd love to leave the bug for Cygwin devs to write a fix. Please try the attached patch. Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat
From 51d38004b2f51ac659f7ccc663c222f5ffe24b80 Mon Sep 17 00:00:00 2001
From: Corinna Vinschen <cori...@vinschen.de>
Date: Tue, 8 Sep 2015 10:57:54 +0200
Subject: [PATCH] flock.cc: Fix stack allocation from callee used in caller
* flock.cc (lockf_t::create_lock_obj_attr): Add buffer parameter.
Call _everyone_sd with buffer argument from caller rather than
everyone_sd with locally allocated stack buffer.
(lockf_t::create_lock_obj): Call create_lock_obj_attr only once
outside the loop and with additional buffer argument.
(lockf_t::open_lock_obj): Call create_lock_obj_attr with additional
buffer argument.
---
winsup/cygwin/flock.cc | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/winsup/cygwin/flock.cc b/winsup/cygwin/flock.cc
index 2332f54..f26a76a 100644
--- a/winsup/cygwin/flock.cc
+++ b/winsup/cygwin/flock.cc
@@ -290,7 +290,7 @@ class lockf_t
{ cfree (p); }
POBJECT_ATTRIBUTES create_lock_obj_attr (lockfattr_t *attr,
- ULONG flags);
+ ULONG flags, void *sd_buf);
void create_lock_obj ();
bool open_lock_obj ();
@@ -636,7 +636,7 @@ inode_t::get_all_locks_list ()
/* Create the lock object name. The name is constructed from the lock
properties which identify it uniquely, all values in hex. */
POBJECT_ATTRIBUTES
-lockf_t::create_lock_obj_attr (lockfattr_t *attr, ULONG flags)
+lockf_t::create_lock_obj_attr (lockfattr_t *attr, ULONG flags, void *sd_buf)
{
__small_swprintf (attr->name, LOCK_OBJ_NAME_FMT,
lf_flags & (F_POSIX | F_FLOCK), lf_type, lf_start, lf_end,
@@ -644,7 +644,7 @@ lockf_t::create_lock_obj_attr (lockfattr_t *attr, ULONG
flags)
RtlInitCountedUnicodeString (&attr->uname, attr->name,
LOCK_OBJ_NAME_LEN * sizeof (WCHAR));
InitializeObjectAttributes (&attr->attr, &attr->uname, flags,
lf_inode->i_dir,
- everyone_sd (FLOCK_EVENT_ACCESS));
+ _everyone_sd (sd_buf, FLOCK_EVENT_ACCESS));
return &attr->attr;
}
@@ -766,11 +766,13 @@ lockf_t::create_lock_obj ()
{
lockfattr_t attr;
NTSTATUS status;
+ POBJECT_ATTRIBUTES lock_obj_attr;
+ lock_obj_attr = create_lock_obj_attr (&attr, OBJ_INHERIT,
+ alloca (SD_MIN_SIZE));
do
{
- status = NtCreateEvent (&lf_obj, CYG_EVENT_ACCESS,
- create_lock_obj_attr (&attr, OBJ_INHERIT),
+ status = NtCreateEvent (&lf_obj, CYG_EVENT_ACCESS, lock_obj_attr,
NotificationEvent, FALSE);
if (!NT_SUCCESS (status))
{
@@ -852,7 +854,7 @@ lockf_t::open_lock_obj ()
NTSTATUS status;
status = NtOpenEvent (&lf_obj, FLOCK_EVENT_ACCESS,
- create_lock_obj_attr (&attr, 0));
+ create_lock_obj_attr (&attr, 0, alloca (SD_MIN_SIZE)));
if (!NT_SUCCESS (status))
{
SetLastError (RtlNtStatusToDosError (status));
--
2.1.0
pgp1Iu5RaaU8c.pgp
Description: PGP signature
