asyn-thrdd: drop pthread_cancel

Remove use of pthread_cancel in asnyc threaded resolving. While there
are system where this works, others might leak to resource leakage
(memory, files, etc.). The popular nsswitch is one example where resolve
code can be dragged in that is not prepared.

The overall promise and mechanism of pthread_cancel() is just too
brittle and the historcal design of getaddrinfo() continues to haunt us.

Fixes #18532
Reported-by: Javier Blazquez
Closes #18540

lib/asyn-thrdd.c

@@ -199,14 +199,6 @@ err_exit:
   return NULL;
 }
 
-static void async_thrd_cleanup(void *arg)
-{
-  struct async_thrdd_addr_ctx *addr_ctx = arg;
-
-  Curl_thread_disable_cancel();
-  addr_ctx_unlink(&addr_ctx, NULL);
-}
-
 #ifdef HAVE_GETADDRINFO
 
 /*
@@ -220,15 +212,6 @@ static CURL_THREAD_RETURN_T CURL_STDCALL getaddrinfo_thread(void *arg)
   struct async_thrdd_addr_ctx *addr_ctx = arg;
   bool do_abort;
 
-/* clang complains about empty statements and the pthread_cleanup* macros
- * are pretty ill defined. */
-#if defined(__clang__)
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wextra-semi-stmt"
-#endif
-
-  Curl_thread_push_cleanup(async_thrd_cleanup, addr_ctx);
-
   Curl_mutex_acquire(&addr_ctx->mutx);
   do_abort = addr_ctx->do_abort;
   Curl_mutex_release(&addr_ctx->mutx);
@@ -237,9 +220,6 @@ static CURL_THREAD_RETURN_T CURL_STDCALL getaddrinfo_thread(void *arg)
     char service[12];
     int rc;
 
-#ifdef DEBUGBUILD
-    Curl_resolve_test_delay();
-#endif
     msnprintf(service, sizeof(service), "%d", addr_ctx->port);
 
     rc = Curl_getaddrinfo_ex(addr_ctx->hostname, service,
@@ -274,11 +254,6 @@ static CURL_THREAD_RETURN_T CURL_STDCALL getaddrinfo_thread(void *arg)
 
   }
 
-  Curl_thread_pop_cleanup();
-#if defined(__clang__)
-#pragma clang diagnostic pop
-#endif
-
   addr_ctx_unlink(&addr_ctx, NULL);
   return 0;
 }
@@ -293,24 +268,11 @@ static CURL_THREAD_RETURN_T CURL_STDCALL gethostbyname_thread(void *arg)
   struct async_thrdd_addr_ctx *addr_ctx = arg;
   bool do_abort;
 
-/* clang complains about empty statements and the pthread_cleanup* macros
- * are pretty ill defined. */
-#if defined(__clang__)
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wextra-semi-stmt"
-#endif
-
-  Curl_thread_push_cleanup(async_thrd_cleanup, addr_ctx);
-
   Curl_mutex_acquire(&addr_ctx->mutx);
   do_abort = addr_ctx->do_abort;
   Curl_mutex_release(&addr_ctx->mutx);
 
   if(!do_abort) {
-#ifdef DEBUGBUILD
-    Curl_resolve_test_delay();
-#endif
-
     addr_ctx->res = Curl_ipv4_resolve_r(addr_ctx->hostname, addr_ctx->port);
     if(!addr_ctx->res) {
       addr_ctx->sock_error = SOCKERRNO;
@@ -337,12 +299,7 @@ static CURL_THREAD_RETURN_T CURL_STDCALL gethostbyname_thread(void *arg)
 #endif
   }
 
-  Curl_thread_pop_cleanup();
-#if defined(__clang__)
-#pragma clang diagnostic pop
-#endif
-
-  async_thrd_cleanup(addr_ctx);
+  addr_ctx_unlink(&addr_ctx, NULL);
   return 0;
 }
 
@@ -381,12 +338,12 @@ static void async_thrdd_destroy(struct Curl_easy *data)
       CURL_TRC_DNS(data, "async_thrdd_destroy, thread joined");
     }
     else {
-      /* thread is still running. Detach the thread while mutexed, it will
-       * trigger the cleanup when it releases its reference. */
+      /* thread is still running. Detach it. */
       Curl_thread_destroy(&addr->thread_hnd);
       CURL_TRC_DNS(data, "async_thrdd_destroy, thread detached");
     }
   }
+  /* release our reference to the shared context */
   addr_ctx_unlink(&thrdd->addr, data);
 }
 
@@ -532,10 +489,12 @@ static void async_thrdd_shutdown(struct Curl_easy *data)
   done = addr_ctx->thrd_done;
   Curl_mutex_release(&addr_ctx->mutx);
 
-  DEBUGASSERT(addr_ctx->thread_hnd != curl_thread_t_null);
-  if(!done && (addr_ctx->thread_hnd != curl_thread_t_null)) {
-    CURL_TRC_DNS(data, "cancelling resolve thread");
-    (void)Curl_thread_cancel(&addr_ctx->thread_hnd);
+  /* Wait for the thread to terminate if it is already marked done. If it is
+     not done yet we cannot do anything here. We had tried pthread_cancel but
+     it caused hanging and resource leaks (#18532). */
+  if(done && (addr_ctx->thread_hnd != curl_thread_t_null)) {
+    Curl_thread_join(&addr_ctx->thread_hnd);
+    CURL_TRC_DNS(data, "async_thrdd_shutdown, thread joined");
   }
 }
 
@@ -553,9 +512,11 @@ static CURLcode asyn_thrdd_await(struct Curl_easy *data,
     if(!entry)
       async_thrdd_shutdown(data);
 
-    CURL_TRC_DNS(data, "resolve, wait for thread to finish");
-    if(!Curl_thread_join(&addr_ctx->thread_hnd)) {
-      DEBUGASSERT(0);
+    if(addr_ctx->thread_hnd != curl_thread_t_null) {
+      CURL_TRC_DNS(data, "resolve, wait for thread to finish");
+      if(!Curl_thread_join(&addr_ctx->thread_hnd)) {
+        DEBUGASSERT(0);
+      }
     }
 
     if(entry)

lib/curl_threads.c

@@ -100,34 +100,6 @@ int Curl_thread_join(curl_thread_t *hnd)
   return ret;
 }
 
-/* do not use pthread_cancel if:
- * - pthread_cancel seems to be absent
- * - on FreeBSD, as we see hangers in CI testing
- * - this is a -fsanitize=thread build
- *   (clang sanitizer reports false positive when functions to not return)
- */
-#if defined(PTHREAD_CANCEL_ENABLE) && !defined(__FreeBSD__)
-#if defined(__has_feature)
-#  if !__has_feature(thread_sanitizer)
-#define USE_PTHREAD_CANCEL
-#  endif
-#else /* __has_feature */
-#define USE_PTHREAD_CANCEL
-#endif /* !__has_feature */
-#endif /* PTHREAD_CANCEL_ENABLE && !__FreeBSD__ */
-
-int Curl_thread_cancel(curl_thread_t *hnd)
-{
-  (void)hnd;
-  if(*hnd != curl_thread_t_null)
-#ifdef USE_PTHREAD_CANCEL
-    return pthread_cancel(**hnd);
-#else
-    return 1; /* not supported */
-#endif
-  return 0;
-}
-
 #elif defined(USE_THREADS_WIN32)
 
 curl_thread_t Curl_thread_create(CURL_THREAD_RETURN_T
@@ -182,12 +154,4 @@ int Curl_thread_join(curl_thread_t *hnd)
   return ret;
 }
 
-int Curl_thread_cancel(curl_thread_t *hnd)
-{
-  if(*hnd != curl_thread_t_null) {
-    return 1; /* not supported */
-  }
-  return 0;
-}
-
 #endif /* USE_THREADS_* */

lib/curl_threads.h

@@ -66,22 +66,6 @@ void Curl_thread_destroy(curl_thread_t *hnd);
 
 int Curl_thread_join(curl_thread_t *hnd);
 
-int Curl_thread_cancel(curl_thread_t *hnd);
-
-#if defined(USE_THREADS_POSIX) && defined(PTHREAD_CANCEL_ENABLE)
-#define Curl_thread_push_cleanup(a,b)   pthread_cleanup_push(a,b)
-#define Curl_thread_pop_cleanup()       pthread_cleanup_pop(0)
-#define Curl_thread_enable_cancel()     \
-    pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL)
-#define Curl_thread_disable_cancel()     \
-    pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL)
-#else
-#define Curl_thread_push_cleanup(a,b)   ((void)a,(void)b)
-#define Curl_thread_pop_cleanup()       Curl_nop_stmt
-#define Curl_thread_enable_cancel()     Curl_nop_stmt
-#define Curl_thread_disable_cancel()    Curl_nop_stmt
-#endif
-
 #endif /* USE_THREADS_POSIX || USE_THREADS_WIN32 */
 
 #endif /* HEADER_CURL_THREADS_H */

lib/hostip.c

@@ -1132,10 +1132,6 @@ CURLcode Curl_resolv_timeout(struct Curl_easy *data,
     prev_alarm = alarm(curlx_sltoui(timeout/1000L));
   }
 
-#ifdef DEBUGBUILD
-  Curl_resolve_test_delay();
-#endif
-
 #else /* !USE_ALARM_TIMEOUT */
 #ifndef CURLRES_ASYNCH
   if(timeoutms)
@@ -1639,18 +1635,3 @@ CURLcode Curl_resolver_error(struct Curl_easy *data, const char *detail)
   return result;
 }
 #endif /* USE_CURL_ASYNC */
-
-#ifdef DEBUGBUILD
-#include "curlx/wait.h"
-
-void Curl_resolve_test_delay(void)
-{
-  const char *p = getenv("CURL_DNS_DELAY_MS");
-  if(p) {
-    curl_off_t l;
-    if(!curlx_str_number(&p, &l, TIME_T_MAX) && l) {
-      curlx_wait_ms((timediff_t)l);
-    }
-  }
-}
-#endif

lib/hostip.h

@@ -216,8 +216,4 @@ struct Curl_addrinfo *Curl_sync_getaddrinfo(struct Curl_easy *data,
 
 #endif
 
-#ifdef DEBUGBUILD
-void Curl_resolve_test_delay(void);
-#endif
-
 #endif /* HEADER_CURL_HOSTIP_H */