Make evdns_cancel_request() safe for canceling requests with scheduled-to-run or running callbacks.

[stablebits]

Previously, evdns_base_clear_nameservers_and_suspend() would call evdns_cancel_request() to cancel a probe request (server->probe_request) before freeing a nameserver object (server). However, if the request's callback was already scheduled by the event loop (handle->pending_cb is set), evdns_cancel_request() would not cancel it effectively. The callback would run and access the freed nameserver object.

[azat]

Hm, some tests failed, can you please take a look?

[stablebits]

Hm, some tests failed, can you please take a look?

@azat the problem is that reply_run_callback() is now accessing handle->base, which is struct evdns_base *base. And test cases free evdns_base while leaving callbacks pending on the queue. For example,

2375     evdns_base_free(env->dns_base, send_err_shutdown);
2376     env->dns_base = 0;
2377     
2378     /* `req` is freed in callback, that's why one loop is required */
2379     event_base_loop(env->base, EVLOOP_NONBLOCK);

evdns_base_free_and_unlock(struct evdns_base *base, int fail_requests) does

4983     /* TODO(nickm) we might need to refcount here. */
4984 
4985     while (base->req_waiting_head) {
4986         if (fail_requests)
4987             reply_schedule_callback(base->req_waiting_head, 0, DNS_ERR_SHUTDOWN, NULL);
4988         request_finished(base->req_waiting_head, &base->req_waiting_head, 1);
4989     }

hm, maybe that's why this "we might need to refcount here" comment is here.

In principal, there is a problem in the existing code too. nameserver_probe_callback(), which is called by reply_run_callback(), already accesses evdns_base through ns->base.

I'll see how to address this. Suggestions are of course welcome.

Updated

On the other hand, there is this code https://github.com/libevent/libevent/blob/master/evdns.c#L5344 handling DNS_ERR_SHUTDOWN case explicitly.

Hmm, how outrageous would it be to schedule a delayed destruction of struct evdns_base *base by the event loop? Like adding a delete-function as the last callback after the ones created by reply_schedule_callback(base->req_heads[i], 0, DNS_ERR_SHUTDOWN, NULL);. Another alternative would be scheduling a single callback that would run reply_run_callback() with DNS_ERR_SHUTDOWN for all requests from req_waiting_queue and req_heads[i] and then free base.

[azat]

On the other hand, there is this code master/evdns.c#L5344 handling DNS_ERR_SHUTDOWN case explicitly.

All this is too complex...

Hmm, how outrageous would it be to schedule a delayed destruction of struct evdns_base *base by the event loop? Like adding a delete-function as the last callback after the ones created by

Not sure that it is a great idea since it may leave evdns in some corner cases (like event_base_loopexit from some callback), and will require running event_base_loop after evnds_base_free

Another alternative would be scheduling a single callback that would run reply_run_callback() with DNS_ERR_SHUTDOWN for all requests from req_waiting_queue and req_heads[i] and then free base.

This looks like the best way for now to avoid extra complexity for this patch

As a long-term goal I think we need refcnt for evdns_base to avoid use-after-free, we can do it the same way as it is done in bufferevents, decref and free check on unlock and in evdns_base_free.