From 008a912cc1efdeebc94102953fdd129ea68101df 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_getaddrinfo.c | 8 ++++---- src/lib/ares_gethostbyaddr.c | 10 +++++----- src/lib/ares_getnameinfo.c | 11 ++++++----- src/lib/ares_init.c | 5 ++--- src/lib/ares_private.h | 22 ++++++++++++++++++++++ src/lib/ares_query.c | 14 +++++++------- src/lib/ares_search.c | 2 +- src/lib/ares_send.c | 10 +++++----- src/lib/ares_update_servers.c | 21 ++++++++++----------- 11 files changed, 65 insertions(+), 55 deletions(-) diff --git a/docs/ares_init_options.3 b/docs/ares_init_options.3 index 81e53cd0..0f7cff38 100644 --- a/docs/ares_init_options.3 +++ b/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 diff --git a/src/lib/ares__threads.c b/src/lib/ares__threads.c index 39d98d22..a3ed950c 100644 --- a/src/lib/ares__threads.c +++ b/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; } diff --git a/src/lib/ares_getaddrinfo.c b/src/lib/ares_getaddrinfo.c index cfc889c7..2a0fb9a8 100644 --- a/src/lib/ares_getaddrinfo.c +++ b/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; diff --git a/src/lib/ares_gethostbyaddr.c b/src/lib/ares_gethostbyaddr.c index 45367326..c5fbb107 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_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; diff --git a/src/lib/ares_getnameinfo.c b/src/lib/ares_getnameinfo.c index 57c6ddaf..85bb23a2 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 518abd54..26975b54 100644 --- a/src/lib/ares_init.c +++ b/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 */ } diff --git a/src/lib/ares_private.h b/src/lib/ares_private.h index fd290c4d..69b9709f 100644 --- a/src/lib/ares_private.h +++ b/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) diff --git a/src/lib/ares_query.c b/src/lib/ares_query.c index 0eea80e7..a63b116b 100644 --- a/src/lib/ares_query.c +++ b/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; } diff --git a/src/lib/ares_search.c b/src/lib/ares_search.c index 4fd909cd..a72b12ff 100644 --- a/src/lib/ares_search.c +++ b/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; diff --git a/src/lib/ares_send.c b/src/lib/ares_send.c index d15a9b95..9e7c118e 100644 --- a/src/lib/ares_send.c +++ b/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); diff --git a/src/lib/ares_update_servers.c b/src/lib/ares_update_servers.c index 91e9e093..7e999420 100644 --- a/src/lib/ares_update_servers.c +++ b/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); }