From fc57af01749239b6735f950a2b9a02b2e059f182 Mon Sep 17 00:00:00 2001 From: Brad House Date: Sun, 23 Jun 2024 12:45:06 -0400 Subject: [PATCH] thread deadlock: make sure channel lock isn't used recursively --- docs/ares_init_options.3 | 3 ++- src/lib/ares__threads.c | 14 +------------- src/lib/ares_gethostbyaddr.c | 10 +++++----- src/lib/ares_getnameinfo.c | 11 ++++++----- src/lib/ares_init.c | 5 ++--- src/lib/ares_private.h | 7 +++++++ src/lib/ares_search.c | 14 +++++++------- src/lib/ares_update_servers.c | 16 ++++++---------- 8 files changed, 36 insertions(+), 44 deletions(-) diff --git a/docs/ares_init_options.3 b/docs/ares_init_options.3 index 33bca309..e32436ff 100644 --- a/docs/ares_init_options.3 +++ b/docs/ares_init_options.3 @@ -217,7 +217,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 diff --git a/src/lib/ares__threads.c b/src/lib/ares__threads.c index 6cb6fccc..e4e2a6a5 100644 --- a/src/lib/ares__threads.c +++ b/src/lib/ares__threads.c @@ -145,30 +145,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; } diff --git a/src/lib/ares_gethostbyaddr.c b/src/lib/ares_gethostbyaddr.c index ab54706b..6b455509 100644 --- a/src/lib/ares_gethostbyaddr.c +++ b/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': diff --git a/src/lib/ares_getnameinfo.c b/src/lib/ares_getnameinfo.c index 003e4cf3..eb358dc0 100644 --- a/src/lib/ares_getnameinfo.c +++ b/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); } } } diff --git a/src/lib/ares_init.c b/src/lib/ares_init.c index 40a85258..7e0077ac 100644 --- a/src/lib/ares_init.c +++ b/src/lib/ares_init.c @@ -422,7 +422,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); @@ -441,6 +440,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; @@ -454,7 +454,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 @@ -486,7 +486,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 */ } diff --git a/src/lib/ares_private.h b/src/lib/ares_private.h index 018f8b80..08ea512c 100644 --- a/src/lib/ares_private.h +++ b/src/lib/ares_private.h @@ -490,6 +490,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. @@ -539,7 +544,9 @@ ares_status_t ares__dns_name_write(ares__buf_t *buf, ares__llist_t **list, #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) diff --git a/src/lib/ares_search.c b/src/lib/ares_search.c index 429c7e1d..971275b4 100644 --- a/src/lib/ares_search.c +++ b/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); diff --git a/src/lib/ares_update_servers.c b/src/lib/ares_update_servers.c index 33d45088..9421e5f8 100644 --- a/src/lib/ares_update_servers.c +++ b/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); }