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

docs/ares_init_options.3

@@ -227,7 +227,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_getaddrinfo.c

@@ -675,20 +675,20 @@ static ares_bool_t next_dns_lookup(struct host_query *hquery)
   switch (hquery->hints.ai_family) {
     case AF_INET:
       hquery->remaining += 1;
-      ares_query_dnsrec(hquery->channel, name, ARES_CLASS_IN, ARES_REC_TYPE_A,
+      ares_query_nolock(hquery->channel, name, ARES_CLASS_IN, ARES_REC_TYPE_A,
                         host_callback, hquery, &hquery->qid_a);
       break;
     case AF_INET6:
       hquery->remaining += 1;
-      ares_query_dnsrec(hquery->channel, name, ARES_CLASS_IN,
+      ares_query_nolock(hquery->channel, name, ARES_CLASS_IN,
                         ARES_REC_TYPE_AAAA, host_callback, hquery,
                         &hquery->qid_aaaa);
       break;
     case AF_UNSPEC:
       hquery->remaining += 2;
-      ares_query_dnsrec(hquery->channel, name, ARES_CLASS_IN, ARES_REC_TYPE_A,
+      ares_query_nolock(hquery->channel, name, ARES_CLASS_IN, ARES_REC_TYPE_A,
                         host_callback, hquery, &hquery->qid_a);
-      ares_query_dnsrec(hquery->channel, name, ARES_CLASS_IN,
+      ares_query_nolock(hquery->channel, name, ARES_CLASS_IN,
                         ARES_REC_TYPE_AAAA, host_callback, hquery,
                         &hquery->qid_aaaa);
       break;

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_dnsrec(aquery->channel, name, ARES_CLASS_IN,
+        ares_query_nolock(aquery->channel, name, ARES_CLASS_IN,
                           ARES_REC_TYPE_PTR, addr_callback, aquery, NULL);
         ares_free(name);
         return;

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

@@ -490,7 +490,6 @@ int ares_dup(ares_channel_t **dest, const 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);
@@ -509,6 +508,7 @@ int ares_dup(ares_channel_t **dest, const 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;
@@ -524,7 +524,7 @@ int ares_dup(ares_channel_t **dest, const 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
@@ -556,7 +556,6 @@ int ares_dup(ares_channel_t **dest, const ares_channel_t *src)
 
   rc = ARES_SUCCESS;
 done:
-  ares__channel_unlock(src);
   return (int)rc; /* everything went fine */
 }
 

src/lib/ares_private.h

@@ -556,6 +556,26 @@ 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_query_dnsrec() except does not take a channel lock.  Use this
+ * if a channel lock is already held */
+ares_status_t ares_query_nolock(ares_channel_t *channel, const char *name,
+                                ares_dns_class_t     dnsclass,
+                                ares_dns_rec_type_t  type,
+                                ares_callback_dnsrec callback, void *arg,
+                                unsigned short *qid);
+
+/* Same as ares_send_dnsrec() except does not take a channel lock.  Use this
+ * if a channel lock is already held */
+ares_status_t ares_send_nolock(ares_channel_t          *channel,
+                               const ares_dns_record_t *dnsrec,
+                               ares_callback_dnsrec     callback,
+                               void *arg, unsigned short *qid);
+
+/* 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.
@@ -615,7 +635,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_query.c

@@ -64,11 +64,11 @@ static void ares_query_dnsrec_cb(void *arg, ares_status_t status,
   ares_free(qquery);
 }
 
-static ares_status_t ares_query_int(ares_channel_t *channel, const char *name,
-                                    ares_dns_class_t     dnsclass,
-                                    ares_dns_rec_type_t  type,
-                                    ares_callback_dnsrec callback, void *arg,
-                                    unsigned short *qid)
+ares_status_t ares_query_nolock(ares_channel_t *channel, const char *name,
+                                ares_dns_class_t     dnsclass,
+                                ares_dns_rec_type_t  type,
+                                ares_callback_dnsrec callback, void *arg,
+                                unsigned short *qid)
 {
   ares_status_t            status;
   ares_dns_record_t       *dnsrec = NULL;
@@ -107,7 +107,7 @@ static ares_status_t ares_query_int(ares_channel_t *channel, const char *name,
   qquery->arg      = arg;
 
   /* Send it off.  qcallback will be called when we get an answer. */
-  status = ares_send_dnsrec(channel, dnsrec, ares_query_dnsrec_cb, qquery, qid);
+  status = ares_send_nolock(channel, dnsrec, ares_query_dnsrec_cb, qquery, qid);
 
   ares_dns_record_destroy(dnsrec);
   return status;
@@ -126,7 +126,7 @@ ares_status_t ares_query_dnsrec(ares_channel_t *channel, const char *name,
   }
 
   ares__channel_lock(channel);
-  status = ares_query_int(channel, name, dnsclass, type, callback, arg, qid);
+  status = ares_query_nolock(channel, name, dnsclass, type, callback, arg, qid);
   ares__channel_unlock(channel);
   return status;
 }

src/lib/ares_search.c

@@ -97,7 +97,7 @@ static ares_status_t ares_search_next(ares_channel_t      *channel,
   }
 
   status =
-    ares_send_dnsrec(channel, squery->dnsrec, search_callback, squery, NULL);
+    ares_send_nolock(channel, squery->dnsrec, search_callback, squery, NULL);
 
   if (status != ARES_EFORMERR) {
     *skip_cleanup = ARES_TRUE;

src/lib/ares_send.c

@@ -48,10 +48,10 @@ static unsigned short generate_unique_qid(ares_channel_t *channel)
   return id;
 }
 
-static ares_status_t ares_send_dnsrec_int(ares_channel_t          *channel,
-                                          const ares_dns_record_t *dnsrec,
-                                          ares_callback_dnsrec     callback,
-                                          void *arg, unsigned short *qid)
+ares_status_t ares_send_nolock(ares_channel_t          *channel,
+                               const ares_dns_record_t *dnsrec,
+                               ares_callback_dnsrec     callback,
+                               void *arg, unsigned short *qid)
 {
   struct query            *query;
   size_t                   packetsz;
@@ -157,7 +157,7 @@ ares_status_t ares_send_dnsrec(ares_channel_t          *channel,
 
   ares__channel_lock(channel);
 
-  status = ares_send_dnsrec_int(channel, dnsrec, callback, arg, qid);
+  status = ares_send_nolock(channel, dnsrec, callback, arg, qid);
 
   ares__channel_unlock(channel);
 

src/lib/ares_update_servers.c

@@ -711,8 +711,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
    */
@@ -781,7 +779,6 @@ ares_status_t ares__servers_update(ares_channel_t *channel,
   status = ARES_SUCCESS;
 
 done:
-  ares__channel_unlock(channel);
   return status;
 }
 
@@ -1109,8 +1106,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);
 
@@ -1132,8 +1130,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);
 
@@ -1151,11 +1150,12 @@ 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__servers_update(channel, NULL, ARES_TRUE);
+    ares__channel_lock(channel);
+    status = ares__servers_update(channel, NULL, ARES_TRUE);
+    ares__channel_unlock(channel);
+    return status;
   }
 
   status = ares__sconfig_append_fromstr(&slist, _csv, ARES_FALSE);
@@ -1164,8 +1164,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);
 
@@ -1175,13 +1176,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);
 }