Tests: fix test cleanup race condition (#803)

There was a thread passed data for processing that was cleaned up before
thread exit, and it could cause a use-after-free in the test suite. This
doesn't affect c-ares. This was found during trying to reproduce #798,
but appears unrelated, don't use a helper thread as it isn't necessary.

Fix By: Brad House (@bradh352)

test/ares-test-mock-et.cc

@@ -789,8 +789,46 @@ TEST_P(MockEventThreadTest, PartialQueryCancel) {
   EXPECT_TRUE(result.done_);
   EXPECT_EQ(ARES_ECANCELLED, result.status_);
 }
+
+// Test case for Issue #798, we're really looking for a crash, the results
+// don't matter.  Should either be successful or canceled.
+TEST_P(MockEventThreadTest, BulkCancel) {
+  std::vector<byte> nothing;
+  DNSPacket reply;
+  reply.set_response().set_aa()
+    .add_question(new DNSQuestion("www.google.com", T_A))
+    .add_answer(new DNSARR("www.google.com", 0x0100, {0x01, 0x02, 0x03, 0x04}));
+
+  ON_CALL(server_, OnRequest("www.google.com", T_A))
+    .WillByDefault(SetReply(&server_, &reply));
+
+#define BULKCANCEL_LOOP 5
+#define BULKCANCEL_CNT 50
+  for (size_t l = 0; l<BULKCANCEL_LOOP; l++) {
+    HostResult result[BULKCANCEL_CNT];
+    for (size_t i = 0; i<BULKCANCEL_CNT; i++) {
+      ares_gethostbyname(channel_, "www.google.com.", AF_INET, HostCallback, &result[i]);
+    }
+    // After 1ms, issues ares_cancel(), there should be queries outstanding that
+    // are cancelled.
+    Process(1);
+
+    size_t success_cnt = 0;
+    size_t cancel_cnt = 0;
+    for (size_t i = 0; i<BULKCANCEL_CNT; i++) {
+      EXPECT_TRUE(result[i].done_);
+      EXPECT_TRUE(result[i].status_ == ARES_ECANCELLED || result[i].status_ == ARES_SUCCESS);
+      if (result[i].status_ == ARES_SUCCESS)
+        success_cnt++;
+      if (result[i].status_ == ARES_ECANCELLED)
+        cancel_cnt++;
+    }
+    if (verbose) std::cerr << "success: " << success_cnt << ", cancel: " << cancel_cnt << std::endl;
+  }
+}
 #endif
 
+
 TEST_P(MockEventThreadTest, UnspecifiedFamilyV6) {
   DNSPacket rsp6;
   rsp6.set_response().set_aa()

test/ares-test.cc

@@ -874,39 +874,37 @@ void MockChannelOptsTest::Process(unsigned int cancel_ms) {
               cancel_ms);
 }
 
-void MockEventThreadOptsTest::ProcessThread() {
+void MockEventThreadOptsTest::Process(unsigned int cancel_ms) {
   std::set<ares_socket_t> fds;
 
 #ifndef CARES_SYMBOL_HIDING
-  bool has_cancel_ms = false;
+  bool has_cancel_ms = (cancel_ms > 0)?true:false;
   ares_timeval_t tv_begin;
   ares_timeval_t tv_cancel;
 #endif
 
-  mutex.lock();
-
-  while (isup) {
-    int nfds = 0;
-    fd_set readers;
 #ifndef CARES_SYMBOL_HIDING
-    ares_timeval_t tv_now;
-    ares_timeval_t atv_remaining;
+  ares_timeval_t tv_now;
+  ares_timeval_t atv_remaining;
 
-    ares__tvnow(&tv_now);
-    if (cancel_ms_ && !has_cancel_ms) {
-      ares__tvnow(&tv_begin);
-      memcpy(&tv_cancel, &tv_begin, sizeof(tv_cancel));
-      if (verbose) std::cerr << "ares_cancel will be called after " << cancel_ms_ << "ms" << std::endl;
-      tv_cancel.sec  += (cancel_ms_ / 1000);
-      tv_cancel.usec += ((cancel_ms_ % 1000) * 1000);
-      has_cancel_ms = true;
-    }
+  if (has_cancel_ms) {
+    ares__tvnow(&tv_begin);
+    memcpy(&tv_cancel, &tv_begin, sizeof(tv_cancel));
+    if (verbose) std::cerr << "ares_cancel will be called after " << cancel_ms << "ms" << std::endl;
+    tv_cancel.sec  += (cancel_ms / 1000);
+    tv_cancel.usec += ((cancel_ms % 1000) * 1000);
+  }
 #else
-    if (cancel_ms_) {
-      std::cerr << "library built with symbol hiding, can't test with cancel support" << std::endl;
-      return;
-    }
+  if (cancel_ms) {
+    std::cerr << "library built with symbol hiding, can't test with cancel support" << std::endl;
+    return;
+  }
 #endif
+
+  while (ares_queue_active_queries(channel_)) {
+    int nfds = 0;
+    fd_set readers;
+
     struct timeval  tv;
 
     /* c-ares is using its own event thread, so we only need to monitor the
@@ -920,9 +918,15 @@ void MockEventThreadOptsTest::ProcessThread() {
       }
     }
 
+    /* We just always wait 20ms then recheck. Not doing any complex signaling. */
+    tv.tv_sec  = 0;
+    tv.tv_usec = 20000;
+
 #ifndef CARES_SYMBOL_HIDING
+    ares__tvnow(&tv_now);
+
+    unsigned int remaining_ms = 0;
     if (has_cancel_ms) {
-      unsigned int remaining_ms;
       ares__timeval_remaining(&atv_remaining,
                               &tv_now,
                               &tv_cancel);
@@ -930,17 +934,16 @@ void MockEventThreadOptsTest::ProcessThread() {
       if (remaining_ms == 0) {
         if (verbose) std::cerr << "Issuing ares_cancel()" << std::endl;
         ares_cancel(channel_);
-        cancel_ms_ = 0; /* Disable issuing cancel again */
+        cancel_ms = 0; /* Disable issuing cancel again */
         has_cancel_ms = false;
       }
     }
-#endif
 
-    /* We just always wait 20ms then recheck. Not doing any complex signaling. */
-    tv.tv_sec  = 0;
-    tv.tv_usec = 20000;
+    if (has_cancel_ms && remaining_ms < 20) {
+      tv.tv_usec = (int)remaining_ms * 1000;
+    }
+#endif
 
-    mutex.unlock();
     if (select(nfds, &readers, nullptr, nullptr, &tv) < 0) {
       fprintf(stderr, "select() failed, errno %d\n", errno);
       return;
@@ -952,10 +955,7 @@ void MockEventThreadOptsTest::ProcessThread() {
         ProcessFD(fd);
       }
     }
-    mutex.lock();
   }
-  mutex.unlock();
-
 }
 
 std::ostream& operator<<(std::ostream& os, const HostResult& result) {

test/ares-test.h

@@ -369,17 +369,10 @@ public:
                           FillOptionsET(&evopts_, givenopts, evsys),
                           optmask | ARES_OPT_EVENT_THREAD)
   {
-    cancel_ms_ = 0;
-    isup       = true;
-    thread     = std::thread(&MockEventThreadOptsTest::ProcessThread, this);
   }
 
   ~MockEventThreadOptsTest()
   {
-    mutex.lock();
-    isup = false;
-    mutex.unlock();
-    thread.join();
   }
 
   static struct ares_options *FillOptionsET(struct ares_options *opts,
@@ -395,21 +388,9 @@ public:
     return opts;
   }
 
-  void Process(unsigned int cancel_ms = 0)
-  {
-    mutex.lock();
-    cancel_ms_ = cancel_ms;
-    mutex.unlock();
-    ares_queue_wait_empty(channel_, -1);
-  }
-
+  void Process(unsigned int cancel_ms = 0);
 private:
-  void                ProcessThread();
   struct ares_options evopts_;
-  unsigned int        cancel_ms_;
-  bool                isup;
-  std::mutex          mutex;
-  std::thread         thread;
 };
 
 class MockEventThreadTest