Proposal to use ThreadQuerySetWin32StartAddress inside munge_threadfunc (Cygwin randomly crashes on Wine)

[Corinna Vinschen]

Hi Qian,

On Oct  2 21:27, Qian Hong wrote:
> Dear Cygwin developers,
> 
> 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.
> 
> 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.
> 
> 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 ()
 
   if (threadfunc_ix[0])
     {
-      char *threadfunc = ebp[threadfunc_ix[0]];
+      char *threadfunc = NULL;
+
+      NtQueryInformationThread (NtCurrentThread (),
+				ThreadQuerySetWin32StartAddress,
+				&threadfunc, sizeof threadfunc, NULL);
       if (!search_for || threadfunc == search_for)
 	{
 	  search_for = NULL;
 	  for (i = 0; threadfunc_ix[i]; i++)
-	    ebp[threadfunc_ix[i]] = (char *) threadfunc_fe;
+	    if (!threadfunc || ebp[threadfunc_ix[i]] == threadfunc)
+	       ebp[threadfunc_ix[i]] = (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 = 0,
   ThreadTimes = 1,
-  ThreadImpersonationToken = 5
+  ThreadImpersonationToken = 5,
+  ThreadQuerySetWin32StartAddress = 9
 } THREADINFOCLASS, *PTHREADINFOCLASS;
 
 /* Checked on 64 bit. */

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin/attachments/20151021/d598e0a1/attachment.sig>