ares_destroy() deadlock during cleanup of config change monitoring if a configuration change comes in during shutdown

src/lib/ares_destroy.c

@@ -42,8 +42,17 @@ void ares_destroy(ares_channel_t *channel)
     return;
   }
 
-  /* Disable configuration change monitoring */
+  /* Mark as being shutdown */
   ares__channel_lock(channel);
+  channel->sys_up = ARES_FALSE;
+  ares__channel_unlock(channel);
+
+  /* Disable configuration change monitoring.  We can't hold a lock because
+   * some cleanup routines, such as on Windows, are synchronous operations.
+   * What we've observed is a system config change event was triggered right
+   * at shutdown time and it tries to take the channel lock and the destruction
+   * waits for that event to complete before it continues so we get a channel
+   * lock deadlock at shutdown if we hold a lock during this process. */
   if (channel->optmask & ARES_OPT_EVENT_THREAD) {
     ares_event_thread_t *e = channel->sock_state_cb_data;
     if (e && e->configchg) {
@@ -51,7 +60,6 @@ void ares_destroy(ares_channel_t *channel)
       e->configchg = NULL;
     }
   }
-  ares__channel_unlock(channel);
 
   /* Wait for reinit thread to exit if there was one pending, can't be
    * holding a lock as the thread may take locks. */
@@ -61,7 +69,8 @@ void ares_destroy(ares_channel_t *channel)
     channel->reinit_thread = NULL;
   }
 
-  /* Lock because callbacks will be triggered */
+  /* Lock because callbacks will be triggered, and any system-generated
+   * callbacks need to hold a channel lock. */
   ares__channel_lock(channel);
 
   /* Destroy all queries */

src/lib/ares_init.c

@@ -287,6 +287,9 @@ int ares_init_options(ares_channel_t           **channelptr,
     return ARES_ENOMEM;
   }
 
+  /* We are in a good state */
+  channel->sys_up = ARES_TRUE;
+
   /* One option where zero is valid, so set default value here */
   channel->ndots = 1;
 
@@ -443,8 +446,9 @@ ares_status_t ares_reinit(ares_channel_t *channel)
 
   ares__channel_lock(channel);
 
-  /* If a reinit is already in process, lets not do it again */
-  if (channel->reinit_pending) {
+  /* If a reinit is already in process, lets not do it again. Or if we are
+   * shutting down, skip. */
+  if (!channel->sys_up || channel->reinit_pending) {
     ares__channel_unlock(channel);
     return ARES_SUCCESS;
   }

src/lib/ares_private.h

@@ -345,6 +345,12 @@ struct ares_channeldata {
    * exit. */
   ares_bool_t                         reinit_pending;
   ares__thread_t                     *reinit_thread;
+
+  /* Whether the system is up or not.  This is mainly to prevent deadlocks
+   * and access violations during the cleanup process.  Some things like
+   * system config changes might get triggered and we need a flag to make
+   * sure we don't take action. */
+  ares_bool_t                         sys_up;
 };
 
 /* Does the domain end in ".onion" or ".onion."? Case-insensitive. */