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=RrwMn5dijkraezBeLTgTgamxZDF4rq6qsNvD/Knv3BcA7LPJmcDVG
	kP8sYbX+DAe9zmXmnAIwmhVMVl2y9tjmxbaCuZ4r+ixhu0JCoWa2qkkk09bguGxs
	858Jlz7i9DNV285+JBh27FMKW08TuFXpqTme87/Cqqk/+INfcdo5xQ=
Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm
Sender: cygwin-owner AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
Date: Wed, 21 Oct 2015 10:17:11 +0200
From: Corinna Vinschen <corinna-cygwin AT cygwin DOT com>
To: cygwin AT cygwin DOT com
Subject: Re: Proposal to use ThreadQuerySetWin32StartAddress inside munge_threadfunc (Cygwin randomly crashes on Wine)
Message-ID: <20151021081711.GJ5319@calimero.vinschen.de>
Reply-To: cygwin AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
References: <CALd+sZSPx-bUuq6FQ42vzWpXgGRhuakML87J-N3Wg+3Q0HSFCw AT mail DOT gmail DOT com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;	protocol="application/pgp-signature"; boundary="uJWb33pM2TcUAXIl"
Content-Disposition: inline
In-Reply-To: <CALd+sZSPx-bUuq6FQ42vzWpXgGRhuakML87J-N3Wg+3Q0HSFCw@mail.gmail.com>
User-Agent: Mutt/1.5.23 (2014-03-12)

--uJWb33pM2TcUAXIl
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi Qian,

On Oct  2 21:27, Qian Hong wrote:
> Dear Cygwin developers,
>=20
> While testing Cygwin on Wine, there is a random crashing puzzled me
> for a wrong time.
> [...]
> Alternative, we also tried a hack in the Cygwin side, which use
> ThreadQuerySetWin32StartAddress to query the thread entry point, as
> 0001-hack-use-ThreadQuerySetWin32StartAddress.txt show. I tested this
> hack with recent Cygwin git repo and confirming it works for me
> (without hack from Wine side). I also tested my own cygwin build with
> this hack on Windows to confirm it doesn't break things.
>=20
> Is the proposal way accepted by Cygwin? I understand we hate changing
> working code (on Windows), but using ThreadQuerySetWin32StartAddress
> seems like an improvement than rely on searching result from stack
> memory. If we could discuss a solution which makes both Cygwin and
> Wine happy that would be great.
>=20
> MSDN says, "Note that on versions of Windows prior to Windows Vista,
> the returned start address is only reliable before the thread starts
> running.". Actually I tested my build on Windows XP sp2 and it works
> for me. Additional, since Cygwin is moving to the end of Windows XP
> support, maybe we are at the right time to do this change.

Thanks for this patch.  I tried it on Windows 10 under 64 bit and it
works fine, too.  While the existing code seems to work on Windows as
desired, it's not really a safe bet, so calling NtQueryInformationThread
to get the correct thread start address seems like a nice improvement.

Sidenote: During testing it occured to me that it would be even better
if we could just call NtSetInformationThread setting the entry point to
threadfunc_fe dropping the rather hackish stack walk entirely.  Alas,
apparently this functionality, calling NtSetInformationThread with the
ThreadQuerySetWin32StartAddress information class, has been
intentionally broken starting with Windows Vista :-P

I simplified your patch, taking out the printf's and added a test in the
loop in case NtQueryInformationThread failed.  See below.  It's not
overly large, but it introduces new functionality.  It would be nice if
you could sign a copyright assignment and send it as PDF.  Please see
https://cygwin.com/assign.txt.


Thanks,
Corinna


diff --git a/winsup/cygwin/init.cc b/winsup/cygwin/init.cc
index 56d4668..69e66a0 100644
--- a/winsup/cygwin/init.cc
+++ b/winsup/cygwin/init.cc
@@ -55,12 +55,17 @@ munge_threadfunc ()
=20
   if (threadfunc_ix[0])
     {
-      char *threadfunc =3D ebp[threadfunc_ix[0]];
+      char *threadfunc =3D NULL;
+
+      NtQueryInformationThread (NtCurrentThread (),
+				ThreadQuerySetWin32StartAddress,
+				&threadfunc, sizeof threadfunc, NULL);
       if (!search_for || threadfunc =3D=3D search_for)
 	{
 	  search_for =3D NULL;
 	  for (i =3D 0; threadfunc_ix[i]; i++)
-	    ebp[threadfunc_ix[i]] =3D (char *) threadfunc_fe;
+	    if (!threadfunc || ebp[threadfunc_ix[i]] =3D=3D threadfunc)
+	       ebp[threadfunc_ix[i]] =3D (char *) threadfunc_fe;
 	  TlsSetValue (_my_oldfunc, threadfunc);
 	}
     }
diff --git a/winsup/cygwin/ntdll.h b/winsup/cygwin/ntdll.h
index 13a131d..050e848 100644
--- a/winsup/cygwin/ntdll.h
+++ b/winsup/cygwin/ntdll.h
@@ -1162,7 +1162,8 @@ typedef enum _THREADINFOCLASS
 {
   ThreadBasicInformation =3D 0,
   ThreadTimes =3D 1,
-  ThreadImpersonationToken =3D 5
+  ThreadImpersonationToken =3D 5,
+  ThreadQuerySetWin32StartAddress =3D 9
 } THREADINFOCLASS, *PTHREADINFOCLASS;
=20
 /* Checked on 64 bit. */

--=20
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

--uJWb33pM2TcUAXIl
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJWJ0oHAAoJEPU2Bp2uRE+gXIkQAIKN/BsxVDgDj1d3LRlQu43G
Js0GIHJr5KaU97Y1cBt1nsfQAb6XJiDj37Dl4Ja7+6fLasm1lNog6TMVz+A95cpB
XhV+0XpM6DpXt0OGvsu+vEoqD8JkngO+TT55W8kKGUwoZBp4loEXSFeBb5AW35D9
P4jttnohGqLm+7v7x7lmr+bla778X2UWVJjLHEyMWaj+bO+8AXQMIA16BqW3ewM/
drq9Md3DGoO7sA876M/fI77pFMVZymKm+tUYNf9d5x1WcIgBoZsh5aM0LD2FCunD
NOsTQnbQ88utLI4IltRR/DYc1VOXgslaDD75CSSgN6GejhhrW2BIYjuO8Fn/Ok4b
hIdZYduP7302in1z1BmmAQUUitxSu1+UzwSh64mbWknnjO7Vg7Bw2NuPqfa2MTsw
jWpT/pH8E2aMntoyFT9LucRf7xQg3QQk8owgDUgyY6wYwRdDW5KYptQy4vRJCgPO
K9uh18ljwRvecg1EuYLQIkvruY77yog4I01KUvV6Xa6s6vwMHquGAqpA+bQArdTM
dPfILYDpl1WyWZhs3midDrCg68GrYdijDvHixJvXLRV3l5l2DNNQAD887cHzKn1O
qBjcjNMvmOoc+RF8IJ0kAkSZzmUCSk5rWwxW2HM4+73ehywBJcEKE5rZVTBaNrdF
UVpE4kJifdzk5dYQoogW
=aUt4
-----END PGP SIGNATURE-----

--uJWb33pM2TcUAXIl--