Igor Polyakov fixed a rather nasty problem with the threaded name resolver

for Windows, that could lead to an Access Violation when the multi interface
was used due to an issue with how the resolver thread was and was not
terminated.

3 files changed, 200 insertions, 35 deletions

diff --git a/CHANGES b/CHANGES
index 868edcbf3..556c2155c 100644
--- a/CHANGES
+++ b/CHANGES
@@ -8,6 +8,11 @@
 
 
 Daniel (29 August 2005)
+- Igor Polyakov fixed a rather nasty problem with the threaded name resolver
+  for Windows, that could lead to an Access Violation when the multi interface
+  was used due to an issue with how the resolver thread was and was not
+  terminated.
+
 - Simon Josefsson brought a patch that allows curl to get built to use GNU GSS
   instead of MIT/Heimdal for GSS capabilities.
 
@@ -18,7 +23,7 @@ Daniel (24 August 2005)
   still having problems serving files larger than 2 or 4 GB. When this option
   is enabled, curl will simply have to wait for the server to close the
   connection to signal end of transfer. I wrote test case 269 that runs a
-  simple test that this works.
+  simple test to verify that this works.
 
 - (Trying hard to exclude emotions now.) valgrind version 3 suddenly renamed
   the --logfile command line option to --log-file, and thus the test script
@@ -26,7 +31,8 @@ Daniel (24 August 2005)
   alters the valgrind command line accordingly.
 
 - Fixed CA cert verification using GnuTLS with the default bundle, which
-  previously failed due to GnuTLS not allowing x509 v1 CA certs by default.     
+  previously failed due to GnuTLS not allowing x509 v1 CA certs by default.
+  Ralph Mitchell reported.
 
 Daniel (19 August 2005)
 - Norbert Novotny had problems with FTPS and he helped me work out a patch
diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index d346f0999..253a5e937 100644
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -21,6 +21,7 @@ This release includes the following changes:
 
 This release includes the following bugfixes:
 
+ o windows threaded resolver access violation with multi interface
  o test suite works with valgrind 3
  o CA cert verification with GnuTLS builds
  o handles expiry times in cookie files that go beyond 32 bits in size
@@ -66,6 +67,6 @@ advice from friends like these:
  Tupone Alfredo, Gisle Vanem, David Shaw, Andrew Bushnell, Dan Fandrich,
  Adrian Schuur, Diego Casorran, Peteris Krumins, Jon Grubbs, Christopher
  R. Palmer, Mario Schroeder, Richard Clayton, James Bursa, Jeff Pohlmeyer,
- Norbert Novotny, Toby Peterson, Simon Josefsson
+ Norbert Novotny, Toby Peterson, Simon Josefsson, Igor Polyakov
 
         Thanks! (and sorry if I forgot to mention someone)
diff --git a/lib/hostthre.c b/lib/hostthre.c
index 070eb22ff..7687038f2 100644
--- a/lib/hostthre.c
+++ b/lib/hostthre.c
@@ -157,11 +157,119 @@ struct thread_data {
   FILE *stderr_file;
   HANDLE mutex_waiting;  /* marks that we are still waiting for a resolve */
   HANDLE event_resolved; /* marks that the thread obtained the information */
+  HANDLE event_thread_started; /* marks that the thread has initialized and
+                                  started */
+  HANDLE mutex_terminate; /* serializes access to flag_terminate */
+  HANDLE event_terminate; /* flag for thread to terminate instead of calling
+                             callbacks */
 #ifdef CURLRES_IPV6
   struct addrinfo hints;
 #endif
 };
 
+/* Data for synchronization between resolver thread and its parent */
+struct thread_sync_data {
+  HANDLE mutex_waiting;   /* thread_data.mutex_waiting duplicate */
+  HANDLE mutex_terminate; /* thread_data.mutex_terminate duplicate */
+  HANDLE event_terminate; /* thread_data.event_terminate duplicate */
+  char * hostname;        /* hostname to resolve, Curl_async.hostname
+                             duplicate */
+};
+
+/* Destroy resolver thread synchronization data */
+void destroy_thread_sync_data(struct thread_sync_data * tsd)
+{
+  if (tsd->hostname) {
+    free(tsd->hostname);
+    tsd->hostname = NULL;
+  }
+  if (tsd->event_terminate) {
+    CloseHandle(tsd->event_terminate);
+    tsd->event_terminate = NULL;
+  }
+  if (tsd->mutex_terminate) {
+    CloseHandle(tsd->mutex_terminate);
+    tsd->mutex_terminate = NULL;
+  }
+  if (tsd->mutex_waiting) {
+    CloseHandle(tsd->mutex_waiting);
+    tsd->mutex_waiting = NULL;
+  }
+}
+
+/* Initialize resolver thread synchronization data */
+BOOL init_thread_sync_data(struct thread_data * td,
+                           char * hostname,
+                           struct thread_sync_data * tsd)
+{
+  HANDLE curr_proc = GetCurrentProcess();
+
+  memset(tsd, 0, sizeof(tsd));
+  if (!DuplicateHandle(curr_proc, td->mutex_waiting,
+                       curr_proc, &tsd->mutex_waiting, 0, FALSE,
+                       DUPLICATE_SAME_ACCESS)) {
+    /* failed to duplicate the mutex, no point in continuing */
+    destroy_thread_sync_data(tsd);
+    return FALSE;
+  }
+  if (!DuplicateHandle(curr_proc, td->mutex_terminate,
+                       curr_proc, &tsd->mutex_terminate, 0, FALSE,
+                       DUPLICATE_SAME_ACCESS)) {
+    /* failed to duplicate the mutex, no point in continuing */
+    destroy_thread_sync_data(tsd);
+    return FALSE;
+  }
+  if (!DuplicateHandle(curr_proc, td->event_terminate,
+                       curr_proc, &tsd->event_terminate, 0, FALSE,
+                       DUPLICATE_SAME_ACCESS)) {
+    /* failed to duplicate the event, no point in continuing */
+    destroy_thread_sync_data(tsd);
+    return FALSE;
+  }
+  /* Copying hostname string because original can be destroyed by parent
+   * thread during gethostbyname execution.
+   */
+  tsd->hostname = strdup(hostname);
+  if (!tsd->hostname) {
+    /* Memory allocation failed */
+    destroy_thread_sync_data(tsd);
+    return FALSE;
+  }
+  return TRUE;
+}
+
+/* acquire resolver thread synchronization */
+BOOL acquire_thread_sync(struct thread_sync_data * tsd)
+{
+  /* is the thread initiator still waiting for us ? */
+  if (WaitForSingleObject(tsd->mutex_waiting, 0) == WAIT_TIMEOUT) {
+    /* yes, it is */
+
+    /* Waiting access to event_terminate */
+    if (WaitForSingleObject(tsd->mutex_terminate, INFINITE) != WAIT_OBJECT_0) {
+      /* Something went wrong - now just ignoring */
+    }
+    else {
+      if (WaitForSingleObject(tsd->event_terminate, 0) != WAIT_TIMEOUT) {
+        /* Parent thread signaled us to terminate.
+         * This means that all data in conn->async is now destroyed
+         * and we cannot use it.
+         */
+      }
+      else {
+        return TRUE;
+      }
+    }
+  }
+  return FALSE;
+}
+
+/* release resolver thread synchronization */
+void release_thread_sync(struct thread_sync_data * tsd)
+{
+  ReleaseMutex(tsd->mutex_terminate);
+}
+
 #if defined(CURLRES_IPV4)
 /*
  * gethostbyname_thread() resolves a name, calls the Curl_addrinfo4_callback
@@ -177,17 +285,13 @@ static unsigned __stdcall gethostbyname_thread (void *arg)
   struct hostent *he;
   int    rc = 0;
 
-  /* Duplicate the passed mutex handle.
+  /* Duplicate the passed mutex and event handles.
    * This allows us to use it even after the container gets destroyed
    * due to a resolver timeout.
    */
-  HANDLE mutex_waiting = NULL;
-  HANDLE curr_proc = GetCurrentProcess();
-
-  if (!DuplicateHandle(curr_proc, td->mutex_waiting,
-                       curr_proc, &mutex_waiting, 0, FALSE,
-                       DUPLICATE_SAME_ACCESS)) {
-    /* failed to duplicate the mutex, no point in continuing */
+  struct thread_sync_data tsd = {0};
+  if (!init_thread_sync_data(td, conn->async.hostname, &tsd)) {
+    /* thread synchronization data initialization failed */
     return -1;
   }
 
@@ -200,17 +304,18 @@ static unsigned __stdcall gethostbyname_thread (void *arg)
 #endif
 
   WSASetLastError (conn->async.status = NO_DATA); /* pending status */
-  he = gethostbyname (conn->async.hostname);
 
-  /* is the thread initiator still waiting for us ? */
-  if (WaitForSingleObject(mutex_waiting, 0) == WAIT_TIMEOUT) {
-    /* yes, it is */
+  /* Signaling that we have initialized all copies of data and handles we
+     need */
+  SetEvent(td->event_thread_started);
 
-    /* Mark that we have obtained the information, and that we are
-     * calling back with it.
-     */
-    SetEvent(td->event_resolved);
+  he = gethostbyname (tsd.hostname);
 
+  /* is parent thread waiting for us and are we able to access conn members? */
+  if (acquire_thread_sync(&tsd)) {
+    /* Mark that we have obtained the information, and that we are calling
+     * back with it. */
+    SetEvent(td->event_resolved);
     if (he) {
       rc = Curl_addrinfo4_callback(conn, CURL_ASYNC_SUCCESS, he);
     }
@@ -219,10 +324,11 @@ static unsigned __stdcall gethostbyname_thread (void *arg)
     }
     TRACE(("Winsock-error %d, addr %s\n", conn->async.status,
            he ? inet_ntoa(*(struct in_addr*)he->h_addr) : "unknown"));
+    release_thread_sync(&tsd);
   }
 
   /* clean up */
-  CloseHandle(mutex_waiting);
+  destroy_thread_sync_data(&tsd);
 
   return (rc);
   /* An implicit _endthreadex() here */
@@ -244,18 +350,15 @@ static unsigned __stdcall getaddrinfo_thread (void *arg)
   struct addrinfo    *res;
   char   service [NI_MAXSERV];
   int    rc;
+  addrinfo hints = td->hints;
 
   /* Duplicate the passed mutex handle.
    * This allows us to use it even after the container gets destroyed
    * due to a resolver timeout.
    */
-  HANDLE mutex_waiting = NULL;
-  HANDLE curr_proc = GetCurrentProcess();
-
-  if (!DuplicateHandle(curr_proc, td->mutex_waiting,
-                       curr_proc, &mutex_waiting, 0, FALSE,
-                       DUPLICATE_SAME_ACCESS)) {
-    /* failed to duplicate the mutex, no point in continuing */
+  struct thread_sync_data tsd = {0};
+  if (!init_thread_sync_data(td, conn->async.hostname, &tsd)) {
+    /* thread synchronization data initialization failed */
     return -1;
   }
 
@@ -267,15 +370,16 @@ static unsigned __stdcall getaddrinfo_thread (void *arg)
 
   WSASetLastError(conn->async.status = NO_DATA); /* pending status */
 
-  rc = getaddrinfo(conn->async.hostname, service, &td->hints, &res);
+  /* Signaling that we have initialized all copies of data and handles we
+     need */
+  SetEvent(td->event_thread_started);
 
-  /* is the thread initiator still waiting for us ? */
-  if (WaitForSingleObject(mutex_waiting, 0) == WAIT_TIMEOUT) {
-    /* yes, it is */
+  rc = getaddrinfo(tsd.hostname, service, &hints, &res);
 
-    /* Mark that we have obtained the information, and that we are
-     * calling back with it.
-     */
+  /* is parent thread waiting for us and are we able to access conn members? */
+  if (acquire_thread_sync(&tsd)) {
+    /* Mark that we have obtained the information, and that we are calling
+       back with it. */
     SetEvent(td->event_resolved);
 
     if (rc == 0) {
@@ -288,10 +392,11 @@ static unsigned __stdcall getaddrinfo_thread (void *arg)
       rc = Curl_addrinfo6_callback(conn, (int)WSAGetLastError(), NULL);
       TRACE(("Winsock-error %d, no address\n", conn->async.status));
     }
+    release_thread_sync(&tsd);
   }
 
   /* clean up */
-  CloseHandle(mutex_waiting);
+  destroy_thread_sync_data(&tsd);
 
   return (rc);
   /* An implicit _endthreadex() here */
@@ -311,6 +416,24 @@ void Curl_destroy_thread_data (struct Curl_async *async)
     struct thread_data *td = (struct thread_data*) async->os_specific;
     curl_socket_t sock = td->dummy_sock;
 
+    if (td->mutex_terminate && td->event_terminate) {
+      /* Signaling resolver thread to terminate */
+      if (WaitForSingleObject(td->mutex_terminate, INFINITE) == WAIT_OBJECT_0) {
+        SetEvent(td->event_terminate);
+        ReleaseMutex(td->mutex_terminate);
+      }
+      else {
+        /* Something went wrong - just ignoring it */
+      }
+    }
+
+    if (td->mutex_terminate)
+      CloseHandle(td->mutex_terminate);
+    if (td->event_terminate)
+      CloseHandle(td->event_terminate);
+    if (td->event_thread_started)
+      CloseHandle(td->event_thread_started);
+
     if (sock != CURL_SOCKET_BAD)
       sclose(sock);
 
@@ -341,6 +464,7 @@ static bool init_resolve_thread (struct connectdata *conn,
                                  const Curl_addrinfo *hints)
 {
   struct thread_data *td = calloc(sizeof(*td), 1);
+  HANDLE thread_and_event[2] = {0};
 
   if (!td) {
     SetLastError(ENOMEM);
@@ -381,6 +505,31 @@ static bool init_resolve_thread (struct connectdata *conn,
     SetLastError(EAGAIN);
     return FALSE;
   }
+  /* Create the mutex used to serialize access to event_terminated
+   * between us and resolver thread.
+   */
+  td->mutex_terminate = CreateMutex(NULL, FALSE, NULL);
+  if (td->mutex_terminate == NULL) {
+    Curl_destroy_thread_data(&conn->async);
+    SetLastError(EAGAIN);
+    return FALSE;
+  }
+  /* Create the event used to signal thread that it should terminate.
+   */
+  td->event_terminate = CreateEvent(NULL, TRUE, FALSE, NULL);
+  if (td->event_terminate == NULL) {
+    Curl_destroy_thread_data(&conn->async);
+    SetLastError(EAGAIN);
+    return FALSE;
+  }
+  /* Create the event used by thread to inform it has initialized its own data.
+   */
+  td->event_thread_started = CreateEvent(NULL, TRUE, FALSE, NULL);
+  if (td->event_thread_started == NULL) {
+    Curl_destroy_thread_data(&conn->async);
+    SetLastError(EAGAIN);
+    return FALSE;
+  }
 
   td->stderr_file = stderr;
 
@@ -406,6 +555,15 @@ static bool init_resolve_thread (struct connectdata *conn,
      Curl_destroy_thread_data(&conn->async);
      return FALSE;
   }
+  /* Waiting until the thread will initialize its data or it will exit due errors.
+   */
+  thread_and_event[0] = td->thread_hnd;
+  thread_and_event[1] = td->event_thread_started;
+  if (WaitForMultipleObjects(sizeof(thread_and_event) / sizeof(thread_and_event[0]), thread_and_event, FALSE, INFINITE) == WAIT_FAILED) {
+    /* The resolver thread has been created,
+     * most probably it works now - ignoring this "minor" error
+     */
+  }
   /* This socket is only to keep Curl_resolv_fdset() and select() happy;
    * should never become signalled for read/write since it's unbound but
    * Windows needs atleast 1 socket in select().