Re: Question about flock - potential memory corruption?

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

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

Attachment: 0001-flock.cc-Fix-stack-allocation-from-callee-used-in-ca.patch
Description: Text document

Attachment: pgpcbflMfocDK.pgp
Description: PGP signature