Explicitly allocate ResState on the call stack
Previously known as __res_state, res_state, res or even statp, the
ResState struct holds the context of a query in progress.
After a long series of preparatory patches, the thread-local storage
nonsense that's been plaguing this fine codebase for decades is finally
gone. Sayonara! ResState is now explicitly allocated on the stack at the
beginning of each query and passed down as a regular function argument.
To keep this patch reviewable, I threw in two compatibility typedefs for
the legacy names. We'll get rid of them in a separate mechanical patch.
Next steps:
- Give ResState a regular constructor and, crucially, a real destructor
- Move the definition to ResState.h, like it's a real class
- Populate the nameservers immediately on construction
- Switch to safe C++ data structures
Change-Id: I5c0b85da3ad9a55fa41dca6c87e57fe8f50c6abc
diff --git a/Android.bp b/Android.bp
index d05881f..bc2c0cd 100644
--- a/Android.bp
+++ b/Android.bp
@@ -50,7 +50,6 @@
"res_mkquery.cpp",
"res_query.cpp",
"res_send.cpp",
- "res_state.cpp",
"res_stats.cpp",
"Dns64Configuration.cpp",
"DnsProxyListener.cpp",
diff --git a/PREUPLOAD.cfg b/PREUPLOAD.cfg
index c8dbf77..27eac94 100644
--- a/PREUPLOAD.cfg
+++ b/PREUPLOAD.cfg
@@ -1,5 +1,6 @@
[Builtin Hooks]
clang_format = true
+commit_msg_test_field = false
[Builtin Hooks Options]
clang_format = --commit ${PREUPLOAD_COMMIT} --style file --extensions c,h,cc,cpp
diff --git a/getaddrinfo.cpp b/getaddrinfo.cpp
index 4207251..46137de 100644
--- a/getaddrinfo.cpp
+++ b/getaddrinfo.cpp
@@ -56,6 +56,7 @@
#include <android-base/logging.h>
#include "netd_resolv/resolv.h"
+#include "res_init.h"
#include "resolv_cache.h"
#include "resolv_private.h"
@@ -1445,18 +1446,11 @@
return EAI_FAMILY;
}
- res_state res = res_get_state();
- if (!res) return EAI_MEMORY;
-
- /* this just sets our netid val in the thread private data so we don't have to
- * modify the api's all the way down to res_send.c's res_nsend. We could
- * fully populate the thread private data here, but if we get down there
- * and have a cache hit that would be wasted, so we do the rest there on miss
- */
- res_setnetcontext(res, netcontext, event);
+ ResState res;
+ res_init(&res, netcontext, event);
int he;
- if (res_searchN(name, &q, res, &he) < 0) {
+ if (res_searchN(name, &q, &res, &he) < 0) {
// Return h_errno (he) to catch more detailed errors rather than EAI_NODATA.
// Note that res_searchN() doesn't set the pair NETDB_INTERNAL and errno.
// See also herrnoToAiErrno().
@@ -1612,7 +1606,8 @@
LOG(DEBUG) << __func__ << ": (" << cl << ", " << type << ")";
- n = res_nmkquery(res, QUERY, name, cl, type, NULL, 0, NULL, buf, sizeof(buf));
+ n = res_nmkquery(QUERY, name, cl, type, /*data=*/nullptr, /*datalen=*/0, buf, sizeof(buf),
+ res->netcontext_flags);
if (n > 0 &&
(res->netcontext_flags &
(NET_CONTEXT_FLAG_USE_DNS_OVER_TLS | NET_CONTEXT_FLAG_USE_EDNS)) &&
diff --git a/gethnamaddr.cpp b/gethnamaddr.cpp
index 0d9683c..2a8c92b 100644
--- a/gethnamaddr.cpp
+++ b/gethnamaddr.cpp
@@ -74,6 +74,7 @@
#include "hostent.h"
#include "netd_resolv/resolv.h"
+#include "res_init.h"
#include "resolv_cache.h"
#include "resolv_private.h"
#include "stats.pb.h"
@@ -116,7 +117,7 @@
static int dns_gethtbyaddr(const unsigned char* uaddr, int len, int af,
const android_net_context* netcontext, getnamaddr* info,
NetworkDnsEventReported* event);
-static int dns_gethtbyname(const char* name, int af, getnamaddr* info);
+static int dns_gethtbyname(ResState* res, const char* name, int af, getnamaddr* info);
#define BOUNDED_INCR(x) \
do { \
@@ -393,9 +394,8 @@
NetworkDnsEventReported* event) {
getnamaddr info;
- res_state res = res_get_state();
- if (res == nullptr) return EAI_MEMORY;
- res_setnetcontext(res, netcontext, event);
+ ResState res;
+ res_init(&res, netcontext, event);
size_t size;
switch (af) {
@@ -450,7 +450,7 @@
info.buf = buf;
info.buflen = buflen;
if (_hf_gethtbyname2(name, af, &info)) {
- int error = dns_gethtbyname(name, af, &info);
+ int error = dns_gethtbyname(&res, name, af, &info);
if (error != 0) return error;
}
*result = hp;
@@ -652,7 +652,7 @@
});
}
-static int dns_gethtbyname(const char* name, int addr_type, getnamaddr* info) {
+static int dns_gethtbyname(ResState* res, const char* name, int addr_type, getnamaddr* info) {
int n, type;
info->hp->h_addrtype = addr_type;
@@ -670,9 +670,6 @@
}
auto buf = std::make_unique<querybuf>();
- res_state res = res_get_state();
- if (!res) return EAI_MEMORY;
-
int he;
n = res_nsearch(res, name, C_IN, type, buf->buf, (int)sizeof(buf->buf), &he);
if (n < 0) {
@@ -734,12 +731,10 @@
auto buf = std::make_unique<querybuf>();
- res_state res = res_get_state();
- if (!res) return EAI_MEMORY;
-
- res_setnetcontext(res, netcontext, event);
+ ResState res;
+ res_init(&res, netcontext, event);
int he;
- n = res_nquery(res, qbuf, C_IN, T_PTR, buf->buf, (int)sizeof(buf->buf), &he);
+ n = res_nquery(&res, qbuf, C_IN, T_PTR, buf->buf, (int)sizeof(buf->buf), &he);
if (n < 0) {
LOG(DEBUG) << __func__ << ": res_nquery failed (" << n << ")";
// Note that res_nquery() doesn't set the pair NETDB_INTERNAL and errno.
diff --git a/res_cache.cpp b/res_cache.cpp
index 68dc48b..bc36bef 100644
--- a/res_cache.cpp
+++ b/res_cache.cpp
@@ -140,6 +140,7 @@
* *****************************************
*/
const int CONFIG_MAX_ENTRIES = 64 * 2 * 5;
+constexpr int DNSEVENT_SUBSAMPLING_MAP_DEFAULT_KEY = -1;
static time_t _time_now(void) {
struct timeval tv;
diff --git a/res_init.cpp b/res_init.cpp
index 12f4124..a1619d9 100644
--- a/res_init.cpp
+++ b/res_init.cpp
@@ -92,17 +92,19 @@
#include "netd_resolv/resolv.h"
#include "resolv_private.h"
-// Set up Resolver state default settings.
-// Note: this is called with statp zero-initialized
-void res_init(res_state statp) {
- statp->netid = NETID_UNSET;
+void res_init(ResState* statp, const struct android_net_context* _Nonnull netcontext,
+ android::net::NetworkDnsEventReported* _Nonnull event) {
+ memset(statp, 0, sizeof *statp);
+
+ statp->netid = netcontext->dns_netid;
+ statp->uid = netcontext->uid;
statp->id = arc4random_uniform(65536);
- statp->_mark = MARK_UNSET;
+ statp->_mark = netcontext->dns_mark;
+ statp->netcontext_flags = netcontext->flags;
+ statp->event = event;
statp->ndots = 1;
statp->_vcsock = -1;
- statp->_flags = 0;
- statp->netcontext_flags = 0;
for (int ns = 0; ns < MAXNS; ns++) {
statp->nssocks[ns] = -1;
@@ -142,14 +144,3 @@
}
}
}
-
-void res_setnetcontext(res_state statp, const struct android_net_context* netcontext,
- android::net::NetworkDnsEventReported* _Nonnull event) {
- if (statp != nullptr) {
- statp->netid = netcontext->dns_netid;
- statp->uid = netcontext->uid;
- statp->_mark = netcontext->dns_mark;
- statp->netcontext_flags = netcontext->flags;
- statp->event = event;
- }
-}
diff --git a/res_init.h b/res_init.h
index af76414..4c759c4 100644
--- a/res_init.h
+++ b/res_init.h
@@ -15,5 +15,8 @@
*/
#pragma once
-struct __res_state;
-void res_init(__res_state* statp);
+#include "resolv_private.h"
+
+// TODO: make this a constructor for ResState
+void res_init(ResState* res, const struct android_net_context* netcontext,
+ android::net::NetworkDnsEventReported* event);
diff --git a/res_mkquery.cpp b/res_mkquery.cpp
index 5dde9be..fb8a074 100644
--- a/res_mkquery.cpp
+++ b/res_mkquery.cpp
@@ -93,19 +93,15 @@
"11", "12", "13", "ZONEINIT", "ZONEREF",
};
-/*
- * Form all types of queries.
- * Returns the size of the result or -1.
- */
-int res_nmkquery(res_state statp, int op, /* opcode of query */
- const char* dname, /* domain name */
- int cl, int type, /* class and type of query */
- const uint8_t* data, /* resource record data */
- int datalen, /* length of data */
- const uint8_t* /*newrr_in*/, /* new rr for modify or append */
- uint8_t* buf, /* buffer to put query */
- int buflen) /* size of buffer */
-{
+// Form all types of queries. Returns the size of the result or -1.
+int res_nmkquery(int op, // opcode of query
+ const char* dname, // domain name
+ int cl, int type, // class and type of query
+ const uint8_t* data, // resource record data
+ int datalen, // length of data
+ uint8_t* buf, // buffer to put query
+ int buflen, // size of buffer
+ int netcontext_flags) {
HEADER* hp;
uint8_t *cp, *ep;
int n;
@@ -123,7 +119,7 @@
hp->id = htons(arc4random_uniform(65536));
hp->opcode = op;
hp->rd = true;
- hp->ad = (statp->netcontext_flags & NET_CONTEXT_FLAG_USE_DNS_OVER_TLS) != 0U;
+ hp->ad = (netcontext_flags & NET_CONTEXT_FLAG_USE_DNS_OVER_TLS) != 0U;
hp->rcode = NOERROR;
cp = buf + HFIXEDSZ;
ep = buf + buflen;
diff --git a/res_query.cpp b/res_query.cpp
index f54ff3f..43c81fa 100644
--- a/res_query.cpp
+++ b/res_query.cpp
@@ -123,7 +123,8 @@
LOG(DEBUG) << __func__ << ": (" << cl << ", " << type << ")";
- n = res_nmkquery(statp, QUERY, name, cl, type, NULL, 0, NULL, buf, sizeof(buf));
+ n = res_nmkquery(QUERY, name, cl, type, /*data=*/nullptr, 0, buf, sizeof(buf),
+ statp->netcontext_flags);
if (n > 0 &&
(statp->netcontext_flags &
(NET_CONTEXT_FLAG_USE_DNS_OVER_TLS | NET_CONTEXT_FLAG_USE_EDNS)) &&
diff --git a/res_send.cpp b/res_send.cpp
index 6616ca3..9eee05d 100644
--- a/res_send.cpp
+++ b/res_send.cpp
@@ -107,6 +107,7 @@
#include "netd_resolv/stats.h"
#include "private/android_filesystem_config.h"
#include "res_debug.h"
+#include "res_init.h"
#include "resolv_cache.h"
#include "stats.pb.h"
@@ -1237,9 +1238,9 @@
uint8_t* ans, int ansLen, int* rcode, uint32_t flags,
NetworkDnsEventReported* event) {
assert(event != nullptr);
- res_state res = res_get_state();
- res_setnetcontext(res, netContext, event);
- _resolv_populate_res_for_net(res);
+ ResState res;
+ res_init(&res, netContext, event);
+ _resolv_populate_res_for_net(&res);
*rcode = NOERROR;
- return res_nsend(res, msg, msgLen, ans, ansLen, rcode, flags);
+ return res_nsend(&res, msg, msgLen, ans, ansLen, rcode, flags);
}
diff --git a/res_state.cpp b/res_state.cpp
deleted file mode 100644
index 7388082..0000000
--- a/res_state.cpp
+++ /dev/null
@@ -1,95 +0,0 @@
-/*
- * Copyright (C) 2008 The Android Open Source Project
- * All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- * * Redistributions in binary form must reproduce the above copyright
- * notice, this list of conditions and the following disclaimer in
- * the documentation and/or other materials provided with the
- * distribution.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
- * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
- * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
- * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
- * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
- * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
- * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
- * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
- * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
- * SUCH DAMAGE.
- */
-
-#define LOG_TAG "resolv"
-
-#include <arpa/inet.h>
-#include <arpa/nameser.h>
-#include <netdb.h>
-#include <pthread.h>
-#include <stdlib.h>
-#include <string.h>
-#include <unistd.h> /* for gettid() */
-
-#include <android-base/logging.h>
-
-#include "res_init.h"
-#include "resolv_cache.h"
-#include "resolv_private.h"
-
-typedef struct {
- // TODO: Have one __res_state per network so we don't have to repopulate frequently.
- struct __res_state _nres[1];
-} _res_thread;
-
-static _res_thread* res_thread_alloc(void) {
- _res_thread* rt = (_res_thread*) calloc(1, sizeof(*rt));
- return rt;
-}
-
-static void res_thread_free(void* _rt) {
- _res_thread* rt = (_res_thread*) _rt;
-
- LOG(VERBOSE) << __func__ << ": rt=" << rt << " for thread=" << gettid();
-
- res_nclose(rt->_nres);
- free(rt);
-}
-
-static pthread_key_t _res_key;
-
-__attribute__((constructor)) static void __res_key_init() {
- pthread_key_create(&_res_key, res_thread_free);
-}
-
-static _res_thread* res_thread_get(void) {
- _res_thread* rt = (_res_thread*) pthread_getspecific(_res_key);
- if (rt != NULL) {
- return rt;
- }
-
- /* It is the first time this function is called in this thread,
- * we need to create a new thread-specific DNS resolver state. */
- rt = res_thread_alloc();
- if (rt == NULL) {
- return NULL;
- }
- pthread_setspecific(_res_key, rt);
-
- LOG(VERBOSE) << __func__ << ": tid=" << gettid() << ", rt=" << rt;
- res_init(rt->_nres);
- return rt;
-}
-
-struct __res_state _nres;
-
-res_state res_get_state(void) {
- _res_thread* rt = res_thread_get();
-
- return rt ? rt->_nres : NULL;
-}
diff --git a/resolv_cache.h b/resolv_cache.h
index 9a9787e..2b372f6 100644
--- a/resolv_cache.h
+++ b/resolv_cache.h
@@ -35,15 +35,11 @@
#include <unordered_map>
#include <vector>
-struct __res_state;
-struct resolv_cache;
-
-constexpr int DNSEVENT_SUBSAMPLING_MAP_DEFAULT_KEY = -1;
-
-/* sets the name server addresses to the provided res_state structure. The
- * name servers are retrieved from the cache which is associated
- * with the network to which the res_state structure is associated */
-void _resolv_populate_res_for_net(struct __res_state* statp);
+// Sets the name server addresses to the provided ResState.
+// The name servers are retrieved from the cache which is associated
+// with the network to which ResState is associated.
+struct ResState;
+void _resolv_populate_res_for_net(ResState* statp);
std::vector<unsigned> resolv_list_caches();
diff --git a/resolv_cache_unit_test.cpp b/resolv_cache_unit_test.cpp
index f62f317..1db209f 100644
--- a/resolv_cache_unit_test.cpp
+++ b/resolv_cache_unit_test.cpp
@@ -30,6 +30,7 @@
#include <gtest/gtest.h>
#include "netd_resolv/stats.h"
+#include "res_init.h"
#include "resolv_cache.h"
#include "resolv_private.h"
#include "tests/dns_responder/dns_responder.h"
@@ -64,9 +65,10 @@
};
std::vector<char> makeQuery(int op, const char* qname, int qclass, int qtype) {
- res_state res = res_get_state();
uint8_t buf[MAXPACKET] = {};
- const int len = res_nmkquery(res, op, qname, qclass, qtype, NULL, 0, NULL, buf, sizeof(buf));
+ const int len = res_nmkquery(op, qname, qclass, qtype, /*data=*/nullptr, /*datalen=*/0, buf,
+ sizeof(buf),
+ /*netcontext_flags=*/0);
return std::vector<char>(buf, buf + len);
}
diff --git a/resolv_private.h b/resolv_private.h
index 4c881c2..3272844 100644
--- a/resolv_private.h
+++ b/resolv_private.h
@@ -87,7 +87,7 @@
struct sockaddr_in6 sin6;
};
-struct __res_state {
+struct ResState {
unsigned netid; // NetId: cache key and socket mark
uid_t uid; // uid of the app that sent the DNS lookup
int nscount; // number of name srvers
@@ -103,7 +103,9 @@
uint32_t netcontext_flags;
};
-typedef struct __res_state* res_state;
+// TODO: remove these legacy aliases
+typedef ResState __res_state;
+typedef ResState* res_state;
/* Retrieve a local copy of the stats for the given netid. The buffer must have space for
* MAXNS __resolver_stats. Returns the revision id of the resolvers used.
@@ -140,9 +142,6 @@
extern const char* const _res_opcodes[];
-/* Things involving an internal (static) resolver context. */
-struct __res_state* res_get_state(void);
-
int res_hnok(const char*);
int res_ownok(const char*);
int res_mailok(const char*);
@@ -157,16 +156,12 @@
int res_nquery(res_state, const char*, int, int, uint8_t*, int, int*);
int res_nsearch(res_state, const char*, int, int, uint8_t*, int, int*);
int res_nquerydomain(res_state, const char*, const char*, int, int, uint8_t*, int, int*);
-int res_nmkquery(res_state, int, const char*, int, int, const uint8_t*, int, const uint8_t*,
- uint8_t*, int);
+int res_nmkquery(int op, const char* qname, int cl, int type, const uint8_t* data, int datalen,
+ uint8_t* buf, int buflen, int netcontext_flags);
int res_nsend(res_state, const uint8_t*, int, uint8_t*, int, int*, uint32_t);
void res_nclose(res_state);
int res_nopt(res_state, int, uint8_t*, int, int);
-struct android_net_context;
-void res_setnetcontext(res_state, const struct android_net_context*,
- android::net::NetworkDnsEventReported* event);
-
int getaddrinfo_numeric(const char* hostname, const char* servname, addrinfo hints,
addrinfo** result);