X-Recipient: archive-cygwin AT delorie DOT com DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:subject:message-id:reply-to :references:mime-version:content-type:in-reply-to; q=dns; s= default; b=XNm/0De5OqCwe+IFqQ7g5jH8bWv7C8GhMnXRtMIkA638AWLx7semr NjVRx1AFzMSclCz9W8sOEgaQUN+ZRnfViGXwO2Qcef1xEfZ79fI9TGd0tMG2ssnz Yl/unQehIlaQIsvXh+PIZuA+Ipfu7+bVZxpt4ON40AFnEB75t0wzTU= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:subject:message-id:reply-to :references:mime-version:content-type:in-reply-to; s=default; bh=8olV/JJxGoaFuJenHAw+34DGu3U=; b=SpFKH+A2gzZCDybSQJupa8G7UYJp VyF0GYWGxgSABkcBPhKC5ZdpqP7WjJM5EpyaSe8cbq0r3uxXZt80dYIORQV5Y+kb Haj5DEd8AkQ/GuwyhKitEvtO4ydtoy0amY+zBUZQheefXsv65f9cnXdO72xvINHg ra7J3F5i2uloP78= Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: cygwin-owner AT cygwin DOT com Mail-Followup-To: cygwin AT cygwin DOT com Delivered-To: mailing list cygwin AT cygwin DOT com Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.5 required=5.0 tests=AWL,BAYES_20,KAM_LAZY_DOMAIN_SECURITY autolearn=no version=3.3.2 X-HELO: calimero.vinschen.de Date: Tue, 8 Sep 2015 10:58:54 +0200 From: Corinna Vinschen To: cygwin AT cygwin DOT com Subject: Re: Question about flock - potential memory corruption? Message-ID: <20150908085854.GB24811@calimero.vinschen.de> Reply-To: cygwin AT cygwin DOT com Mail-Followup-To: cygwin AT cygwin DOT com References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="L6iaP+gRLNZHKoI4" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) --L6iaP+gRLNZHKoI4 Content-Type: multipart/mixed; boundary="z6Eq5LdranGa6ru8" Content-Disposition: inline --z6Eq5LdranGa6ru8 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Qian, sorry for the late reply, somehow I missed this mail. On Sep 5 04:22, Qian Hong wrote: > Dear list, >=20 > When testing Cygwin/MSYS2 on Wine, I found randomly failure of flock(): > https://bugs.wine-staging.com/show_bug.cgi?id=3D466#c13 >=20 > 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 =3D=3D19315=3D=3D Conditional jump or move depends on uninitialised v= alue(s) > 8 =3D=3D19315=3D=3D at 0x7BC82750: RtlGetOwnerSecurityDescriptor (se= c.c:740) > 9 =3D=3D19315=3D=3D by 0x7BC9222A: NTDLL_create_struct_sd (sync.c:96) > 10 =3D=3D19315=3D=3D by 0x7BC92CE4: NtCreateEvent (sync.c:294) > 11 =3D=3D19315=3D=3D by 0x6107B687: ??? > 12 =3D=3D19315=3D=3D by 0x612FC347: ??? >=20 > Then I read Cygwin/MSYS2 source code, and found: >=20 > --- snip --- > extern PSECURITY_DESCRIPTOR _everyone_sd (void *buf, ACCESS_MASK access); > #define everyone_sd(access) (_everyone_sd (alloca (SD_MIN_SIZE), (access)= )) > --- snip --- >=20 > 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. >=20 > However, Cygwin/MSYS2 seems passed a freed pointer to NtCreateEvent: > https://cygwin.com/git/gitweb.cgi?p=3Dnewlib-cygwin.git;a=3Dblob;f=3Dwins= up/cygwin/flock.cc;h=3D2332f5467e37d124acfd12c0f85a30281f10a952;hb=3DHEAD#l= 773 >=20 > 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 } >=20 > 772 status =3D NtCreateEvent (&lf_obj, CYG_EVENT_ACCESS, > 773 create_lock_obj_attr (&attr, OBJ_INHER= IT), > 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. >=20 > 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 --=20 Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat --z6Eq5LdranGa6ru8 Content-Type: text/plain; charset=utf-8 Content-Disposition: attachment; filename="0001-flock.cc-Fix-stack-allocation-from-callee-used-in-ca.patch" Content-Transfer-Encoding: quoted-printable =46rom 51d38004b2f51ac659f7ccc663c222f5ffe24b80 Mon Sep 17 00:00:00 2001 From: Corinna Vinschen 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); } =20 POBJECT_ATTRIBUTES create_lock_obj_attr (lockfattr_t *attr, - ULONG flags); + ULONG flags, void *sd_buf); =20 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_bu= f) { __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; } =20 @@ -766,11 +766,13 @@ lockf_t::create_lock_obj () { lockfattr_t attr; NTSTATUS status; + POBJECT_ATTRIBUTES lock_obj_attr; =20 + lock_obj_attr =3D create_lock_obj_attr (&attr, OBJ_INHERIT, + alloca (SD_MIN_SIZE)); do { - status =3D NtCreateEvent (&lf_obj, CYG_EVENT_ACCESS, - create_lock_obj_attr (&attr, OBJ_INHERIT), + status =3D 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; =20 status =3D 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)); --=20 2.1.0 --z6Eq5LdranGa6ru8-- --L6iaP+gRLNZHKoI4 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJV7qNOAAoJEPU2Bp2uRE+gDIwP/3/7qngEXk24Sl8WNBmO71e7 icrLINR2QV6gaLJCmmiUmgwCA0WsteoIfz8qHshrC9R58J+9ZJvZ/5a0++8Uinvb hiMLONJ3JljqN2eaQbNF5RzxCp3h2cChbkkm7GMD6cDtW5LfbcnQuPWE22gNdD4w AtpFO3pP5V8CaTLwVj+1qKZwBrJRzviaPHoMQScw0R6CwiH/KLWGX2RYH7YR9PIr kLbMM+EzVdX2LXnfwGN05uPevE6DbylhBNR9IIIIQDNXP4G2c2iaygqKmBTnNU8v Y2FyNHgGknlT/az7OWQWBGNYrwm/XtLVQQVIfnc2CrhkzMN1TKb291AjtFoVRRGZ sVGdHrEM6NsPRzpgVI9qE6CcNIUZG5ajjd+fzYKHeZUSE5DLSShIuqcKFHJmQea9 WikEoIHhomXGKgDh1aKQQnCcjRNuoDy0nuyaxyUKrDk6uejRcOW1xB6jIB3D+63z rbrv3Tr7USd9bqmoZ9jJ2FBCBkfbzrcA4pFqk/qXeJDRJnWPs/9W4D2N3Z68X4xH Of5Ize3Dck0X25qGk3AoETS3NPEImKqoCNZFfJhRn1nq4IuEGzD48pgC2c3V910T lj455Agh5b2+4xMbcRjqX9gOVHDJHh9sIjvfXQ5IuzZBYfC/EPAY+RRoD56KrJH3 ly15EdQuV9e9DHLBNw2r =pzTG -----END PGP SIGNATURE----- --L6iaP+gRLNZHKoI4--