From b6d19fca92fecc9d0f3495012ecfddb6e7f2ed25 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Fri, 25 Oct 2019 21:37:32 +0200 Subject: [PATCH] dnsdist: Increment the DOHUnit ref count when it's set in the IDState We need to increment the reference counter even before sending the query to the backend, as soon as we copy a reference into the IDState. Because: - that makes sense anyway, we are storing a new copy ; - otherwise, in the unlikely event where we reuse the IDState before the query has been sent to the backend we might free the DOHUnit before the reference counter has been incremented and cause a double-free. --- pdns/dnsdist.cc | 6 ++++-- pdns/dnsdistdist/doh.cc | 36 +++++++++++++++++++++++++++--------- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc index 955743971..d9a149df5 100644 --- a/pdns/dnsdist.cc +++ b/pdns/dnsdist.cc @@ -616,8 +616,10 @@ try { du->response = std::string(response, responseLen); if (send(du->rsock, &du, sizeof(du), 0) != sizeof(du)) { /* at this point we have the only remaining pointer on this - DOHUnit object since we did set ids->du to nullptr earlier */ - delete du; + DOHUnit object since we did set ids->du to nullptr earlier, + except if we got the response before the pointer could be + released by the frontend */ + du->release(); } #endif /* HAVE_DNS_OVER_HTTPS */ du = nullptr; diff --git a/pdns/dnsdistdist/doh.cc b/pdns/dnsdistdist/doh.cc index bff3ef6a1..923f9eb81 100644 --- a/pdns/dnsdistdist/doh.cc +++ b/pdns/dnsdistdist/doh.cc @@ -349,11 +349,11 @@ static void handleResponse(DOHFrontend& df, st_h2o_req_t* req, uint16_t statusCo this function calls 'return -1' to drop a query without sending it caller should make sure HTTPS thread hears of that */ - static int processDOHQuery(DOHUnit* du) { uint16_t queryId = 0; ComboAddress remote; + bool duRefCountIncremented = false; try { if(!du->req) { // we got closed meanwhile. XXX small race condition here @@ -466,6 +466,9 @@ static int processDOHQuery(DOHUnit* du) } ids->origFD = 0; + /* increase the ref count since we are about to store the pointer */ + du->get(); + duRefCountIncremented = true; ids->du = du; ids->cs = &cs; @@ -491,18 +494,17 @@ static int processDOHQuery(DOHUnit* du) int fd = pickBackendSocketForSending(ss); try { - /* increase the ref count since we are about to send the pointer */ - du->get(); /* you can't touch du after this line, because it might already have been freed */ ssize_t ret = udpClientSendRequestToBackend(ss, fd, query, dq.len); if(ret < 0) { - du->release(); /* we are about to handle the error, make sure that this pointer is not accessed when the state is cleaned, but first check that it still belongs to us */ if (ids->tryMarkUnused(generation)) { ids->du = nullptr; + du->release(); + duRefCountIncremented = false; --ss->outstanding; } ++ss->sendErrors; @@ -512,7 +514,9 @@ static int processDOHQuery(DOHUnit* du) } } catch (const std::exception& e) { - du->release(); + if (duRefCountIncremented) { + du->release(); + } throw; } @@ -527,6 +531,7 @@ static int processDOHQuery(DOHUnit* du) return 0; } +/* called when a HTTP response is about to be sent */ static void on_response_ready_cb(struct st_h2o_filter_t *self, h2o_req_t *req, h2o_ostream_t **slot) { if (req == nullptr) { @@ -599,6 +604,8 @@ static void on_generator_dispose(void *_self) } } +/* We allocate a DOHUnit and send it to dnsdistclient() function in the doh client thread + via a pipe */ static void doh_dispatch_query(DOHServerConfig* dsc, h2o_handler_t* self, h2o_req_t* req, std::string&& query, const ComboAddress& local, const ComboAddress& remote) { try { @@ -633,8 +640,9 @@ static void doh_dispatch_query(DOHServerConfig* dsc, h2o_handler_t* self, h2o_re } /* - For GET, the base64url-encoded payload is in the 'dns' parameter, which might be the first parameter, or not. - For POST, the payload is the payload. + A query has been parsed by h2o. + For GET, the base64url-encoded payload is in the 'dns' parameter, which might be the first parameter, or not. + For POST, the payload is the payload. */ static int doh_handler(h2o_handler_t *self, h2o_req_t *req) try @@ -884,7 +892,10 @@ void DOHUnit::setHTTPResponse(uint16_t statusCode, const std::string& body_, con contentType = contentType_; } -void dnsdistclient(int qsock, int rsock) +/* query has been parsed by h2o, which called doh_handler() in the main DoH thread. + In order not to blockfor long, doh_handler() called doh_dispatch_query() which allocated + a DOHUnit object and passed it to us */ +static void dnsdistclient(int qsock, int rsock) { setThreadName("dnsdist/doh-cli"); @@ -936,7 +947,13 @@ void dnsdistclient(int qsock, int rsock) } } -// called if h2o finds that dnsdist gave us an answer +/* called if h2o finds that dnsdist gave us an answer by writing into + the dohresponsepair[0] side of the pipe so from: + - handleDOHTimeout() when we did not get a response fast enough (called + either from the health check thread (active) or from the frontend ones (reused)) + - dnsdistclient (error 500 because processDOHQuery() returned a negative value) + - processDOHQuery (self-answered queries) + */ static void on_dnsdist(h2o_socket_t *listener, const char *err) { DOHUnit *du = nullptr; @@ -964,6 +981,7 @@ static void on_dnsdist(h2o_socket_t *listener, const char *err) du->release(); } +/* called when a TCP connection has been accepted, the TLS session has not been established */ static void on_accept(h2o_socket_t *listener, const char *err) { DOHServerConfig* dsc = reinterpret_cast(listener->data); -- 2.40.0