thread deadlock: make sure channel lock isn't used recursively

docs/ares_init_options.3

@@ -221,7 +221,8 @@ The value of
 .I sock_state_cb_data
 will be passed as the
 .I data
-argument.
+argument.  Since this may be called cross-thread, a channel lock is NOT held
+when this callback is called.
 
 Cannot be used with \fBARES_OPT_EVENT_THREAD\fP.
 .TP 18

src/lib/ares__threads.c

@@ -219,30 +219,18 @@ struct ares__thread_mutex {
 
 ares__thread_mutex_t *ares__thread_mutex_create(void)
 {
-  pthread_mutexattr_t   attr;
   ares__thread_mutex_t *mut = ares_malloc_zero(sizeof(*mut));
   if (mut == NULL) {
     return NULL;
   }
 
-  if (pthread_mutexattr_init(&attr) != 0) {
-    ares_free(mut);
-    return NULL;
-  }
-
-  if (pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE) != 0) {
-    goto fail;
-  }
-
-  if (pthread_mutex_init(&mut->mutex, &attr) != 0) {
+  if (pthread_mutex_init(&mut->mutex, NULL) != 0) {
     goto fail;
   }
 
-  pthread_mutexattr_destroy(&attr);
   return mut;
 
 fail:
-  pthread_mutexattr_destroy(&attr);
   ares_free(mut);
   return NULL;
 }

src/lib/ares_gethostbyaddr.c

@@ -68,9 +68,9 @@ static ares_status_t file_lookup(ares_channel_t         *channel,
                                  const struct ares_addr *addr,
                                  struct hostent        **host);
 
-static void ares_gethostbyaddr_int(ares_channel_t *channel, const void *addr,
-                                   int addrlen, int family,
-                                   ares_host_callback callback, void *arg)
+void ares_gethostbyaddr_nolock(ares_channel_t *channel, const void *addr,
+                               int addrlen, int family,
+                               ares_host_callback callback, void *arg)
 {
   struct addr_query *aquery;
 
@@ -118,7 +118,7 @@ void ares_gethostbyaddr(ares_channel_t *channel, const void *addr, int addrlen,
     return;
   }
   ares__channel_lock(channel);
-  ares_gethostbyaddr_int(channel, addr, addrlen, family, callback, arg);
+  ares_gethostbyaddr_nolock(channel, addr, addrlen, family, callback, arg);
   ares__channel_unlock(channel);
 }
 
@@ -138,7 +138,7 @@ static void next_lookup(struct addr_query *aquery)
           return;
         }
         aquery->remaining_lookups = p + 1;
-        ares_query(aquery->channel, name, C_IN, T_PTR, addr_callback, aquery);
+        ares_query_qid(aquery->channel, name, C_IN, T_PTR, addr_callback, aquery, NULL);
         ares_free(name);
         return;
       case 'f':

src/lib/ares_getnameinfo.c

@@ -173,14 +173,15 @@ static void  ares_getnameinfo_int(ares_channel_t        *channel,
       if (sa->sa_family == AF_INET) {
         niquery->family = AF_INET;
         memcpy(&niquery->addr.addr4, addr, sizeof(niquery->addr.addr4));
-        ares_gethostbyaddr(channel, &addr->sin_addr, sizeof(struct in_addr),
-                           AF_INET, nameinfo_callback, niquery);
+        ares_gethostbyaddr_nolock(channel, &addr->sin_addr,
+                                  sizeof(struct in_addr), AF_INET,
+                                  nameinfo_callback, niquery);
       } else {
         niquery->family = AF_INET6;
         memcpy(&niquery->addr.addr6, addr6, sizeof(niquery->addr.addr6));
-        ares_gethostbyaddr(channel, &addr6->sin6_addr,
-                           sizeof(struct ares_in6_addr), AF_INET6,
-                           nameinfo_callback, niquery);
+        ares_gethostbyaddr_nolock(channel, &addr6->sin6_addr,
+                                  sizeof(struct ares_in6_addr), AF_INET6,
+                                  nameinfo_callback, niquery);
       }
     }
   }

src/lib/ares_init.c

@@ -430,7 +430,6 @@ int ares_dup(ares_channel_t **dest, ares_channel_t *src)
 
   *dest = NULL; /* in case of failure return NULL explicitly */
 
-  ares__channel_lock(src);
   /* First get the options supported by the old ares_save_options() function,
      which is most of them */
   rc = (ares_status_t)ares_save_options(src, &opts, &optmask);
@@ -449,6 +448,7 @@ int ares_dup(ares_channel_t **dest, ares_channel_t *src)
     goto done;
   }
 
+  ares__channel_lock(src);
   /* Now clone the options that ares_save_options() doesn't support, but are
    * user-provided */
   (*dest)->sock_create_cb      = src->sock_create_cb;
@@ -462,7 +462,7 @@ int ares_dup(ares_channel_t **dest, ares_channel_t *src)
               sizeof((*dest)->local_dev_name));
   (*dest)->local_ip4 = src->local_ip4;
   memcpy((*dest)->local_ip6, src->local_ip6, sizeof(src->local_ip6));
-
+  ares__channel_unlock(src);
 
   /* Servers are a bit unique as ares_init_options() only allows ipv4 servers
    * and not a port per server, but there are other user specified ways, that
@@ -494,7 +494,6 @@ int ares_dup(ares_channel_t **dest, ares_channel_t *src)
 
   rc = ARES_SUCCESS;
 done:
-  ares__channel_unlock(src);
   return (int)rc; /* everything went fine */
 }
 

src/lib/ares_private.h

@@ -495,6 +495,11 @@ ares_status_t ares__hosts_entry_to_addrinfo(const ares_hosts_entry_t *entry,
                                             ares_bool_t           want_cnames,
                                             struct ares_addrinfo *ai);
 
+/* Same as ares_gethostbyaddr() except does not take a channel lock.  Use this
+ * if a channel lock is already held */
+void ares_gethostbyaddr_nolock(ares_channel_t *channel, const void *addr,
+                               int addrlen, int family,
+                               ares_host_callback callback, void *arg);
 
 /*! Parse a compressed DNS name as defined in RFC1035 starting at the current
  *  offset within the buffer.
@@ -554,7 +559,9 @@ void          ares_queue_notify_empty(ares_channel_t *channel);
 #define SOCK_STATE_CALLBACK(c, s, r, w)                           \
   do {                                                            \
     if ((c)->sock_state_cb) {                                     \
+      ares__channel_unlock((c));                                  \
       (c)->sock_state_cb((c)->sock_state_cb_data, (s), (r), (w)); \
+      ares__channel_lock((c));                                    \
     }                                                             \
   } while (0)
 

src/lib/ares_search.c

@@ -82,7 +82,7 @@ static void ares_search_int(ares_channel_t *channel, const char *name,
     return;
   }
   if (s) {
-    ares_query(channel, s, dnsclass, type, callback, arg);
+    ares_query_qid(channel, s, dnsclass, type, callback, arg, NULL);
     ares_free(s);
     return;
   }
@@ -140,14 +140,14 @@ static void ares_search_int(ares_channel_t *channel, const char *name,
     /* Try the name as-is first. */
     squery->next_domain  = 0;
     squery->trying_as_is = ARES_TRUE;
-    ares_query(channel, name, dnsclass, type, search_callback, squery);
+    ares_query_qid(channel, name, dnsclass, type, search_callback, squery, NULL);
   } else {
     /* Try the name as-is last; start with the first search domain. */
     squery->next_domain  = 1;
     squery->trying_as_is = ARES_FALSE;
     status               = ares__cat_domain(name, squery->domains[0], &s);
     if (status == ARES_SUCCESS) {
-      ares_query(channel, s, dnsclass, type, search_callback, squery);
+      ares_query_qid(channel, s, dnsclass, type, search_callback, squery, NULL);
       ares_free(s);
     } else {
       /* failed, free the malloc()ed memory */
@@ -209,15 +209,15 @@ static void search_callback(void *arg, int status, int timeouts,
       } else {
         squery->trying_as_is = ARES_FALSE;
         squery->next_domain++;
-        ares_query(channel, s, squery->dnsclass, squery->type, search_callback,
-                   squery);
+        ares_query_qid(channel, s, squery->dnsclass, squery->type,
+                       search_callback, squery, NULL);
         ares_free(s);
       }
     } else if (squery->status_as_is == -1) {
       /* Try the name as-is at the end. */
       squery->trying_as_is = ARES_TRUE;
-      ares_query(channel, squery->name, squery->dnsclass, squery->type,
-                 search_callback, squery);
+      ares_query_qid(channel, squery->name, squery->dnsclass, squery->type,
+                     search_callback, squery, NULL);
     } else {
       if (squery->status_as_is == ARES_ENOTFOUND && squery->ever_got_nodata) {
         end_squery(squery, ARES_ENODATA, NULL, 0);

src/lib/ares_update_servers.c

@@ -703,8 +703,6 @@ ares_status_t ares__servers_update(ares_channel_t *channel,
     return ARES_EFORMERR;
   }
 
-  ares__channel_lock(channel);
-
   /* NOTE: a NULL or zero entry server list is considered valid due to
    *       real-world people needing support for this for their test harnesses
    */
@@ -773,7 +771,6 @@ ares_status_t ares__servers_update(ares_channel_t *channel,
   status = ARES_SUCCESS;
 
 done:
-  ares__channel_unlock(channel);
   return status;
 }
 
@@ -1044,8 +1041,9 @@ int ares_set_servers(ares_channel_t              *channel,
     return (int)status;
   }
 
-  /* NOTE: lock is in ares__servers_update() */
+  ares__channel_lock(channel);
   status = ares__servers_update(channel, slist, ARES_TRUE);
+  ares__channel_unlock(channel);
 
   ares__llist_destroy(slist);
 
@@ -1067,8 +1065,9 @@ int ares_set_servers_ports(ares_channel_t                   *channel,
     return (int)status;
   }
 
-  /* NOTE: lock is in ares__servers_update() */
+  ares__channel_lock(channel);
   status = ares__servers_update(channel, slist, ARES_TRUE);
+  ares__channel_unlock(channel);
 
   ares__llist_destroy(slist);
 
@@ -1086,8 +1085,6 @@ static ares_status_t set_servers_csv(ares_channel_t *channel, const char *_csv)
     return ARES_ENODATA;
   }
 
-  /* NOTE: lock is in ares__servers_update() */
-
   if (ares_strlen(_csv) == 0) {
     /* blank all servers */
     return (ares_status_t)ares_set_servers_ports(channel, NULL);
@@ -1099,8 +1096,9 @@ static ares_status_t set_servers_csv(ares_channel_t *channel, const char *_csv)
     return status;
   }
 
-  /* NOTE: lock is in ares__servers_update() */
+  ares__channel_lock(channel);
   status = ares__servers_update(channel, slist, ARES_TRUE);
+  ares__channel_unlock(channel);
 
   ares__llist_destroy(slist);
 
@@ -1110,13 +1108,11 @@ static ares_status_t set_servers_csv(ares_channel_t *channel, const char *_csv)
 /* We'll go ahead and honor ports anyhow */
 int ares_set_servers_csv(ares_channel_t *channel, const char *_csv)
 {
-  /* NOTE: lock is in ares__servers_update() */
   return (int)set_servers_csv(channel, _csv);
 }
 
 int ares_set_servers_ports_csv(ares_channel_t *channel, const char *_csv)
 {
-  /* NOTE: lock is in ares__servers_update() */
   return (int)set_servers_csv(channel, _csv);
 }