evdns: avoid read-after-free in evdns_request_timeout_callback()

In evdns_request_timeout_callback() in case we a giving up, we call
request_finished() which will free() req structure, however we ns from
it to fail it, so save pointer to ns to call nameserver_failed() on
them.

Founded with valgrind:
$ valgrind regress dns/retry
==10497== Memcheck, a memory error detector
==10497== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==10497== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==10497== Command: regress dns/retry
==10497==
dns/retry: [forking] ==10498== Invalid read of size 8
==10498==    at 0x4C309D: evdns_request_timeout_callback (evdns.c:2179)
==10498==    by 0x49EA95: event_process_active_single_queue (event.c:1576)
==10498==    by 0x49EFDD: event_process_active (event.c:1668)
==10498==    by 0x49F6DD: event_base_loop (event.c:1891)
==10498==    by 0x49F063: event_base_dispatch (event.c:1702)
==10498==    by 0x44C7F1: dns_retry_test_impl (regress_dns.c:724)
==10498==    by 0x44CF60: dns_retry_test (regress_dns.c:749)
==10498==    by 0x48A8A1: testcase_run_bare_ (tinytest.c:105)
==10498==    by 0x48A94E: testcase_run_forked_ (tinytest.c:189)
==10498==    by 0x48AB73: testcase_run_one (tinytest.c:247)
==10498==    by 0x48B4C2: tinytest_main (tinytest.c:434)
==10498==    by 0x477FC7: main (regress_main.c:459)
==10498==  Address 0x6176ef8 is 40 bytes inside a block of size 342 free'd
==10498==    at 0x4C29E90: free (vg_replace_malloc.c:473)
==10498==    by 0x4A4411: event_mm_free_ (event.c:3443)
==10498==    by 0x4BE8C5: request_finished (evdns.c:702)
==10498==    by 0x4C3098: evdns_request_timeout_callback (evdns.c:2178)
==10498==    by 0x49EA95: event_process_active_single_queue (event.c:1576)
==10498==    by 0x49EFDD: event_process_active (event.c:1668)
==10498==    by 0x49F6DD: event_base_loop (event.c:1891)
==10498==    by 0x49F063: event_base_dispatch (event.c:1702)
==10498==    by 0x44C7F1: dns_retry_test_impl (regress_dns.c:724)
==10498==    by 0x44CF60: dns_retry_test (regress_dns.c:749)
==10498==    by 0x48A8A1: testcase_run_bare_ (tinytest.c:105)
==10498==    by 0x48A94E: testcase_run_forked_ (tinytest.c:189)
==10498==
==10498==
==10498== HEAP SUMMARY:
==10498==     in use at exit: 0 bytes in 0 blocks
==10498==   total heap usage: 83 allocs, 83 frees, 10,020 bytes allocated
==10498==
==10498== All heap blocks were freed -- no leaks are possible
==10498==
==10498== For counts of detected and suppressed errors, rerun with: -v
==10498== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
OK
1 tests ok.  (0 skipped)
==10497==
==10497== HEAP SUMMARY:
==10497==     in use at exit: 0 bytes in 0 blocks
==10497==   total heap usage: 3 allocs, 3 frees, 96 bytes allocated
==10497==
==10497== All heap blocks were freed -- no leaks are possible
==10497==
==10497== For counts of detected and suppressed errors, rerun with: -v
==10497== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Bug was introduced in 97c750d6602517f22a1100f16592b421c38f2a45 ("evdns:
fail ns after we are failing/retrasmitting request").

diff --git a/evdns.c b/evdns.c
index fdffbd70a2b125d85fff3250b37f27d7bf4429bc..177d56c1d69e762a134a1a8aa1596cfdef691bec 100644 (file)
--- a/evdns.c
+++ b/evdns.c
@@ -2175,7 +2175,10 @@ evdns_request_timeout_callback(evutil_socket_t fd, short events, void *arg) {
                log(EVDNS_LOG_DEBUG, "Giving up on request %p; tx_count==%d",
                    arg, req->tx_count);
                reply_schedule_callback(req, 0, DNS_ERR_TIMEOUT, NULL);
+
+               struct nameserver *ns = req->ns;
                request_finished(req, &REQ_HEAD(req->base, req->trans_id), 1);
+               nameserver_failed(ns, "request timed out.");
        } else {
                /* retransmit it */
                log(EVDNS_LOG_DEBUG, "Retransmitting request %p; tx_count==%d",
@@ -2183,12 +2186,12 @@ evdns_request_timeout_callback(evutil_socket_t fd, short events, void *arg) {
                (void) evtimer_del(&req->timeout_event);
                request_swap_ns(req, nameserver_pick(base));
                evdns_request_transmit(req);
-       }
 
-       req->ns->timedout++;
-       if (req->ns->timedout > req->base->global_max_nameserver_timeout) {
-               req->ns->timedout = 0;
-               nameserver_failed(req->ns, "request timed out.");
+               req->ns->timedout++;
+               if (req->ns->timedout > req->base->global_max_nameserver_timeout) {
+                       req->ns->timedout = 0;
+                       nameserver_failed(req->ns, "request timed out.");
+               }
        }
 
        EVDNS_UNLOCK(base);