From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11420 invoked by alias); 8 Sep 2015 08:58:59 -0000 Mailing-List: contact cygwin-help@cygwin.com; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: cygwin-owner@cygwin.com Mail-Followup-To: cygwin@cygwin.com Received: (qmail 11400 invoked by uid 89); 8 Sep 2015 08:58:59 -0000 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 Received: from aquarius.hirmke.de (HELO calimero.vinschen.de) (217.91.18.234) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 08 Sep 2015 08:58:57 +0000 Received: by calimero.vinschen.de (Postfix, from userid 500) id E6DB3A80858; Tue, 8 Sep 2015 10:58:54 +0200 (CEST) Date: Tue, 08 Sep 2015 08:58:00 -0000 From: Corinna Vinschen To: cygwin@cygwin.com Subject: Re: Question about flock - potential memory corruption? Message-ID: <20150908085854.GB24811@calimero.vinschen.de> Reply-To: cygwin@cygwin.com Mail-Followup-To: cygwin@cygwin.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) X-SW-Source: 2015-09/txt/msg00122.txt.bz2 --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 Content-length: 3297 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 Content-length: 2821 =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 Content-length: 819 -----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--