From 61262a0f08097718e566c0aa7e2878b1d868f3ed Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Wed, 12 Nov 2014 20:16:18 +0300 Subject: 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"). --- evdns.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/evdns.c b/evdns.c index fdffbd70..177d56c1 100644 --- 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); -- cgit v1.2.1