]> git.ipfire.org Git - thirdparty/curl.git/commitdiff
asyn-thread: stop using GetAddrInfoExW on Windows
authorJay Satiro <raysatiro@yahoo.com>
Thu, 5 Sep 2024 06:18:25 +0000 (02:18 -0400)
committerJay Satiro <raysatiro@yahoo.com>
Sun, 8 Sep 2024 15:39:30 +0000 (11:39 -0400)
- For the threaded resolver backend on Windows, revert back to
  exclusively use the threaded resolver with libcurl-owned threading
  instead of GetAddrInfoExW with Windows-owned threading.

Winsock (the Windows sockets library) has a bug where it does not wait
for all of the name resolver threads it is managing to terminate before
returning from WSACleanup. The threads continue to run and may cause a
crash.

This commit is effectively a revert of several commits that encompass
all GetAddrInfoExW code in libcurl. A manual review of merge conflicts
was used to resolve minor changes that had modified the code for
aesthetic or build reasons in other commits.

Prior to this change if libcurl was built with the threaded resolver
backend for Windows, and Windows 8 or later was the operating system at
runtime, and the caller was not impersonating another user, then libcurl
would use GetAddrInfoExW to handle asynchronous name lookups.

GetAddrInfoExW support was added in a6bbc87f, which preceded 8.6.0, and
prior to that the threaded resolver backend used libcurl-owned threading
exclusively on Windows.

Reported-by: IonuČ›-Francisc Oancea
Reported-by: Razvan Pricope
Ref: https://developercommunity.visualstudio.com/t/ASAN:-heap-use-after-free-in-NdrFullPoin/10654169

Fixes https://github.com/curl/curl/issues/13509#issuecomment-2225338110
Closes https://github.com/curl/curl/pull/14794

---

Revert "asyn-thread: avoid using GetAddrInfoExW with impersonation"

This reverts commit 0caadc1f24d20514eed2bf6e5ef0adc140f122c3.

Conflicts:
lib/system_win32.c

--

Revert "asyn-thread: fix curl_global_cleanup crash in Windows"

This reverts commit 428579f5d136fd473e97fe089c42ffee55b72a8f.

--

Revert "system_win32: fix a function pointer assignment warning"

This reverts commit 26f002e02ef1142a432c8dc087bd27de71ce38bf.

--

Revert "asyn-thread: use GetAddrInfoExW on >= Windows 8"

This reverts commit a6bbc87f9e9ffb46a1801dfb983e7534825ed56b.

Conflicts:
lib/asyn-thread.c
lib/system_win32.c

--

lib/asyn-thread.c
lib/system_win32.c
lib/system_win32.h

index 2136720602fdfe48edd14a38893347131f17c631..79b9c239cf0cd777a15d85b43f0fd00d80faa36e 100644 (file)
@@ -54,7 +54,6 @@
 #  define RESOLVER_ENOMEM  ENOMEM
 #endif
 
-#include "system_win32.h"
 #include "urldata.h"
 #include "sendf.h"
 #include "hostip.h"
@@ -145,22 +144,9 @@ static bool init_resolve_thread(struct Curl_easy *data,
                                 const char *hostname, int port,
                                 const struct addrinfo *hints);
 
-#ifdef _WIN32
-/* Thread sync data used by GetAddrInfoExW for win8+ */
-struct thread_sync_data_w8
-{
-  OVERLAPPED overlapped;
-  ADDRINFOEXW_ *res;
-  HANDLE cancel_ev;
-  ADDRINFOEXW_ hints;
-};
-#endif
 
 /* Data for synchronization between resolver thread and its parent */
 struct thread_sync_data {
-#ifdef _WIN32
-  struct thread_sync_data_w8 w8;
-#endif
   curl_mutex_t *mtx;
   int done;
   int port;
@@ -179,9 +165,6 @@ struct thread_sync_data {
 };
 
 struct thread_data {
-#ifdef _WIN32
-  HANDLE complete_ev;
-#endif
   curl_thread_t thread_hnd;
   unsigned int poll_interval;
   timediff_t interval_end;
@@ -293,162 +276,6 @@ static CURLcode getaddrinfo_complete(struct Curl_easy *data)
   return result;
 }
 
-#ifdef _WIN32
-static VOID WINAPI
-query_complete(DWORD err, DWORD bytes, LPWSAOVERLAPPED overlapped)
-{
-  size_t ss_size;
-  const ADDRINFOEXW_ *ai;
-  struct Curl_addrinfo *ca;
-  struct Curl_addrinfo *cafirst = NULL;
-  struct Curl_addrinfo *calast = NULL;
-#ifndef CURL_DISABLE_SOCKETPAIR
-#ifdef USE_EVENTFD
-  const void *buf;
-  const uint64_t val = 1;
-#else
-  char buf[1];
-#endif
-#endif
-#ifdef __clang__
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wcast-align"
-#endif
-  struct thread_sync_data *tsd =
-    CONTAINING_RECORD(overlapped, struct thread_sync_data, w8.overlapped);
-#ifdef __clang__
-#pragma clang diagnostic pop
-#endif
-  struct thread_data *td = tsd->td;
-  const ADDRINFOEXW_ *res = tsd->w8.res;
-  int error = (int)err;
-  (void)bytes;
-
-  if(error == ERROR_SUCCESS) {
-    /* traverse the addrinfo list */
-
-    for(ai = res; ai != NULL; ai = ai->ai_next) {
-      size_t namelen = ai->ai_canonname ? wcslen(ai->ai_canonname) + 1 : 0;
-      /* ignore elements with unsupported address family, */
-      /* settle family-specific sockaddr structure size.  */
-      if(ai->ai_family == AF_INET)
-        ss_size = sizeof(struct sockaddr_in);
-#ifdef USE_IPV6
-      else if(ai->ai_family == AF_INET6)
-        ss_size = sizeof(struct sockaddr_in6);
-#endif
-      else
-        continue;
-
-      /* ignore elements without required address info */
-      if(!ai->ai_addr || !(ai->ai_addrlen > 0))
-        continue;
-
-      /* ignore elements with bogus address size */
-      if((size_t)ai->ai_addrlen < ss_size)
-        continue;
-
-      ca = malloc(sizeof(struct Curl_addrinfo) + ss_size + namelen);
-      if(!ca) {
-        error = EAI_MEMORY;
-        break;
-      }
-
-      /* copy each structure member individually, member ordering, */
-      /* size, or padding might be different for each platform.    */
-      ca->ai_flags     = ai->ai_flags;
-      ca->ai_family    = ai->ai_family;
-      ca->ai_socktype  = ai->ai_socktype;
-      ca->ai_protocol  = ai->ai_protocol;
-      ca->ai_addrlen   = (curl_socklen_t)ss_size;
-      ca->ai_addr      = NULL;
-      ca->ai_canonname = NULL;
-      ca->ai_next      = NULL;
-
-      ca->ai_addr = (void *)((char *)ca + sizeof(struct Curl_addrinfo));
-      memcpy(ca->ai_addr, ai->ai_addr, ss_size);
-
-      if(namelen) {
-        size_t i;
-        ca->ai_canonname = (void *)((char *)ca->ai_addr + ss_size);
-        for(i = 0; i < namelen; ++i) /* convert wide string to ASCII */
-          ca->ai_canonname[i] = (char)ai->ai_canonname[i];
-        ca->ai_canonname[namelen] = '\0';
-      }
-
-      /* if the return list is empty, this becomes the first element */
-      if(!cafirst)
-        cafirst = ca;
-
-      /* add this element last in the return list */
-      if(calast)
-        calast->ai_next = ca;
-      calast = ca;
-    }
-
-    /* if we failed, also destroy the Curl_addrinfo list */
-    if(error) {
-      Curl_freeaddrinfo(cafirst);
-      cafirst = NULL;
-    }
-    else if(!cafirst) {
-#ifdef EAI_NONAME
-      /* rfc3493 conformant */
-      error = EAI_NONAME;
-#else
-      /* rfc3493 obsoleted */
-      error = EAI_NODATA;
-#endif
-#ifdef USE_WINSOCK
-      SET_SOCKERRNO(error);
-#endif
-    }
-    tsd->res = cafirst;
-  }
-
-  if(tsd->w8.res) {
-    Curl_FreeAddrInfoExW(tsd->w8.res);
-    tsd->w8.res = NULL;
-  }
-
-  if(error) {
-    tsd->sock_error = SOCKERRNO?SOCKERRNO:error;
-    if(tsd->sock_error == 0)
-      tsd->sock_error = RESOLVER_ENOMEM;
-  }
-  else {
-    Curl_addrinfo_set_port(tsd->res, tsd->port);
-  }
-
-  Curl_mutex_acquire(tsd->mtx);
-  if(tsd->done) {
-    /* too late, gotta clean up the mess */
-    Curl_mutex_release(tsd->mtx);
-    destroy_thread_sync_data(tsd);
-    free(td);
-  }
-  else {
-#ifndef CURL_DISABLE_SOCKETPAIR
-    if(tsd->sock_pair[1] != CURL_SOCKET_BAD) {
-#ifdef USE_EVENTFD
-      buf = &val;
-#else
-      buf[0] = 1;
-#endif
-      /* DNS has been resolved, signal client task */
-      if(wakeup_write(tsd->sock_pair[1], buf, sizeof(buf)) < 0) {
-        /* update sock_erro to errno */
-        tsd->sock_error = SOCKERRNO;
-      }
-    }
-#endif
-    tsd->done = 1;
-    Curl_mutex_release(tsd->mtx);
-    if(td->complete_ev)
-      SetEvent(td->complete_ev); /* Notify caller that the query completed */
-  }
-}
-#endif
 
 #ifdef HAVE_GETADDRINFO
 
@@ -585,26 +412,9 @@ static void destroy_async_data(struct Curl_async *async)
     Curl_mutex_release(td->tsd.mtx);
 
     if(!done) {
-#ifdef _WIN32
-      if(td->complete_ev) {
-        CloseHandle(td->complete_ev);
-        td->complete_ev = NULL;
-      }
-#endif
-      if(td->thread_hnd != curl_thread_t_null) {
-        Curl_thread_destroy(td->thread_hnd);
-        td->thread_hnd = curl_thread_t_null;
-      }
+      Curl_thread_destroy(td->thread_hnd);
     }
     else {
-#ifdef _WIN32
-      if(td->complete_ev) {
-        Curl_GetAddrInfoExCancel(&td->tsd.w8.cancel_ev);
-        WaitForSingleObject(td->complete_ev, INFINITE);
-        CloseHandle(td->complete_ev);
-        td->complete_ev = NULL;
-      }
-#endif
       if(td->thread_hnd != curl_thread_t_null)
         Curl_thread_join(&td->thread_hnd);
 
@@ -650,9 +460,6 @@ static bool init_resolve_thread(struct Curl_easy *data,
   asp->status = 0;
   asp->dns = NULL;
   td->thread_hnd = curl_thread_t_null;
-#ifdef _WIN32
-  td->complete_ev = NULL;
-#endif
 
   if(!init_thread_sync_data(td, hostname, port, hints)) {
     asp->tdata = NULL;
@@ -668,42 +475,6 @@ static bool init_resolve_thread(struct Curl_easy *data,
   /* The thread will set this to 1 when complete. */
   td->tsd.done = 0;
 
-#ifdef _WIN32
-  if(Curl_isWindows8OrGreater && Curl_FreeAddrInfoExW &&
-     Curl_GetAddrInfoExCancel && Curl_GetAddrInfoExW &&
-     !Curl_win32_impersonating()) {
-#define MAX_NAME_LEN 256 /* max domain name is 253 chars */
-#define MAX_PORT_LEN 8
-    WCHAR namebuf[MAX_NAME_LEN];
-    WCHAR portbuf[MAX_PORT_LEN];
-    /* calculate required length */
-    int w_len = MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, hostname,
-                                    -1, NULL, 0);
-    if((w_len > 0) && (w_len < MAX_NAME_LEN)) {
-      /* do utf8 conversion */
-      w_len = MultiByteToWideChar(CP_UTF8, 0, hostname, -1, namebuf, w_len);
-      if((w_len > 0) && (w_len < MAX_NAME_LEN)) {
-        swprintf(portbuf, MAX_PORT_LEN, L"%d", port);
-        td->tsd.w8.hints.ai_family = hints->ai_family;
-        td->tsd.w8.hints.ai_socktype = hints->ai_socktype;
-        td->complete_ev = CreateEvent(NULL, TRUE, FALSE, NULL);
-        if(!td->complete_ev) {
-          /* failed to start, mark it as done here for proper cleanup. */
-          td->tsd.done = 1;
-          goto err_exit;
-        }
-        err = Curl_GetAddrInfoExW(namebuf, portbuf, NS_DNS,
-                                  NULL, &td->tsd.w8.hints, &td->tsd.w8.res,
-                                  NULL, &td->tsd.w8.overlapped,
-                                  &query_complete, &td->tsd.w8.cancel_ev);
-        if(err != WSA_IO_PENDING)
-          query_complete((DWORD)err, 0, &td->tsd.w8.overlapped);
-        return TRUE;
-      }
-    }
-  }
-#endif
-
 #ifdef HAVE_GETADDRINFO
   td->thread_hnd = Curl_thread_create(getaddrinfo_thread, &td->tsd);
 #else
@@ -740,23 +511,9 @@ static CURLcode thread_wait_resolv(struct Curl_easy *data,
   DEBUGASSERT(data);
   td = data->state.async.tdata;
   DEBUGASSERT(td);
-#ifdef _WIN32
-  DEBUGASSERT(td->complete_ev || td->thread_hnd != curl_thread_t_null);
-#else
   DEBUGASSERT(td->thread_hnd != curl_thread_t_null);
-#endif
 
   /* wait for the thread to resolve the name */
-#ifdef _WIN32
-  if(td->complete_ev) {
-    WaitForSingleObject(td->complete_ev, INFINITE);
-    CloseHandle(td->complete_ev);
-    td->complete_ev = NULL;
-    if(entry)
-      result = getaddrinfo_complete(data);
-  }
-  else
-#endif
   if(Curl_thread_join(&td->thread_hnd)) {
     if(entry)
       result = getaddrinfo_complete(data);
@@ -793,13 +550,6 @@ void Curl_resolver_kill(struct Curl_easy *data)
   /* If we are still resolving, we must wait for the threads to fully clean up,
      unfortunately. Otherwise, we can simply cancel to clean up any resolver
      data. */
-#ifdef _WIN32
-  if(td && td->complete_ev) {
-    Curl_GetAddrInfoExCancel(&td->tsd.w8.cancel_ev);
-    (void)thread_wait_resolv(data, NULL, FALSE);
-  }
-  else
-#endif
   if(td && td->thread_hnd != curl_thread_t_null
      && (data->set.quick_exit != 1L))
     (void)thread_wait_resolv(data, NULL, FALSE);
index c3e1fabfbf20d9968641fcf19d5d5b03dbcc6dfc..f4dbe0310af756fd9bb80fdcaba9c55774fba229 100644 (file)
 
 LARGE_INTEGER Curl_freq;
 bool Curl_isVistaOrGreater;
-bool Curl_isWindows8OrGreater;
 
 /* Handle of iphlpapp.dll */
 static HMODULE s_hIpHlpApiDll = NULL;
 
-/* Function pointers */
+/* Pointer to the if_nametoindex function */
 IF_NAMETOINDEX_FN Curl_if_nametoindex = NULL;
-FREEADDRINFOEXW_FN Curl_FreeAddrInfoExW = NULL;
-GETADDRINFOEXCANCEL_FN Curl_GetAddrInfoExCancel = NULL;
-GETADDRINFOEXW_FN Curl_GetAddrInfoExW = NULL;
 
 /* Curl_win32_init() performs Win32 global initialization */
 CURLcode Curl_win32_init(long flags)
 {
-#ifdef USE_WINSOCK
-  HMODULE ws2_32Dll;
-#endif
   /* CURL_GLOBAL_WIN32 controls the *optional* part of the initialization which
      is just for Winsock at the moment. Any required Win32 initialization
      should take place after this block. */
@@ -111,22 +104,6 @@ CURLcode Curl_win32_init(long flags)
       Curl_if_nametoindex = pIfNameToIndex;
   }
 
-#ifdef USE_WINSOCK
-#ifdef CURL_WINDOWS_APP
-  ws2_32Dll = Curl_load_library(TEXT("ws2_32.dll"));
-#else
-  ws2_32Dll = GetModuleHandleA("ws2_32");
-#endif
-  if(ws2_32Dll) {
-    Curl_FreeAddrInfoExW = CURLX_FUNCTION_CAST(FREEADDRINFOEXW_FN,
-      GetProcAddress(ws2_32Dll, "FreeAddrInfoExW"));
-    Curl_GetAddrInfoExCancel = CURLX_FUNCTION_CAST(GETADDRINFOEXCANCEL_FN,
-      GetProcAddress(ws2_32Dll, "GetAddrInfoExCancel"));
-    Curl_GetAddrInfoExW = CURLX_FUNCTION_CAST(GETADDRINFOEXW_FN,
-      GetProcAddress(ws2_32Dll, "GetAddrInfoExW"));
-  }
-#endif
-
   /* curlx_verify_windows_version must be called during init at least once
      because it has its own initialization routine. */
   if(curlx_verify_windows_version(6, 0, 0, PLATFORM_WINNT,
@@ -136,13 +113,6 @@ CURLcode Curl_win32_init(long flags)
   else
     Curl_isVistaOrGreater = FALSE;
 
-  if(curlx_verify_windows_version(6, 2, 0, PLATFORM_WINNT,
-                                  VERSION_GREATER_THAN_EQUAL)) {
-    Curl_isWindows8OrGreater = TRUE;
-  }
-  else
-    Curl_isWindows8OrGreater = FALSE;
-
   QueryPerformanceFrequency(&Curl_freq);
   return CURLE_OK;
 }
@@ -150,9 +120,6 @@ CURLcode Curl_win32_init(long flags)
 /* Curl_win32_cleanup() is the opposite of Curl_win32_init() */
 void Curl_win32_cleanup(long init_flags)
 {
-  Curl_FreeAddrInfoExW = NULL;
-  Curl_GetAddrInfoExCancel = NULL;
-  Curl_GetAddrInfoExW = NULL;
   if(s_hIpHlpApiDll) {
     FreeLibrary(s_hIpHlpApiDll);
     s_hIpHlpApiDll = NULL;
@@ -271,16 +238,4 @@ HMODULE Curl_load_library(LPCTSTR filename)
 #endif
 }
 
-bool Curl_win32_impersonating(void)
-{
-#ifndef CURL_WINDOWS_APP
-  HANDLE token = NULL;
-  if(OpenThreadToken(GetCurrentThread(), TOKEN_QUERY, TRUE, &token)) {
-    CloseHandle(token);
-    return TRUE;
-  }
-#endif
-  return FALSE;
-}
-
 #endif /* _WIN32 */
index 534a5e4988e5ae7e0a494a63fdec2c1a50672844..024d959f327542976862be1284bc4a30477f97c2 100644 (file)
 
 #include "curl_setup.h"
 
-#ifdef _WIN32
+#if defined(_WIN32)
 
 #include <curl/curl.h>
 
 extern LARGE_INTEGER Curl_freq;
 extern bool Curl_isVistaOrGreater;
-extern bool Curl_isWindows8OrGreater;
 
 CURLcode Curl_win32_init(long flags);
 void Curl_win32_cleanup(long init_flags);
@@ -43,35 +42,6 @@ typedef unsigned int(WINAPI *IF_NAMETOINDEX_FN)(const char *);
 /* This is used instead of if_nametoindex if available on Windows */
 extern IF_NAMETOINDEX_FN Curl_if_nametoindex;
 
-/* Identical copy of addrinfoexW/ADDRINFOEXW */
-typedef struct addrinfoexW_
-{
-  int                  ai_flags;
-  int                  ai_family;
-  int                  ai_socktype;
-  int                  ai_protocol;
-  size_t               ai_addrlen;
-  PWSTR                ai_canonname;
-  struct sockaddr     *ai_addr;
-  void                *ai_blob;
-  size_t               ai_bloblen;
-  LPGUID               ai_provider;
-  struct addrinfoexW_ *ai_next;
-} ADDRINFOEXW_;
-
-typedef void (CALLBACK *LOOKUP_COMPLETION_FN)(DWORD, DWORD, LPWSAOVERLAPPED);
-typedef void (WSAAPI *FREEADDRINFOEXW_FN)(ADDRINFOEXW_*);
-typedef int (WSAAPI *GETADDRINFOEXCANCEL_FN)(LPHANDLE);
-typedef int (WSAAPI *GETADDRINFOEXW_FN)(PCWSTR, PCWSTR, DWORD, LPGUID,
-  const ADDRINFOEXW_*, ADDRINFOEXW_**, struct timeval*, LPOVERLAPPED,
-  LOOKUP_COMPLETION_FN, LPHANDLE);
-
-extern FREEADDRINFOEXW_FN Curl_FreeAddrInfoExW;
-extern GETADDRINFOEXCANCEL_FN Curl_GetAddrInfoExCancel;
-extern GETADDRINFOEXW_FN Curl_GetAddrInfoExW;
-
-bool Curl_win32_impersonating(void);
-
 /* This is used to dynamically load DLLs */
 HMODULE Curl_load_library(LPCTSTR filename);
 #else  /* _WIN32 */