Deadlock in ares_queue_wait_empty

[johnthacker]

c-ares/src/lib/ares__threads.c

Lines 571 to 589 in fd81f36

	while (ares__llist_len(channel->all_queries)) {
	if (timeout_ms < 0) {
	ares__thread_cond_wait(channel->cond_empty, channel->lock);
	} else {
	struct timeval tv_remaining;
	struct timeval tv_now = ares__tvnow();
	unsigned long tms;
	ares__timeval_remaining(&tv_remaining, &tv_now, &tout);
	tms = (unsigned long)((tv_remaining.tv_sec * 1000) +
	(tv_remaining.tv_usec / 1000));
	if (tms == 0) {
	status = ARES_ETIMEOUT;
	} else {
	status =
	ares__thread_cond_timedwait(channel->cond_empty, channel->lock, tms);
	}
	}
	}

There's nothing to cancel or timeout or server the pending requests and break out of the loop in the tms == 0 case. If the timeout is short enough that it expires while, e.g., waiting to obtain the mutex, then while the status is set to ARES_ETIMEOUT, the mutex isn't released (unlike the other branches which call ares__thread_cond_timedwait, which releases the mutex while waiting), and so the event thread when calling ares_process_fd cannot obtain the channel lock, channel->all_queries never decreases and the while loop never exits.

I believe the intention is to simply return ARES_ETIMEOUT and let the caller decide to perhaps call ares_cancel, though I'm not sure.

[bradh352]

you're right, it looks like when timeout_ms > 0 its missing logic to break out of the loop when a timeout condition is actually hit and will only actually break out when the query count is zero.

[johnthacker]

I added a break statement after line 583 in a local fork and that seems to work for me (releases the mutex, returns ARES_ETIMEOUT, then it's the caller's job to call ares_cancel presumably.) Happy to submit a PR.

[bradh352]

proper line was 587, just committed that and backported to the relevant branches, thanks for the report!