* [PATCH 01/16] dnsproxy-simple-test: improve test coverage and test flexibility
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 02/16] autoconf: require C99 compiler and set C99 mode Matthias Gerstner
` (14 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
- enable debug() macro for test invocation which allows to get test logs
- actually trigger caching logic explicitly by querying the same
configurations twice in succession
- count the number of cache hits to catch regressions in the caching
functionality
- support custom domains for testing specified on the command line
---
Makefile.am | 2 +-
src/dnsproxy.c | 6 +-
tools/dnsproxy-simple-test | 113 ++++++++++++++++++++++++++++++++-----
3 files changed, 104 insertions(+), 17 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index c108b8f3c..1a3dbe3c9 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -438,7 +438,7 @@ testdir = $(pkglibdir)/test
test_SCRIPTS = $(test_scripts)
if INTERNAL_DNS_BACKEND
-tools_dnsproxy_standalone_CFLAGS = $(src_connmand_CFLAGS) -I$(srcdir)/src
+tools_dnsproxy_standalone_CFLAGS = $(src_connmand_CFLAGS) -I$(srcdir)/src -DDNSPROXY_DEBUG
tools_dnsproxy_standalone_SOURCES = tools/dnsproxy-standalone.c $(src_connmand_SOURCES)
# for EXTRA_PROGRAMS the BUILT_SOURCES aren't automatically added as
# dependency, so let's do it explicitly
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 227300477..458694e60 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -41,7 +41,11 @@
#include "connman.h"
-#define debug(fmt...) do { } while (0)
+#ifdef DNSPROXY_DEBUG
+# define debug(fmt...) do { fprintf(stderr, fmt); fprintf(stderr, "\n"); } while (0)
+#else
+# define debug(fmt...) do { } while (0)
+#endif
#if __BYTE_ORDER == __LITTLE_ENDIAN
struct domain_hdr {
diff --git a/tools/dnsproxy-simple-test b/tools/dnsproxy-simple-test
index b8fd7f77d..5c2f72925 100755
--- a/tools/dnsproxy-simple-test
+++ b/tools/dnsproxy-simple-test
@@ -6,9 +6,25 @@
echoerr() {
echo $@ 1>&2
+ echo -e "\n >>> ERROR OCCURED <<< \n" 1>&2
exit 1
}
+
+showlog() {
+ if [ -z "$SHOW_LOG" -o -z "$logfile" ]; then
+ return
+ fi
+
+ echo
+ echo "======== debug log ==========="
+ cat "$logfile"
+ echo "===== end debug log =========="
+ echo
+}
+
+TRANSPORTS="-U -T"
+
while [ $# -gt 0 ]; do
case "$1" in
"--valgrind")
@@ -16,10 +32,39 @@ while [ $# -gt 0 ]; do
if [ -z "$VALGRIND" ]; then
echoerr "no valgrind executable found"
fi
+ # log valgrind output to stdout, since stderr is used for
+ # debug output from dnsproxy.c already and we want to parse
+ # that.
+ # also cause an error exit it valgrind error occur so that
+ # they're easily noticed.
+ VALGRIND="$VALGRIND --log-fd=1 --error-exitcode=10"
+ ;;
+ "--gdb")
+ WAIT_GDB=1
+ # wait forever to avoid timeout conditions during debugging
+ HOST_OPTS="-w"
+ ;;
+ "--show-log")
+ SHOW_LOG=1
+ ;;
+ "--testdomain="*)
+ TESTDOMAIN=`echo $1 | cut -d '=' -f2-`
+ CUSTOM_TESTDOMAIN=1
+ ;;
+ "--only-tcp")
+ TRANSPORTS="-T"
+ ;;
+ "--only-udp")
+ TRANSPORTS="-U"
;;
"-h")
- echo "$0 [--valgrind]"
+ echo "$0 [--valgrind] [--gdb] [--show-log] [--only-tcp] [--only-udp] [--testdomain=<mydomain>]"
echo "--valgrind: run dnsproxy-standalone in valgrind"
+ echo "--gdb: allows you to attach via GDB before tests are started"
+ echo "--show-log: dump debug log from dnsproxy at end of test"
+ echo "--only-tcp: only perform TCP protocol based tests"
+ echo "--only-udp: only perform UDP protocol based tests"
+ echo "--testdomain=<mydomain>: the domain name to resolve"
exit 2
;;
*)
@@ -29,6 +74,11 @@ while [ $# -gt 0 ]; do
shift
done
+if [ -n "$VALGRIND" -a -n "$WAIT_GDB" ]; then
+ echo "Cannot mix valgrind frontend and GDB attachment" 1>&2
+ exit 2
+fi
+
if [ -e "Makefile" ]; then
BUILDROOT="$PWD"
else
@@ -66,7 +116,8 @@ fi
PORT=8053
# run the proxy in the background
-$VALGRIND $DNSPROXY $PORT "$DOMAIN1" "$NS1" &
+logfile=`mktemp`
+$VALGRIND $DNSPROXY $PORT "$DOMAIN1" "$NS1" 2>"$logfile" &
proxy_pid=$!
cleanup() {
@@ -77,6 +128,13 @@ cleanup() {
wait $proxy_pid
ret=$?
proxy_pid=-1
+ if [ -n "$logfile" ]; then
+ if [ -n "$SHOW_LOG" ]; then
+ showlog
+ fi
+ rm -f "$logfile"
+ unset logfile
+ fi
return $ret
}
@@ -85,24 +143,49 @@ trap cleanup err exit
sleep 1
echo -e "\n\n"
-# test both UDP and TCP mode
-for TRANSPORT in -U -T; do
- # test both IPv4 and IPv6
- for IP in -4 -6; do
- echo "Testing resolution using transport $TRANSPORT and IP${IP}"
- set -x
- $HOST $TRANSPORT $IP -p$PORT www.example.com 127.0.0.1
- RES=$?
- set +x
- if [ $RES -ne 0 ]; then
- echoerr "resolution failed"
- fi
+if [ -n "$WAIT_GDB" ]; then
+ echo "You can now attach to the dnsproxy process at PID $proxy_pid."
+ echo "Press ENTER to continue test execution"
+ read _
+fi
+
+if [ -z "$TESTDOMAIN" ]; then
+ TESTDOMAIN="www.example.com"
+fi
- echo -e "\n\n"
+# perform each test twice to actually get cached responses served for each
+# combination
+for I in `seq 2`; do
+ # test both UDP and TCP mode
+ for TRANSPORT in $TRANSPORTS; do
+ # test both IPv4 and IPv6
+ for IP in -4 -6; do
+ echo "Testing resolution using transport $TRANSPORT and IP${IP}"
+ set -x
+ $HOST $HOST_OPTS $TRANSPORT $IP -p$PORT $TESTDOMAIN 127.0.0.1
+ RES=$?
+ set +x
+ if [ $RES -ne 0 ]; then
+ echoerr "resolution failed"
+ fi
+
+ echo -e "\n\n"
+ done
done
done
+NUM_HITS=`grep "cache hit.*$TESTDOMAIN" "$logfile" | wc -l`
+
echo -e "\n\nDNS resolution succeeded for all test combinations"
+echo -e "\nNumber of cache hits: $NUM_HITS\n"
+# assert we have seen the expected number of cache hits in the log
+# this is the amount of cache hits for the default domain tests as seen before
+# refactoring of dnsproxy started.
+if [ -z "$CUSTOM_TESTDOMAIN" -a "$NUM_HITS" -ne 15 ]; then
+ echoerr "Unexpected number of cache hits encountered"
+elif [ "$NUM_HITS" -lt 8 ]; then
+ echoerr "Too low number of cache hits encountered"
+fi
cleanup
if [ $? -eq 0 ]; then
exit 0
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 02/16] autoconf: require C99 compiler and set C99 mode
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
2022-06-10 12:33 ` [PATCH 01/16] dnsproxy-simple-test: improve test coverage and test flexibility Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 03/16] dnsproxy: first bits of refactoring data types, global variables, simpler functions Matthias Gerstner
` (13 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
For refactoring the dnsproxy codebase using C99 language features will
come in handy (mostly for using more localized variable declarations).
---
configure.ac | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index a573cefd7..ea7a2facd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -22,7 +22,7 @@ AC_SUBST(abs_top_builddir)
AC_LANG_C
AC_USE_SYSTEM_EXTENSIONS
-AC_PROG_CC
+AC_PROG_CC_C99
AM_PROG_CC_C_O
AC_PROG_CC_PIE
AC_PROG_INSTALL
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 03/16] dnsproxy: first bits of refactoring data types, global variables, simpler functions
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
2022-06-10 12:33 ` [PATCH 01/16] dnsproxy-simple-test: improve test coverage and test flexibility Matthias Gerstner
2022-06-10 12:33 ` [PATCH 02/16] autoconf: require C99 compiler and set C99 mode Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-08-28 16:21 ` Daniel Wagner
2022-06-10 12:33 ` [PATCH 04/16] dnsproxy: refactoring of update_cached_ttl() and append_data() Matthias Gerstner
` (12 subsequent siblings)
15 siblings, 1 reply; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
- move all type declarations to the top of the unit to have them all in
one place, same for global variables
- introduce enums for having more descriptive identifiers for some of
the DNS header constants
- remove unnecessary zero initializations for global variables
- move variable declarations into more local scopes where possible (e.g.
in for loops). Shorter lifetimes of variables can make the code more
easy to follow.
- avoid some repetitive code sequences like `cache_free_ipv4()` by
moving them into separate functions
- use const variables in parameters where possible to make certain
guarantees of function calls more clear and avoid erroneous
assignments.
---
src/dnsproxy.c | 478 +++++++++++++++++++++++--------------------------
1 file changed, 227 insertions(+), 251 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 458694e60..b733ac78d 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -176,11 +176,17 @@ struct cache_data {
struct cache_entry {
char *key;
bool want_refresh;
- int hits;
+ size_t hits;
struct cache_data *ipv4;
struct cache_data *ipv6;
};
+struct cache_timeout {
+ time_t current_time;
+ time_t max_timeout;
+ bool try_harder;
+};
+
struct domain_question {
uint16_t type;
uint16_t class;
@@ -218,21 +224,43 @@ struct domain_rr {
*/
#define MAX_CACHE_SIZE 256
+#define DNS_HEADER_SIZE sizeof(struct domain_hdr)
+#define DNS_HEADER_TCP_EXTRA_BYTES 2
+
+enum dns_type {
+ /* IPv4 address 32-bit */
+ DNS_TYPE_A = ns_t_a,
+ /* IPv6 address 128-bit */
+ DNS_TYPE_AAAA = ns_t_aaaa,
+ /* alias to another name */
+ DNS_TYPE_CNAME = ns_t_cname,
+ /* start of a zone of authority */
+ DNS_TYPE_SOA = ns_t_soa
+};
+
+enum dns_class {
+ DNS_CLASS_IN = ns_c_in
+};
+
static int cache_size;
static GHashTable *cache;
static int cache_refcount;
-static GSList *server_list = NULL;
-static GSList *request_list = NULL;
-static GHashTable *listener_table = NULL;
+static GSList *server_list;
+static GSList *request_list;
+static GHashTable *listener_table;
static time_t next_refresh;
static GHashTable *partial_tcp_req_table;
-static guint cache_timer = 0;
+static guint cache_timer;
static in_port_t dns_listen_port = 53;
+/* we can keep using the same resolve's */
+static GResolv *ipv4_resolve;
+static GResolv *ipv6_resolve;
static guint16 get_id(void)
{
uint64_t rand;
+ /* TODO: return code is ignored, should we rather abort() on error? */
__connman_util_get_random(&rand);
return rand;
@@ -245,7 +273,7 @@ static size_t protocol_offset(int protocol)
return 0;
case IPPROTO_TCP:
- return 2;
+ return DNS_HEADER_TCP_EXTRA_BYTES;
default:
/* this should never happen */
@@ -276,9 +304,7 @@ static time_t round_down_ttl(time_t end_time, int ttl)
static struct request_data *find_request(guint16 id)
{
- GSList *list;
-
- for (list = request_list; list; list = list->next) {
+ for (GSList *list = request_list; list; list = list->next) {
struct request_data *req = list->data;
if (req->dstid == id || req->altid == id)
@@ -292,11 +318,9 @@ static struct server_data *find_server(int index,
const char *server,
int protocol)
{
- GSList *list;
-
debug("index %d server %s proto %d", index, server, protocol);
- for (list = server_list; list; list = list->next) {
+ for (GSList *list = server_list; list; list = list->next) {
struct server_data *data = list->data;
if (index < 0 && data->index < 0 &&
@@ -317,10 +341,6 @@ static struct server_data *find_server(int index,
return NULL;
}
-/* we can keep using the same resolve's */
-static GResolv *ipv4_resolve;
-static GResolv *ipv6_resolve;
-
static void dummy_resolve_func(GResolvResultStatus status,
char **results, gpointer user_data)
{
@@ -330,8 +350,6 @@ static void dummy_resolve_func(GResolvResultStatus status,
* Refresh a DNS entry, but also age the hit count a bit */
static void refresh_dns_entry(struct cache_entry *entry, char *name)
{
- int age = 1;
-
if (!ipv4_resolve) {
ipv4_resolve = g_resolv_new(0);
g_resolv_set_address_family(ipv4_resolve, AF_INET);
@@ -344,6 +362,8 @@ static void refresh_dns_entry(struct cache_entry *entry, char *name)
g_resolv_add_nameserver(ipv6_resolve, "::1", 53, 0);
}
+ int age = 1;
+
if (!entry->ipv4) {
debug("Refreshing A record for %s", name);
g_resolv_lookup_hostname(ipv4_resolve, name,
@@ -358,16 +378,17 @@ static void refresh_dns_entry(struct cache_entry *entry, char *name)
age = 4;
}
- entry->hits -= age;
- if (entry->hits < 0)
+ if (entry->hits > age)
+ entry->hits -= age;
+ else
entry->hits = 0;
}
-static int dns_name_length(unsigned char *buf)
+static size_t dns_name_length(const unsigned char *buf)
{
if ((buf[0] & NS_CMPRSFLGS) == NS_CMPRSFLGS) /* compressed name */
return 2;
- return strlen((char *)buf) + 1;
+ return strlen((const char *)buf) + 1;
}
static void update_cached_ttl(unsigned char *buf, int len, int new_ttl)
@@ -419,13 +440,11 @@ static void update_cached_ttl(unsigned char *buf, int len, int new_ttl)
}
}
-static void send_cached_response(int sk, unsigned char *buf, int len,
+static void send_cached_response(int sk, unsigned char *ptr, int len,
const struct sockaddr *to, socklen_t tolen,
int protocol, int id, uint16_t answers, int ttl)
{
- struct domain_hdr *hdr;
- unsigned char *ptr = buf;
- int err, offset, dns_len, adj_len = len - 2;
+ int offset, dns_len;
/*
* The cached packet contains always the TCP offset (two bytes)
@@ -449,7 +468,7 @@ static void send_cached_response(int sk, unsigned char *buf, int len,
if (len < 12)
return;
- hdr = (void *) (ptr + offset);
+ struct domain_hdr *hdr = (void *) (ptr + offset);
hdr->id = id;
hdr->qr = 1;
@@ -461,42 +480,40 @@ static void send_cached_response(int sk, unsigned char *buf, int len,
/* if this is a negative reply, we are authoritative */
if (answers == 0)
hdr->aa = 1;
- else
+ else {
+ const int adj_len = len - 2;
update_cached_ttl((unsigned char *)hdr, adj_len, ttl);
+ }
debug("sk %d id 0x%04x answers %d ptr %p length %d dns %d",
sk, hdr->id, answers, ptr, len, dns_len);
- err = sendto(sk, ptr, len, MSG_NOSIGNAL, to, tolen);
- if (err < 0) {
+ const int res = sendto(sk, ptr, len, MSG_NOSIGNAL, to, tolen);
+ if (res < 0) {
connman_error("Cannot send cached DNS response: %s",
strerror(errno));
- return;
}
-
- if (err != len || (dns_len != (len - 2) && protocol == IPPROTO_TCP) ||
- (dns_len != len && protocol == IPPROTO_UDP))
+ else if (res != len || dns_len != (len - offset))
debug("Packet length mismatch, sent %d wanted %d dns %d",
- err, len, dns_len);
+ res, len, dns_len);
}
static void send_response(int sk, unsigned char *buf, size_t len,
const struct sockaddr *to, socklen_t tolen,
int protocol)
{
- struct domain_hdr *hdr;
- int err;
- size_t offset = protocol_offset(protocol);
+ const size_t offset = protocol_offset(protocol);
+ const size_t send_size = DNS_HEADER_SIZE + offset;
debug("sk %d", sk);
- if (len < sizeof(*hdr) + offset)
+ if (len < send_size)
return;
- hdr = (void *) (buf + offset);
+ struct domain_hdr *hdr = (void *) (buf + offset);
if (offset) {
buf[0] = 0;
- buf[1] = sizeof(*hdr);
+ buf[1] = DNS_HEADER_SIZE;
}
debug("id 0x%04x qr %d opcode %d", hdr->id, hdr->qr, hdr->opcode);
@@ -509,11 +526,10 @@ static void send_response(int sk, unsigned char *buf, size_t len,
hdr->nscount = 0;
hdr->arcount = 0;
- err = sendto(sk, buf, sizeof(*hdr) + offset, MSG_NOSIGNAL, to, tolen);
+ const int err = sendto(sk, buf, send_size, MSG_NOSIGNAL, to, tolen);
if (err < 0) {
connman_error("Failed to send DNS response to %d: %s",
sk, strerror(errno));
- return;
}
}
@@ -546,8 +562,6 @@ static void destroy_request_data(struct request_data *req)
static gboolean request_timeout(gpointer user_data)
{
struct request_data *req = user_data;
- struct sockaddr *sa;
- int sk;
if (!req)
return FALSE;
@@ -556,13 +570,18 @@ static gboolean request_timeout(gpointer user_data)
request_list = g_slist_remove(request_list, req);
+ struct sockaddr *sa;
+ int sk = -1;
+
if (req->protocol == IPPROTO_UDP) {
sk = get_req_udp_socket(req);
sa = &req->sa;
} else if (req->protocol == IPPROTO_TCP) {
sk = req->client_sk;
sa = NULL;
- } else
+ }
+
+ if (sk < 0)
goto out;
if (req->resplen > 0 && req->resp) {
@@ -571,22 +590,18 @@ static gboolean request_timeout(gpointer user_data)
* "not found" result), so send that back to client instead
* of more fatal server failed error.
*/
- if (sk >= 0)
- sendto(sk, req->resp, req->resplen, MSG_NOSIGNAL,
- sa, req->sa_len);
+ sendto(sk, req->resp, req->resplen, MSG_NOSIGNAL,
+ sa, req->sa_len);
} else if (req->request) {
/*
* There was not reply from server at all.
*/
- struct domain_hdr *hdr;
-
- hdr = (void *)(req->request + protocol_offset(req->protocol));
+ struct domain_hdr *hdr = (void *)(req->request + protocol_offset(req->protocol));
hdr->id = req->srcid;
- if (sk >= 0)
- send_response(sk, req->request, req->request_len,
- sa, req->sa_len, req->protocol);
+ send_response(sk, req->request, req->request_len,
+ sa, req->sa_len, req->protocol);
}
/*
@@ -661,18 +676,36 @@ static int append_query(unsigned char *buf, unsigned int size,
return ptr - buf;
}
-static bool cache_check_is_valid(struct cache_data *data,
- time_t current_time)
+static bool cache_check_is_valid(struct cache_data *data, time_t current_time)
{
if (!data)
return false;
-
- if (data->cache_until < current_time)
+ else if (data->cache_until < current_time)
return false;
return true;
}
+static void cache_free_ipv4(struct cache_entry *entry)
+{
+ if (!entry->ipv4)
+ return;
+
+ g_free(entry->ipv4->data);
+ g_free(entry->ipv4);
+ entry->ipv4 = NULL;
+}
+
+static void cache_free_ipv6(struct cache_entry *entry)
+{
+ if (!entry->ipv6)
+ return;
+
+ g_free(entry->ipv6->data);
+ g_free(entry->ipv6);
+ entry->ipv6 = NULL;
+}
+
/*
* remove stale cached entries so that they can be refreshed
*/
@@ -680,76 +713,64 @@ static void cache_enforce_validity(struct cache_entry *entry)
{
time_t current_time = time(NULL);
- if (!cache_check_is_valid(entry->ipv4, current_time)
- && entry->ipv4) {
+ if (entry->ipv4 && !cache_check_is_valid(entry->ipv4, current_time)) {
debug("cache timeout \"%s\" type A", entry->key);
- g_free(entry->ipv4->data);
- g_free(entry->ipv4);
- entry->ipv4 = NULL;
-
+ cache_free_ipv4(entry);
}
- if (!cache_check_is_valid(entry->ipv6, current_time)
- && entry->ipv6) {
+ if (entry->ipv6 && !cache_check_is_valid(entry->ipv6, current_time)) {
debug("cache timeout \"%s\" type AAAA", entry->key);
- g_free(entry->ipv6->data);
- g_free(entry->ipv6);
- entry->ipv6 = NULL;
+ cache_free_ipv6(entry);
}
}
-static uint16_t cache_check_validity(char *question, uint16_t type,
+static bool cache_check_validity(const char *question, uint16_t type,
struct cache_entry *entry)
{
- time_t current_time = time(NULL);
- bool want_refresh = false;
-
- /*
- * if we have a popular entry, we want a refresh instead of
- * total destruction of the entry.
- */
- if (entry->hits > 2)
- want_refresh = true;
-
cache_enforce_validity(entry);
- switch (type) {
- case 1: /* IPv4 */
- if (!cache_check_is_valid(entry->ipv4, current_time)) {
- debug("cache %s \"%s\" type A", entry->ipv4 ?
- "timeout" : "entry missing", question);
+ struct cache_data *cached_ip = NULL, *other_ip = NULL;
- if (want_refresh)
- entry->want_refresh = true;
+ switch (type) {
+ case DNS_TYPE_A: /* IPv4 */
+ cached_ip = entry->ipv4;
+ other_ip = entry->ipv6;
+ break;
- /*
- * We do not remove cache entry if there is still
- * valid IPv6 entry found in the cache.
- */
- if (!cache_check_is_valid(entry->ipv6, current_time) && !want_refresh) {
- g_hash_table_remove(cache, question);
- type = 0;
- }
- }
+ case DNS_TYPE_AAAA: /* IPv6 */
+ cached_ip = entry->ipv6;
+ other_ip = entry->ipv4;
break;
+ default:
+ return false;
+ }
- case 28: /* IPv6 */
- if (!cache_check_is_valid(entry->ipv6, current_time)) {
- debug("cache %s \"%s\" type AAAA", entry->ipv6 ?
- "timeout" : "entry missing", question);
+ const time_t current_time = time(NULL);
+ /*
+ * if we have a popular entry, we want a refresh instead of
+ * total destruction of the entry.
+ */
+ const bool want_refresh = entry->hits > 2 ? true : false;
- if (want_refresh)
- entry->want_refresh = true;
+ if (!cache_check_is_valid(cached_ip, current_time)) {
+ debug("cache %s \"%s\" type %s",
+ cached_ip ? "timeout" : "entry missing",
+ question,
+ cached_ip == entry->ipv4 ? "A" : "AAAA");
- if (!cache_check_is_valid(entry->ipv4, current_time) && !want_refresh) {
- g_hash_table_remove(cache, question);
- type = 0;
- }
+ if (want_refresh)
+ entry->want_refresh = true;
+ /*
+ * We do not remove cache entry if there is still a
+ * valid entry for another IP version found in the cache.
+ */
+ else if (!cache_check_is_valid(other_ip, current_time)) {
+ g_hash_table_remove(cache, question);
+ return false;
}
- break;
}
- return type;
+ return true;
}
static void cache_element_destroy(gpointer value)
@@ -759,19 +780,13 @@ static void cache_element_destroy(gpointer value)
if (!entry)
return;
- if (entry->ipv4) {
- g_free(entry->ipv4->data);
- g_free(entry->ipv4);
- }
-
- if (entry->ipv6) {
- g_free(entry->ipv6->data);
- g_free(entry->ipv6);
- }
+ cache_free_ipv4(entry);
+ cache_free_ipv6(entry);
g_free(entry->key);
g_free(entry);
+ /* TODO: this would be a worrying condition. Does this ever happen? */
if (--cache_size < 0)
cache_size = 0;
}
@@ -785,6 +800,7 @@ static gboolean try_remove_cache(gpointer user_data)
g_hash_table_destroy(cache);
cache = NULL;
+ cache_size = 0;
}
return FALSE;
@@ -792,32 +808,27 @@ static gboolean try_remove_cache(gpointer user_data)
static void create_cache(void)
{
- if (__sync_fetch_and_add(&cache_refcount, 1) == 0)
+ if (__sync_fetch_and_add(&cache_refcount, 1) == 0) {
cache = g_hash_table_new_full(g_str_hash,
g_str_equal,
NULL,
cache_element_destroy);
+ cache_size = 0;
+ }
}
-static struct cache_entry *cache_check(gpointer request, int *qtype, int proto)
+static struct cache_entry *cache_check(gpointer request, uint16_t *qtype, int proto)
{
- char *question;
- struct cache_entry *entry;
- struct domain_question *q;
- uint16_t type;
- int offset;
-
if (!request)
return NULL;
- question = request + protocol_offset(proto) + 12;
-
- offset = strlen(question) + 1;
- q = (void *) (question + offset);
- type = ntohs(q->type);
+ const char *question = request + protocol_offset(proto) + DNS_HEADER_SIZE;
+ const size_t offset = strlen(question) + 1;
+ const struct domain_question *q = (void *) (question + offset);
+ const uint16_t type = ntohs(q->type);
/* We only cache either A (1) or AAAA (28) requests */
- if (type != 1 && type != 28)
+ if (type != DNS_TYPE_A && type != DNS_TYPE_AAAA)
return NULL;
if (!cache) {
@@ -825,12 +836,11 @@ static struct cache_entry *cache_check(gpointer request, int *qtype, int proto)
return NULL;
}
- entry = g_hash_table_lookup(cache, question);
+ struct cache_entry *entry = g_hash_table_lookup(cache, question);
if (!entry)
return NULL;
- type = cache_check_validity(question, type, entry);
- if (type == 0)
+ if (!cache_check_validity(question, type, entry))
return NULL;
*qtype = type;
@@ -845,20 +855,19 @@ static struct cache_entry *cache_check(gpointer request, int *qtype, int proto)
* format so that we can cache the wire format string directly.
*/
static int get_name(int counter,
- unsigned char *pkt, unsigned char *start, unsigned char *max,
+ const unsigned char *pkt, const unsigned char *start, const unsigned char *max,
unsigned char *output, int output_max, int *output_len,
- unsigned char **end, char *name, size_t max_name, int *name_len)
+ const unsigned char **end, char *name, size_t max_name, int *name_len)
{
- unsigned char *p;
/* Limit recursion to 10 (this means up to 10 labels in domain name) */
if (counter > 10)
return -EINVAL;
- p = start;
+ const unsigned char *p = start;
while (*p) {
if ((*p & NS_CMPRSFLGS) == NS_CMPRSFLGS) {
- uint16_t offset = (*p & 0x3F) * 256 + *(p + 1);
+ const uint16_t offset = (*p & 0x3F) * 256 + *(p + 1);
if (offset >= max - pkt)
return -ENOBUFS;
@@ -874,11 +883,9 @@ static int get_name(int counter,
if (pkt + label_len > max)
return -ENOBUFS;
-
- if (*output_len > output_max)
+ else if (*output_len > output_max)
return -ENOBUFS;
-
- if ((*name_len + 1 + label_len + 1) > max_name)
+ else if ((*name_len + 1 + label_len + 1) > max_name)
return -ENOBUFS;
/*
@@ -907,28 +914,28 @@ static int get_name(int counter,
return 0;
}
-static int parse_rr(unsigned char *buf, unsigned char *start,
- unsigned char *max,
+static int parse_rr(const unsigned char *buf, const unsigned char *start,
+ const unsigned char *max,
unsigned char *response, unsigned int *response_size,
- uint16_t *type, uint16_t *class, int *ttl, int *rdlen,
- unsigned char **end,
+ uint16_t *type, uint16_t *class, int *ttl, uint16_t *rdlen,
+ const unsigned char **end,
char *name, size_t max_name)
{
- struct domain_rr *rr;
- int err, offset;
int name_len = 0, output_len = 0, max_rsp = *response_size;
- err = get_name(0, buf, start, max, response, max_rsp,
+ {
+ int err = get_name(0, buf, start, max, response, max_rsp,
&output_len, end, name, max_name, &name_len);
- if (err < 0)
- return err;
+ if (err < 0)
+ return err;
+ }
- offset = output_len;
+ size_t offset = output_len;
- if ((unsigned int) offset > *response_size)
+ if (offset > *response_size)
return -ENOBUFS;
- rr = (void *) (*end);
+ struct domain_rr *rr = (void *) (*end);
if (!rr)
return -EINVAL;
@@ -946,26 +953,23 @@ static int parse_rr(unsigned char *buf, unsigned char *start,
offset += sizeof(struct domain_rr);
*end += sizeof(struct domain_rr);
- if ((unsigned int) (offset + *rdlen) > *response_size)
+ if ((offset + *rdlen) > *response_size)
return -ENOBUFS;
memcpy(response + offset, *end, *rdlen);
*end += *rdlen;
-
*response_size = offset + *rdlen;
return 0;
}
-static bool check_alias(GSList *aliases, char *name)
+static bool check_alias(GSList *aliases, const char *name)
{
- GSList *list;
-
if (aliases) {
- for (list = aliases; list; list = list->next) {
- int len = strlen((char *)list->data);
- if (strncmp((char *)list->data, name, len) == 0)
+ for (GSList *list = aliases; list; list = list->next) {
+ const char *cmpname = (const char*)list->data;
+ if (strncmp(cmpname, name, NS_MAXDNAME) == 0)
return true;
}
}
@@ -981,12 +985,12 @@ static int parse_response(unsigned char *buf, int buflen,
{
struct domain_hdr *hdr = (void *) buf;
struct domain_question *q;
- unsigned char *ptr;
+ const unsigned char *ptr;
uint16_t qdcount = ntohs(hdr->qdcount);
uint16_t ancount = ntohs(hdr->ancount);
int err, i;
uint16_t qtype, qclass;
- unsigned char *next = NULL;
+ const unsigned char *next = NULL;
unsigned int maxlen = *response_len;
GSList *aliases = NULL, *list;
char name[NS_MAXDNAME + 1];
@@ -1040,7 +1044,8 @@ static int parse_response(unsigned char *buf, int buflen,
*/
unsigned char rsp[NS_MAXCDNAME];
unsigned int rsp_len = sizeof(rsp) - 1;
- int ret, rdlen;
+ int ret;
+ uint16_t rdlen;
memset(rsp, 0, sizeof(rsp));
@@ -1099,7 +1104,7 @@ static int parse_response(unsigned char *buf, int buflen,
* question. We need to find the real A/AAAA record
* of the alias and cache that.
*/
- unsigned char *end = NULL;
+ const unsigned char *end = NULL;
int name_len = 0, output_len = 0;
memset(rsp, 0, sizeof(rsp));
@@ -1171,18 +1176,12 @@ out:
return err;
}
-struct cache_timeout {
- time_t current_time;
- int max_timeout;
- int try_harder;
-};
-
static gboolean cache_check_entry(gpointer key, gpointer value,
gpointer user_data)
{
struct cache_timeout *data = user_data;
struct cache_entry *entry = value;
- int max_timeout;
+ time_t max_timeout;
/* Scale the number of hits by half as part of cache aging */
@@ -1223,14 +1222,14 @@ static gboolean cache_check_entry(gpointer key, gpointer value,
static void cache_cleanup(void)
{
- static int max_timeout;
- struct cache_timeout data;
+ static time_t max_timeout;
+ struct cache_timeout data = {
+ .current_time = time(NULL),
+ .max_timeout = 0,
+ .try_harder = false
+ };
int count = 0;
- data.current_time = time(NULL);
- data.max_timeout = 0;
- data.try_harder = 0;
-
/*
* In the first pass, we only remove entries that have timed out.
* We use a cache of the first time to expire to do this only
@@ -1247,7 +1246,7 @@ static void cache_cleanup(void)
* we also expire entries with a low hit count,
* while aging the hit count at the same time.
*/
- data.try_harder = 1;
+ data.try_harder = true;
if (count == 0)
count = g_hash_table_foreach_remove(cache, cache_check_entry,
&data);
@@ -1277,23 +1276,11 @@ static gboolean cache_invalidate_entry(gpointer key, gpointer value,
entry->want_refresh = true;
/* delete the cached data */
- if (entry->ipv4) {
- g_free(entry->ipv4->data);
- g_free(entry->ipv4);
- entry->ipv4 = NULL;
- }
-
- if (entry->ipv6) {
- g_free(entry->ipv6->data);
- g_free(entry->ipv6);
- entry->ipv6 = NULL;
- }
+ cache_free_ipv4(entry);
+ cache_free_ipv6(entry);
/* keep the entry if we want it refreshed, delete it otherwise */
- if (entry->want_refresh)
- return FALSE;
- else
- return TRUE;
+ return entry->want_refresh ? FALSE : TRUE;
}
/*
@@ -1314,25 +1301,22 @@ static void cache_invalidate(void)
static void cache_refresh_entry(struct cache_entry *entry)
{
-
cache_enforce_validity(entry);
- if (entry->hits > 2 && !entry->ipv4)
- entry->want_refresh = true;
- if (entry->hits > 2 && !entry->ipv6)
+ if (entry->hits > 2 && (!entry->ipv4 || !entry->ipv6))
entry->want_refresh = true;
if (entry->want_refresh) {
- char *c;
- char dns_name[NS_MAXDNAME + 1];
entry->want_refresh = false;
+ char dns_name[NS_MAXDNAME + 1];
/* turn a DNS name into a hostname with dots */
strncpy(dns_name, entry->key, NS_MAXDNAME);
- c = dns_name;
- while (c && *c) {
- int jump;
- jump = *c;
+ char *c = dns_name;
+ while (*c) {
+ /* fetch the size of the current component and replace
+ it by a dot */
+ int jump = *c;
*c = '.';
c += jump + 1;
}
@@ -1358,23 +1342,19 @@ static void cache_refresh(void)
g_hash_table_foreach(cache, cache_refresh_iterator, NULL);
}
-static int reply_query_type(unsigned char *msg, int len)
+static int reply_query_type(const unsigned char *msg, int len)
{
- unsigned char *c;
- int l;
- int type;
-
/* skip the header */
- c = msg + sizeof(struct domain_hdr);
- len -= sizeof(struct domain_hdr);
+ const unsigned char *c = msg + DNS_HEADER_SIZE;
+ len -= DNS_HEADER_SIZE;
if (len < 0)
return 0;
- /* now the query, which is a name and 2 16 bit words */
- l = dns_name_length(c);
- c += l;
- type = c[0] << 8 | c[1];
+ /* now the query, which is a name and 2 16 bit words for type and class */
+ c += dns_name_length(c);
+
+ int type = c[0] << 8 | c[1];
return type;
}
@@ -1607,7 +1587,8 @@ static int ns_resolv(struct server_data *server, struct request_data *req,
gpointer request, gpointer name)
{
GList *list;
- int sk, err, type = 0;
+ int sk, err;
+ uint16_t type = 0;
char *dot, *lookup = (char *) name;
struct cache_entry *entry;
@@ -1723,40 +1704,36 @@ static int ns_resolv(struct server_data *server, struct request_data *req,
return 0;
}
-static char *convert_label(char *start, char *end, char *ptr, char *uptr,
+static bool convert_label(const char *start, const char *end, const char *ptr, char *uptr,
int remaining_len, int *used_comp, int *used_uncomp)
{
- int pos, comp_pos;
char name[NS_MAXLABEL];
- pos = dn_expand((u_char *)start, (u_char *)end, (u_char *)ptr,
+ const int pos = dn_expand((const u_char *)start, (const u_char *)end, (const u_char *)ptr,
name, NS_MAXLABEL);
if (pos < 0) {
debug("uncompress error [%d/%s]", errno, strerror(errno));
- goto out;
+ return false;
}
/*
* We need to compress back the name so that we get back to internal
* label presentation.
*/
- comp_pos = dn_comp(name, (u_char *)uptr, remaining_len, NULL, NULL);
+ const int comp_pos = dn_comp(name, (u_char *)uptr, remaining_len, NULL, NULL);
if (comp_pos < 0) {
debug("compress error [%d/%s]", errno, strerror(errno));
- goto out;
+ return false;
}
*used_comp = pos;
*used_uncomp = comp_pos;
- return ptr;
-
-out:
- return NULL;
+ return true;
}
-static char *uncompress(int16_t field_count, char *start, char *end,
- char *ptr, char *uncompressed, int uncomp_len,
+static const char* uncompress(int16_t field_count, const char *start, const char *end,
+ const char *ptr, char *uncompressed, int uncomp_len,
char **uncompressed_ptr)
{
char *uptr = *uncompressed_ptr; /* position in result buffer */
@@ -1806,7 +1783,7 @@ static char *uncompress(int16_t field_count, char *start, char *end,
dns_type = uptr[0] << 8 | uptr[1];
dns_class = uptr[2] << 8 | uptr[3];
- if (dns_class != ns_c_in)
+ if (dns_class != DNS_CLASS_IN)
goto out;
ptr += NS_RRFIXEDSZ;
@@ -1817,7 +1794,7 @@ static char *uncompress(int16_t field_count, char *start, char *end,
* Typically this portion is also compressed
* so we need to uncompress it also when necessary.
*/
- if (dns_type == ns_t_cname) {
+ if (dns_type == DNS_TYPE_CNAME) {
if (!convert_label(start, end, ptr, uptr,
uncomp_len - (uptr - uncompressed),
&pos, &comp_pos))
@@ -1829,7 +1806,7 @@ static char *uncompress(int16_t field_count, char *start, char *end,
uptr += comp_pos;
ptr += pos;
- } else if (dns_type == ns_t_a || dns_type == ns_t_aaaa) {
+ } else if (dns_type == DNS_TYPE_A || dns_type == DNS_TYPE_AAAA) {
dlen = uptr[-2] << 8 | uptr[-1];
if ((ptr + dlen) > end || (uptr + dlen) > uncomp_end) {
@@ -1841,7 +1818,7 @@ static char *uncompress(int16_t field_count, char *start, char *end,
uptr += dlen;
ptr += dlen;
- } else if (dns_type == ns_t_soa) {
+ } else if (dns_type == DNS_TYPE_SOA) {
int total_len = 0;
char *len_ptr;
@@ -1898,17 +1875,16 @@ out:
static int strip_domains(char *name, char *answers, int maxlen)
{
- uint16_t data_len;
- int name_len = strlen(name);
- char *ptr, *start = answers, *end = answers + maxlen;
+ const size_t name_len = strlen(name);
+ const char *start = answers, *end = answers + maxlen;
while (maxlen > 0) {
- ptr = strstr(answers, name);
+ char *ptr = strstr(answers, name);
if (ptr) {
char *domain = ptr + name_len;
if (*domain) {
- int domain_len = strlen(domain);
+ const size_t domain_len = strlen(domain);
memmove(answers + name_len,
domain + domain_len,
@@ -1922,7 +1898,7 @@ static int strip_domains(char *name, char *answers, int maxlen)
answers += strlen(answers) + 1;
answers += 2 + 2 + 4; /* skip type, class and ttl fields */
- data_len = answers[0] << 8 | answers[1];
+ uint16_t data_len = answers[0] << 8 | answers[1];
answers += 2; /* skip the length field */
if (answers + data_len > end)
@@ -1988,8 +1964,8 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
uint16_t dns_type, dns_class;
uint8_t host_len, dns_type_pos;
char uncompressed[NS_MAXDNAME], *uptr;
- char *ptr, *eom = (char *)reply + reply_len;
- char *domain;
+ const char *ptr, *eom = (char *)reply + reply_len;
+ const char *domain;
/*
* ptr points to the first char of the hostname.
@@ -2023,8 +1999,8 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
ptr[dns_type_pos + 1];
dns_class = ptr[dns_type_pos + 2] << 8 |
ptr[dns_type_pos + 3];
- if (dns_type != ns_t_a && dns_type != ns_t_aaaa &&
- dns_class != ns_c_in) {
+ if (dns_type != DNS_TYPE_A && dns_type != DNS_TYPE_AAAA &&
+ dns_class != DNS_CLASS_IN) {
debug("Pass msg dns type %d class %d",
dns_type, dns_class);
goto pass;
@@ -2258,7 +2234,6 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
gpointer user_data)
{
unsigned char buf[4096];
- int sk, len;
struct server_data *data = user_data;
if (condition & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
@@ -2267,11 +2242,12 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
return FALSE;
}
- sk = g_io_channel_unix_get_fd(channel);
-
- len = recv(sk, buf, sizeof(buf), 0);
+ int sk = g_io_channel_unix_get_fd(channel);
+ ssize_t len = recv(sk, buf, sizeof(buf), 0);
- forward_dns_reply(buf, len, IPPROTO_UDP, data);
+ if (len > 0) {
+ forward_dns_reply(buf, len, IPPROTO_UDP, data);
+ }
return TRUE;
}
@@ -3125,7 +3101,7 @@ static bool read_tcp_data(struct tcp_partial_client_data *client,
unsigned int msg_len;
GSList *list;
bool waiting_for_connect = false;
- int qtype = 0;
+ uint16_t qtype = 0;
struct cache_entry *entry;
client_sk = g_io_channel_unix_get_fd(client->channel);
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 03/16] dnsproxy: first bits of refactoring data types, global variables, simpler functions
2022-06-10 12:33 ` [PATCH 03/16] dnsproxy: first bits of refactoring data types, global variables, simpler functions Matthias Gerstner
@ 2022-08-28 16:21 ` Daniel Wagner
0 siblings, 0 replies; 19+ messages in thread
From: Daniel Wagner @ 2022-08-28 16:21 UTC (permalink / raw)
To: Matthias Gerstner; +Cc: connman
Hi Matthias,
Sorry for the long delay. Looking at the final result with the C99
mixing declaration with code I think it doesn't make sense to do it just
because it's possible.
On Fri, Jun 10, 2022 at 02:33:10PM +0200, Matthias Gerstner wrote:
> static struct request_data *find_request(guint16 id)
> {
> - GSList *list;
> -
> - for (list = request_list; list; list = list->next) {
> + for (GSList *list = request_list; list; list = list->next) {
This is fine and I think we should do it. The iterator is limited to the
loop context and this is what we want most of the time.
> static void dummy_resolve_func(GResolvResultStatus status,
> char **results, gpointer user_data)
> {
> @@ -330,8 +350,6 @@ static void dummy_resolve_func(GResolvResultStatus status,
> * Refresh a DNS entry, but also age the hit count a bit */
> static void refresh_dns_entry(struct cache_entry *entry, char *name)
> {
> - int age = 1;
> -
> if (!ipv4_resolve) {
> ipv4_resolve = g_resolv_new(0);
> g_resolv_set_address_family(ipv4_resolve, AF_INET);
> @@ -344,6 +362,8 @@ static void refresh_dns_entry(struct cache_entry *entry, char *name)
> g_resolv_add_nameserver(ipv6_resolve, "::1", 53, 0);
> }
>
> + int age = 1;
> +
But here nothing really is improved. In fact, it makes it harder to read
with the rest of the code.
> -static void send_cached_response(int sk, unsigned char *buf, int len,
> +static void send_cached_response(int sk, unsigned char *ptr, int len,
> const struct sockaddr *to, socklen_t tolen,
> int protocol, int id, uint16_t answers, int ttl)
> {
> - struct domain_hdr *hdr;
> - unsigned char *ptr = buf;
> - int err, offset, dns_len, adj_len = len - 2;
> + int offset, dns_len;
>
> /*
> * The cached packet contains always the TCP offset (two bytes)
> @@ -449,7 +468,7 @@ static void send_cached_response(int sk, unsigned char *buf, int len,
> if (len < 12)
> return;
>
> - hdr = (void *) (ptr + offset);
> + struct domain_hdr *hdr = (void *) (ptr + offset);
So this struct domain_hdr should still be at the beginning of the basic
block.
>
> hdr->id = id;
> hdr->qr = 1;
> @@ -461,42 +480,40 @@ static void send_cached_response(int sk, unsigned char *buf, int len,
> /* if this is a negative reply, we are authoritative */
> if (answers == 0)
> hdr->aa = 1;
> - else
> + else {
> + const int adj_len = len - 2;
> update_cached_ttl((unsigned char *)hdr, adj_len, ttl);
This is fine as it starts within a basic block and was possible to do
even without C99.
> + }
>
> debug("sk %d id 0x%04x answers %d ptr %p length %d dns %d",
> sk, hdr->id, answers, ptr, len, dns_len);
>
> - err = sendto(sk, ptr, len, MSG_NOSIGNAL, to, tolen);
> - if (err < 0) {
> + const int res = sendto(sk, ptr, len, MSG_NOSIGNAL, to, tolen);
> + if (res < 0) {
As I said the main reason I argue against this, we don't do this in the
rest of the code base and I think it's better to keep it consistent.
I skimmed through the rest of the series and looks reasonable. So it's
just this 'little' nitipick.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 04/16] dnsproxy: refactoring of update_cached_ttl() and append_data()
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (2 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 03/16] dnsproxy: first bits of refactoring data types, global variables, simpler functions Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 05/16] dnsproxy: refactor parse_response() Matthias Gerstner
` (11 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
- use size_t for sizes and lengths where possible
- use named constants in favor of literal numbers
- more localized variable declarations
- prefer byte order macros over explicit byte operations
- add some comments and use early exits to simplify the code
---
src/dnsproxy.c | 137 ++++++++++++++++++++++++-------------------------
1 file changed, 66 insertions(+), 71 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index b733ac78d..bceedfbc7 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -391,52 +391,49 @@ static size_t dns_name_length(const unsigned char *buf)
return strlen((const char *)buf) + 1;
}
-static void update_cached_ttl(unsigned char *buf, int len, int new_ttl)
+static void update_cached_ttl(unsigned char *ptr, int len, int new_ttl)
{
- unsigned char *c;
- uint16_t w;
- int l;
+ if (new_ttl < 0)
+ return;
/* skip the header */
- c = buf + 12;
- len -= 12;
+ ptr += DNS_HEADER_SIZE;
+ len -= DNS_HEADER_SIZE;
+
+ if (len < sizeof(struct domain_question) + 1)
+ return;
- /* skip the query, which is a name and 2 16 bit words */
- l = dns_name_length(c);
- c += l;
- len -= l;
- c += 4;
- len -= 4;
+ /* skip the query, which is a name and a struct domain_question */
+ size_t name_len = dns_name_length(ptr);
+
+ ptr += name_len + sizeof(struct domain_question);
+ len -= name_len + sizeof(struct domain_question);;
+
+ const uint32_t raw_ttl = ntohl((uint32_t)new_ttl);
+ struct domain_rr *rr = NULL;
/* now we get the answer records */
while (len > 0) {
/* first a name */
- l = dns_name_length(c);
- c += l;
- len -= l;
- if (len < 0)
- break;
- /* then type + class, 2 bytes each */
- c += 4;
- len -= 4;
+ name_len = dns_name_length(ptr);
+ ptr += name_len;
+ len -= name_len;
if (len < 0)
break;
- /* now the 4 byte TTL field */
- c[0] = new_ttl >> 24 & 0xff;
- c[1] = new_ttl >> 16 & 0xff;
- c[2] = new_ttl >> 8 & 0xff;
- c[3] = new_ttl & 0xff;
- c += 4;
- len -= 4;
- if (len < 0)
+ rr = (void*)ptr;
+ if (len < sizeof(*rr))
+ /* incomplete record */
break;
- /* now the 2 byte rdlen field */
- w = c[0] << 8 | c[1];
- c += w + 2;
- len -= w + 2;
+ /* update the TTL field */
+ memcpy(&rr->ttl, &raw_ttl, sizeof(raw_ttl));
+
+ /* skip to the next record */
+ size_t rr_len = sizeof(*rr) + ntohs(rr->rdlen);
+ ptr += rr_len;
+ len -= rr_len;
}
}
@@ -621,59 +618,57 @@ out:
return FALSE;
}
-static int append_query(unsigned char *buf, unsigned int size,
- const char *query, const char *domain)
+static int append_data(unsigned char *buf, size_t size, const char *data)
{
unsigned char *ptr = buf;
- int len;
-
- debug("query %s domain %s", query, domain);
+ size_t len;
- while (query) {
- const char *tmp;
+ while (true) {
+ const char *dot = strchr(data, '.');
+ len = dot ? dot - data : strlen(data);
- tmp = strchr(query, '.');
- if (!tmp) {
- len = strlen(query);
- if (len == 0)
- break;
- *ptr = len;
- memcpy(ptr + 1, query, len);
- ptr += len + 1;
+ if (len == 0)
break;
- }
+ else if (size < len + 1)
+ return -1;
- *ptr = tmp - query;
- memcpy(ptr + 1, query, tmp - query);
- ptr += tmp - query + 1;
+ *ptr = len;
+ memcpy(ptr + 1, data, len);
+ ptr += len + 1;
+ size -= len + 1;
- query = tmp + 1;
+ if (!dot)
+ break;
+
+ data = dot + 1;
}
- while (domain) {
- const char *tmp;
+ return ptr - buf;
+}
- tmp = strchr(domain, '.');
- if (!tmp) {
- len = strlen(domain);
- if (len == 0)
- break;
- *ptr = len;
- memcpy(ptr + 1, domain, len);
- ptr += len + 1;
- break;
- }
+static int append_query(unsigned char *buf, size_t size,
+ const char *query, const char *domain)
+{
+ debug("query %s domain %s", query, domain);
- *ptr = tmp - domain;
- memcpy(ptr + 1, domain, tmp - domain);
- ptr += tmp - domain + 1;
+ size_t left_size = size;
+ int res = append_data(buf, left_size, query);
+ if (res < 0)
+ return -1;
+ left_size -= res;
- domain = tmp + 1;
- }
+ res = append_data(buf + res, left_size, domain);
+ if (res < 0)
+ return -1;
+ left_size -= res;
- *ptr++ = 0x00;
+ if (left_size == 0)
+ return -1;
- return ptr - buf;
+ const size_t added = size - left_size;
+ *(buf + added) = 0x00;
+
+ return added;
}
static bool cache_check_is_valid(struct cache_data *data, time_t current_time)
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 05/16] dnsproxy: refactor parse_response()
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (3 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 04/16] dnsproxy: refactoring of update_cached_ttl() and append_data() Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 06/16] dnsproxy: refactoring of cache_update() Matthias Gerstner
` (10 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
- add a descriptive comment to make clear what the function does
- use const pointers and size_t where possible
- move stack variables into more localized scopes
- use named constants over literal numbers where applicable
---
src/dnsproxy.c | 123 +++++++++++++++++++++++++------------------------
1 file changed, 63 insertions(+), 60 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index bceedfbc7..bdd09f7ee 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -911,7 +911,7 @@ static int get_name(int counter,
static int parse_rr(const unsigned char *buf, const unsigned char *start,
const unsigned char *max,
- unsigned char *response, unsigned int *response_size,
+ unsigned char *response, size_t *response_size,
uint16_t *type, uint16_t *class, int *ttl, uint16_t *rdlen,
const unsigned char **end,
char *name, size_t max_name)
@@ -972,55 +972,67 @@ static bool check_alias(GSList *aliases, const char *name)
return false;
}
-static int parse_response(unsigned char *buf, int buflen,
- char *question, int qlen,
+/*
+ * Parses the DNS response packet found in 'buf' consisting of 'buflen' bytes.
+ *
+ * The parsed question label, response type and class, ttl and number of
+ * answer sections are output parameters. The response output buffer will
+ * receive all matching resource records to be cached.
+ *
+ * Return value is < 0 on error (negative errno) or zero on success.
+ */
+static int parse_response(const unsigned char *buf, size_t buflen,
+ char *question, size_t qlen,
uint16_t *type, uint16_t *class, int *ttl,
- unsigned char *response, unsigned int *response_len,
+ unsigned char *response, size_t *response_len,
uint16_t *answers)
{
- struct domain_hdr *hdr = (void *) buf;
- struct domain_question *q;
- const unsigned char *ptr;
- uint16_t qdcount = ntohs(hdr->qdcount);
- uint16_t ancount = ntohs(hdr->ancount);
- int err, i;
- uint16_t qtype, qclass;
- const unsigned char *next = NULL;
- unsigned int maxlen = *response_len;
- GSList *aliases = NULL, *list;
- char name[NS_MAXDNAME + 1];
-
- if (buflen < 12)
+ const size_t maxlen = *response_len;
+ *response_len = 0;
+ *answers = 0;
+
+ if (buflen < DNS_HEADER_SIZE)
return -EINVAL;
- debug("qr %d qdcount %d", hdr->qr, qdcount);
+ struct domain_hdr *hdr = (void *) buf;
- /* We currently only cache responses where question count is 1 */
- if (hdr->qr != 1 || qdcount != 1)
- return -EINVAL;
+ {
+ const uint16_t qdcount = ntohs(hdr->qdcount);
+ debug("qr %d qdcount %d", hdr->qr, qdcount);
- ptr = buf + sizeof(struct domain_hdr);
+ /* We currently only cache responses where question count is 1 */
+ if (hdr->qr != 1 || qdcount != 1)
+ return -EINVAL;
+ }
- strncpy(question, (char *) ptr, qlen);
+ const unsigned char *ptr = buf + DNS_HEADER_SIZE;
+ const unsigned char *eptr = buf + buflen;
+
+ /*
+ * NOTE: currently the *caller* ensures that the `question' buffer is
+ * always zero terminated.
+ */
+ strncpy(question, (const char *) ptr, MIN(qlen, buflen - DNS_HEADER_SIZE));
qlen = strlen(question);
ptr += qlen + 1; /* skip \0 */
- q = (void *) ptr;
- qtype = ntohs(q->type);
+ if ((eptr - ptr) < sizeof(struct domain_question))
+ return -EINVAL;
+
+ const struct domain_question *q = (void *) ptr;
+ const uint16_t qtype = ntohs(q->type);
/* We cache only A and AAAA records */
- if (qtype != 1 && qtype != 28)
+ if (qtype != DNS_TYPE_A && qtype != DNS_TYPE_AAAA)
return -ENOMSG;
- qclass = ntohs(q->class);
+ ptr += sizeof(struct domain_question); /* advance to answers section */
- ptr += 2 + 2; /* ptr points now to answers */
-
- err = -ENOMSG;
- *response_len = 0;
- *answers = 0;
-
- memset(name, 0, sizeof(name));
+ int err = -ENOMSG;
+ const uint16_t ancount = ntohs(hdr->ancount);
+ const uint16_t qclass = ntohs(q->class);
+ char name[NS_MAXDNAME + 1] = {0};
+ GSList *aliases = NULL;
/*
* We have a bunch of answers (like A, AAAA, CNAME etc) to
@@ -1028,7 +1040,7 @@ static int parse_response(unsigned char *buf, int buflen,
* resource records. Only A and AAAA records are cached, all
* the other records in answers are skipped.
*/
- for (i = 0; i < ancount; i++) {
+ for (uint16_t i = 0; i < ancount; i++) {
/*
* Get one address at a time to this buffer.
* The max size of the answer is
@@ -1036,24 +1048,27 @@ static int parse_response(unsigned char *buf, int buflen,
* 4 (ttl) + 2 (rdlen) + addr (16 or 4) = 28
* for A or AAAA record.
* For CNAME the size can be bigger.
+ * TODO: why are we using the MAXCDNAME constant as buffer
+ * size then?
*/
- unsigned char rsp[NS_MAXCDNAME];
- unsigned int rsp_len = sizeof(rsp) - 1;
- int ret;
+ unsigned char rsp[NS_MAXCDNAME] = {0};
+ size_t rsp_len = sizeof(rsp) - 1;
+ const unsigned char *next = NULL;
uint16_t rdlen;
- memset(rsp, 0, sizeof(rsp));
-
- ret = parse_rr(buf, ptr, buf + buflen, rsp, &rsp_len,
+ int ret = parse_rr(buf, ptr, buf + buflen, rsp, &rsp_len,
type, class, ttl, &rdlen, &next, name,
sizeof(name) - 1);
if (ret != 0) {
err = ret;
- goto out;
+ break;
}
+ /* set pointer to the next RR for the next iteration */
+ ptr = next;
+
/*
- * Now rsp contains compressed or uncompressed resource
+ * Now rsp contains a compressed or an uncompressed resource
* record. Next we check if this record answers the question.
* The name var contains the uncompressed label.
* One tricky bit is the CNAME records as they alias
@@ -1065,8 +1080,6 @@ static int parse_response(unsigned char *buf, int buflen,
* looking for.
*/
if (*class != qclass) {
- ptr = next;
- next = NULL;
continue;
}
@@ -1091,7 +1104,7 @@ static int parse_response(unsigned char *buf, int buflen,
* address of ipv6.l.google.com. For caching purposes this
* should not cause any issues.
*/
- if (*type == 5 && strncmp(question, name, qlen) == 0) {
+ if (*type == DNS_TYPE_CNAME && strncmp(question, name, qlen) == 0) {
/*
* So now the alias answered the question. This is
* not very useful from caching point of view as
@@ -1115,8 +1128,6 @@ static int parse_response(unsigned char *buf, int buflen,
name, sizeof(name) - 1, &name_len);
if (ret != 0) {
/* just ignore the error at this point */
- ptr = next;
- next = NULL;
continue;
}
@@ -1128,12 +1139,8 @@ static int parse_response(unsigned char *buf, int buflen,
*/
aliases = g_slist_prepend(aliases, g_strdup(name));
- ptr = next;
- next = NULL;
continue;
- }
-
- if (*type == qtype) {
+ } else if (*type == qtype) {
/*
* We found correct type (A or AAAA)
*/
@@ -1150,7 +1157,7 @@ static int parse_response(unsigned char *buf, int buflen,
*/
if (*response_len + rsp_len > maxlen) {
err = -ENOBUFS;
- goto out;
+ break;
}
memcpy(response + *response_len, rsp, rsp_len);
*response_len += rsp_len;
@@ -1158,13 +1165,9 @@ static int parse_response(unsigned char *buf, int buflen,
err = 0;
}
}
-
- ptr = next;
- next = NULL;
}
-out:
- for (list = aliases; list; list = list->next)
+ for (GSList *list = aliases; list; list = list->next)
g_free(list->data);
g_slist_free(aliases);
@@ -1367,7 +1370,7 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
char question[NS_MAXDNAME + 1];
unsigned char response[NS_MAXDNAME + 1];
unsigned char *ptr;
- unsigned int rsplen;
+ size_t rsplen;
bool new_entry = true;
time_t current_time;
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 06/16] dnsproxy: refactoring of cache_update()
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (4 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 05/16] dnsproxy: refactor parse_response() Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 07/16] dnsproxy: strip_domains(): fix out of bounds read access Matthias Gerstner
` (9 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
- move stack variables into more localized scopes
- use const parameters and variables where possible
- use named constants over literal numbers
- simplify some parsing details by using byte order macros or adding
some comments to make the intentions clearer
---
src/dnsproxy.c | 150 +++++++++++++++++++++++--------------------------
1 file changed, 70 insertions(+), 80 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index bdd09f7ee..209120add 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -226,6 +226,7 @@ struct domain_rr {
#define DNS_HEADER_SIZE sizeof(struct domain_hdr)
#define DNS_HEADER_TCP_EXTRA_BYTES 2
+#define DNS_TCP_HEADER_SIZE DNS_HEADER_SIZE + DNS_HEADER_TCP_EXTRA_BYTES
enum dns_type {
/* IPv4 address 32-bit */
@@ -1357,30 +1358,18 @@ static int reply_query_type(const unsigned char *msg, int len)
return type;
}
-static int cache_update(struct server_data *srv, unsigned char *msg,
- unsigned int msg_len)
+/*
+ * update the cache with the DNS reply found in msg
+ */
+static int cache_update(struct server_data *srv, const char *msg, size_t msg_len)
{
- size_t offset = protocol_offset(srv->protocol);
- int err, qlen, ttl = 0;
- uint16_t answers = 0, type = 0, class = 0;
- struct domain_hdr *hdr = (void *)(msg + offset);
- struct domain_question *q;
- struct cache_entry *entry;
- struct cache_data *data;
- char question[NS_MAXDNAME + 1];
- unsigned char response[NS_MAXDNAME + 1];
- unsigned char *ptr;
- size_t rsplen;
- bool new_entry = true;
- time_t current_time;
-
if (cache_size >= MAX_CACHE_SIZE) {
cache_cleanup();
if (cache_size >= MAX_CACHE_SIZE)
return 0;
}
- current_time = time(NULL);
+ const time_t current_time = time(NULL);
/* don't do a cache refresh more than twice a minute */
if (next_refresh < current_time) {
@@ -1388,6 +1377,9 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
next_refresh = current_time + 30;
}
+ const size_t offset = protocol_offset(srv->protocol);
+ struct domain_hdr *hdr = (void *)(msg + offset);
+
debug("offset %zd hdr %p msg %p rcode %d", offset, hdr, msg, hdr->rcode);
/* Continue only if response code is 0 (=ok) */
@@ -1397,10 +1389,13 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
if (!cache)
create_cache();
- rsplen = sizeof(response) - 1;
+ unsigned char response[NS_MAXDNAME + 1];
+ size_t rsplen = sizeof(response) - 1;
+ char question[NS_MAXDNAME + 1];
question[sizeof(question) - 1] = '\0';
-
- err = parse_response(msg + offset, msg_len - offset,
+ int ttl = 0;
+ uint16_t answers = 0, type = 0, class = 0;
+ const int err = parse_response(msg + offset, msg_len - offset,
question, sizeof(question) - 1,
&type, &class, &ttl,
response, &rsplen, &answers);
@@ -1412,26 +1407,29 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
*/
if ((err == -ENOMSG || err == -ENOBUFS) &&
reply_query_type(msg + offset,
- msg_len - offset) == 28) {
- entry = g_hash_table_lookup(cache, question);
+ msg_len - offset) == DNS_TYPE_AAAA) {
+ struct cache_entry *entry = g_hash_table_lookup(cache, question);
if (entry && entry->ipv4 && !entry->ipv6) {
- int cache_offset = 0;
+ struct cache_data *data = g_try_new(struct cache_data, 1);
- data = g_try_new(struct cache_data, 1);
if (!data)
return -ENOMEM;
data->inserted = entry->ipv4->inserted;
data->type = type;
data->answers = ntohs(hdr->ancount);
data->timeout = entry->ipv4->timeout;
- if (srv->protocol == IPPROTO_UDP)
- cache_offset = 2;
- data->data_len = msg_len + cache_offset;
- data->data = ptr = g_malloc(data->data_len);
- ptr[0] = (data->data_len - 2) / 256;
- ptr[1] = (data->data_len - 2) - ptr[0] * 256;
- if (srv->protocol == IPPROTO_UDP)
- ptr += 2;
+ data->data_len = msg_len +
+ (offset ? 0 : DNS_HEADER_TCP_EXTRA_BYTES);
+ data->data = g_malloc(data->data_len);
+ unsigned char *ptr = data->data;
+ if (srv->protocol == IPPROTO_UDP) {
+ /* add the two bytes length header also for
+ * UDP responses */
+ uint16_t *lenhdr = (void*)ptr;
+ *lenhdr = htons(data->data_len -
+ DNS_HEADER_TCP_EXTRA_BYTES);
+ ptr += DNS_HEADER_TCP_EXTRA_BYTES;
+ }
data->valid_until = entry->ipv4->valid_until;
data->cache_until = entry->ipv4->cache_until;
memcpy(ptr, msg, msg_len);
@@ -1440,9 +1438,7 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
* we will get a "hit" when we serve the response
* out of the cache
*/
- entry->hits--;
- if (entry->hits < 0)
- entry->hits = 0;
+ entry->hits = entry->hits ? entry->hits - 1 : 0;
return 0;
}
}
@@ -1450,8 +1446,6 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
if (err < 0 || ttl == 0)
return 0;
- qlen = strlen(question);
-
/*
* If the cache contains already data, check if the
* type of the cached data is the same and do not add
@@ -1459,7 +1453,11 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
* This is needed so that we can cache both A and AAAA
* records for the same name.
*/
- entry = g_hash_table_lookup(cache, question);
+
+ struct cache_entry *entry = g_hash_table_lookup(cache, question);
+ struct cache_data *data = NULL;
+ const bool new_entry = !entry;
+
if (!entry) {
entry = g_try_new(struct cache_entry, 1);
if (!entry)
@@ -1476,37 +1474,28 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
entry->want_refresh = false;
entry->hits = 0;
- if (type == 1)
- entry->ipv4 = data;
- else
- entry->ipv6 = data;
} else {
- if (type == 1 && entry->ipv4)
+ if (type == DNS_TYPE_A && entry->ipv4)
return 0;
-
- if (type == 28 && entry->ipv6)
+ else if (type == DNS_TYPE_AAAA && entry->ipv6)
return 0;
data = g_try_new(struct cache_data, 1);
if (!data)
return -ENOMEM;
- if (type == 1)
- entry->ipv4 = data;
- else
- entry->ipv6 = data;
-
/*
* compensate for the hit we'll get for serving
* the response out of the cache
*/
- entry->hits--;
- if (entry->hits < 0)
- entry->hits = 0;
-
- new_entry = false;
+ entry->hits = entry->hits ? entry->hits - 1 : 0;
}
+ if (type == DNS_TYPE_A)
+ entry->ipv4 = data;
+ else
+ entry->ipv6 = data;
+
if (ttl < MIN_CACHE_TTL)
ttl = MIN_CACHE_TTL;
@@ -1514,14 +1503,21 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
data->type = type;
data->answers = answers;
data->timeout = ttl;
+ data->valid_until = current_time + ttl;
+
+ const size_t qlen = strlen(question);
/*
- * The "2" in start of the length is the TCP offset. We allocate it
- * here even for UDP packet because it simplifies the sending
- * of cached packet.
+ * We allocate the extra TCP header bytes here even for UDP packet
+ * because it simplifies the sending of cached packet.
*/
- data->data_len = 2 + 12 + qlen + 1 + 2 + 2 + rsplen;
- data->data = ptr = g_malloc(data->data_len);
- data->valid_until = current_time + ttl;
+ data->data_len = DNS_TCP_HEADER_SIZE + qlen + 1 + 2 + 2 + rsplen;
+ data->data = g_malloc(data->data_len);
+ if (!data->data) {
+ g_free(entry->key);
+ g_free(data);
+ g_free(entry);
+ return -ENOMEM;
+ }
/*
* Restrict the cached DNS record TTL to some sane value
@@ -1532,36 +1528,30 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
data->cache_until = round_down_ttl(current_time + ttl, ttl);
- if (!data->data) {
- g_free(entry->key);
- g_free(data);
- g_free(entry);
- return -ENOMEM;
- }
+ unsigned char *ptr = data->data;
/*
* We cache the two extra bytes at the start of the message
- * in a TCP packet. When sending UDP packet, we skip the first
+ * in a TCP packet. When sending UDP packet, we pad the first
* two bytes. This way we do not need to know the format
* (UDP/TCP) of the cached message.
*/
- if (srv->protocol == IPPROTO_UDP)
- memcpy(ptr + 2, msg, offset + 12);
- else
- memcpy(ptr, msg, offset + 12);
+ uint16_t *lenhdr = (void*)ptr;
+ *lenhdr = htons(data->data_len - DNS_HEADER_TCP_EXTRA_BYTES);
+ ptr += DNS_HEADER_TCP_EXTRA_BYTES;
- ptr[0] = (data->data_len - 2) / 256;
- ptr[1] = (data->data_len - 2) - ptr[0] * 256;
- if (srv->protocol == IPPROTO_UDP)
- ptr += 2;
+ memcpy(ptr, hdr, DNS_HEADER_SIZE);
+ ptr += DNS_HEADER_SIZE;
- memcpy(ptr + offset + 12, question, qlen + 1); /* copy also the \0 */
+ memcpy(ptr, question, qlen + 1); /* copy also the \0 */
+ ptr += qlen + 1;
- q = (void *) (ptr + offset + 12 + qlen + 1);
+ struct domain_question *q = (void *)ptr;
q->type = htons(type);
q->class = htons(class);
- memcpy(ptr + offset + 12 + qlen + 1 + sizeof(struct domain_question),
- response, rsplen);
+ ptr += sizeof(struct domain_question);
+
+ memcpy(ptr, response, rsplen);
if (new_entry) {
g_hash_table_replace(cache, entry->key, entry);
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 07/16] dnsproxy: strip_domains(): fix out of bounds read access
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (5 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 06/16] dnsproxy: refactoring of cache_update() Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 08/16] dnsproxy: refactor and document strip_domains() to make it less confusing Matthias Gerstner
` (8 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
If the name is not found in an answer record then `ptr` is NULL and the
calculation at the end of the while loop `maxlen -= answers - ptr` will
underflow, resulting in a very large `maxlen` value and consequently in
out of bound read accesses parsing beyond the actual end of the answers
section.
---
src/dnsproxy.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 209120add..bc92787e4 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1881,6 +1881,8 @@ static int strip_domains(char *name, char *answers, int maxlen)
end -= domain_len;
maxlen -= domain_len;
}
+ } else {
+ ptr = answers;
}
answers += strlen(answers) + 1;
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 08/16] dnsproxy: refactor and document strip_domains() to make it less confusing
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (6 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 07/16] dnsproxy: strip_domains(): fix out of bounds read access Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 09/16] dnsproxy: refactor ns_resolv() and forwards_dns_reply() Matthias Gerstner
` (7 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
---
src/dnsproxy.c | 60 +++++++++++++++++++++++++++++++++++---------------
1 file changed, 42 insertions(+), 18 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index bc92787e4..fa517e1be 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1861,44 +1861,68 @@ out:
return NULL;
}
-static int strip_domains(char *name, char *answers, int maxlen)
+/*
+ * removes the qualified domain name part from the given answer sections
+ * starting at 'answers', consisting of 'length' bytes.
+ *
+ * 'name' points the start of the unqualified host label including the leading
+ * length octet.
+ *
+ * returns the new (possibly shorter) length of remaining payload in the
+ * answers buffer, or a negative (errno) value to indicate error conditions.
+ */
+static int strip_domains(const char *name, char *answers, size_t length)
{
+ /* length of the name label including the length header octet */
const size_t name_len = strlen(name);
- const char *start = answers, *end = answers + maxlen;
+ const char *end = answers + length;
- while (maxlen > 0) {
+ while (answers < end) {
char *ptr = strstr(answers, name);
if (ptr) {
char *domain = ptr + name_len;
+ /* this now points to the domain part length octet. */
if (*domain) {
+ /*
+ * length of the rest of the labels up to the
+ * null label (zero byte).
+ */
const size_t domain_len = strlen(domain);
+ char *remaining = domain + domain_len;
- memmove(answers + name_len,
- domain + domain_len,
- end - (domain + domain_len));
+ /*
+ * now shift the rest of the answer sections
+ * to the left to get rid of the domain label
+ * part
+ */
+ memmove(ptr + name_len,
+ remaining,
+ end - remaining);
end -= domain_len;
- maxlen -= domain_len;
+ length -= domain_len;
}
- } else {
- ptr = answers;
}
- answers += strlen(answers) + 1;
- answers += 2 + 2 + 4; /* skip type, class and ttl fields */
-
- uint16_t data_len = answers[0] << 8 | answers[1];
- answers += 2; /* skip the length field */
+ /* skip to the next answer section */
- if (answers + data_len > end)
+ /* the labels up to the root null label */
+ answers += strlen(answers) + 1;
+ /* the fixed part of the RR */
+ const struct domain_rr *rr = (void*)answers;
+ if (answers + sizeof(*rr) > end)
return -EINVAL;
-
+ const uint16_t data_len = htons(rr->rdlen);
+ /* skip the rest of the RR */
+ answers += sizeof(*rr);
answers += data_len;
- maxlen -= answers - ptr;
}
- return end - start;
+ if (answers > end)
+ return -EINVAL;
+
+ return length;
}
static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 09/16] dnsproxy: refactor ns_resolv() and forwards_dns_reply()
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (7 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 08/16] dnsproxy: refactor and document strip_domains() to make it less confusing Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 10/16] dnsproxy: uncompress: replace unnecessary goto with return statements Matthias Gerstner
` (6 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
- document function behaviour in comments
- use early exits where possible to reduce indentation levels
- move stack variables into more localized scopes
- reduce some duplicate code in uncompress() calls
- add TODO about likely logical error that could have ramifications
when fixing.
---
src/dnsproxy.c | 304 +++++++++++++++++++++++++------------------------
1 file changed, 153 insertions(+), 151 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index fa517e1be..d76cb0d2e 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1571,39 +1571,40 @@ static int cache_update(struct server_data *srv, const char *msg, size_t msg_len
return 0;
}
-static int ns_resolv(struct server_data *server, struct request_data *req,
- gpointer request, gpointer name)
+/*
+ * attempts to answer the given request from cached replies.
+ *
+ * returns:
+ * > 0 on cache hit (answer is already sent out to client)
+ * == 0 on cache miss
+ * < 0 on error condition (errno)
+ */
+static int ns_try_resolv_from_cache(
+ struct request_data *req, gpointer request, const char *lookup)
{
- GList *list;
- int sk, err;
uint16_t type = 0;
- char *dot, *lookup = (char *) name;
- struct cache_entry *entry;
+ struct cache_entry *entry = cache_check(request, &type, req->protocol);
+ if (!entry)
+ return 0;
- entry = cache_check(request, &type, req->protocol);
- if (entry) {
- int ttl_left = 0;
- struct cache_data *data;
+ debug("cache hit %s type %s", lookup, type == 1 ? "A" : "AAAA");
- debug("cache hit %s type %s", lookup, type == 1 ? "A" : "AAAA");
- if (type == 1)
- data = entry->ipv4;
- else
- data = entry->ipv6;
+ const struct cache_data *data = type == DNS_TYPE_A ?
+ entry->ipv4 : entry->ipv6;
- if (data) {
- ttl_left = data->valid_until - time(NULL);
- entry->hits++;
- }
+ if (!data)
+ return 0;
+
+ int ttl_left = data->valid_until - time(NULL);
+ entry->hits++;
- if (data && req->protocol == IPPROTO_TCP) {
+ switch(req->protocol) {
+ case IPPROTO_TCP:
send_cached_response(req->client_sk, data->data,
data->data_len, NULL, 0, IPPROTO_TCP,
req->srcid, data->answers, ttl_left);
return 1;
- }
-
- if (data && req->protocol == IPPROTO_UDP) {
+ case IPPROTO_UDP: {
int udp_sk = get_req_udp_socket(req);
if (udp_sk < 0)
@@ -1617,7 +1618,24 @@ static int ns_resolv(struct server_data *server, struct request_data *req,
}
}
- sk = g_io_channel_unix_get_fd(server->channel);
+ return -EINVAL;
+}
+
+static int ns_resolv(struct server_data *server, struct request_data *req,
+ gpointer request, gpointer name)
+{
+ const char *lookup = (const char *)name;
+
+ int err = ns_try_resolv_from_cache(req, request, lookup);
+ if (err > 0)
+ /* cache hit */
+ return 1;
+ else if (err != 0)
+ /* error other than cache miss, don't continue */
+ return err;
+
+ /* forward request to real DNS server */
+ int sk = g_io_channel_unix_get_fd(server->channel);
err = sendto(sk, request, req->request_len, MSG_NOSIGNAL,
server->server_addr, server->server_addr_len);
@@ -1632,51 +1650,50 @@ static int ns_resolv(struct server_data *server, struct request_data *req,
req->numserv++;
/* If we have more than one dot, we don't add domains */
- dot = strchr(lookup, '.');
- if (dot && dot != lookup + strlen(lookup) - 1)
- return 0;
+ {
+ const char *dot = strchr(lookup, '.');
+ if (dot && dot != lookup + strlen(lookup) - 1)
+ return 0;
+ }
if (server->domains && server->domains->data)
req->append_domain = true;
- for (list = server->domains; list; list = list->next) {
- char *domain;
- unsigned char alt[1024];
- struct domain_hdr *hdr = (void *) &alt;
- int altlen, domlen;
- size_t offset = protocol_offset(server->protocol);
-
- domain = list->data;
-
+ for (GList *list = server->domains; list; list = list->next) {
+ const char *domain = list->data;
if (!domain)
continue;
- domlen = strlen(domain) + 1;
+ const int domlen = strlen(domain) + 1;
if (domlen < 5)
return -EINVAL;
- alt[offset] = req->altid & 0xff;
- alt[offset + 1] = req->altid >> 8;
+ const size_t offset = protocol_offset(server->protocol);
+ unsigned char alt[1024];
+ /* TODO: is this a bug? the offset isn't considered here... */
+ struct domain_hdr *hdr = (void *) &alt;
+
+ memcpy(alt + offset, &req->altid, sizeof(req->altid));
memcpy(alt + offset + 2, request + offset + 2, 10);
hdr->qdcount = htons(1);
- altlen = append_query(alt + offset + 12, sizeof(alt) - 12,
+ int altlen = append_query(alt + offset + DNS_HEADER_SIZE, sizeof(alt) - DNS_HEADER_SIZE,
name, domain);
if (altlen < 0)
return -EINVAL;
- altlen += 12;
+ altlen += DNS_HEADER_SIZE;
+ altlen += offset;
- memcpy(alt + offset + altlen,
- request + offset + altlen - domlen,
- req->request_len - altlen - offset + domlen);
+ memcpy(alt + altlen,
+ request + altlen - domlen,
+ req->request_len - altlen + domlen);
if (server->protocol == IPPROTO_TCP) {
- int req_len = req->request_len + domlen - 2;
-
- alt[0] = (req_len >> 8) & 0xff;
- alt[1] = req_len & 0xff;
+ uint16_t req_len = req->request_len + domlen - DNS_HEADER_TCP_EXTRA_BYTES;
+ uint16_t *len_hdr = (void*)alt;
+ *len_hdr = htons(req_len);
}
debug("req %p dstid 0x%04x altid 0x%04x", req, req->dstid,
@@ -1925,94 +1942,83 @@ static int strip_domains(const char *name, char *answers, size_t length)
return length;
}
-static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
+static int forward_dns_reply(char *reply, size_t reply_len, int protocol,
struct server_data *data)
{
- struct domain_hdr *hdr;
- struct request_data *req;
- int dns_id, sk, err;
- size_t offset = protocol_offset(protocol);
-
- if (reply_len < 0)
- return -EINVAL;
- if ((size_t)reply_len < offset + 1)
- return -EINVAL;
- if ((size_t)reply_len < sizeof(struct domain_hdr))
- return -EINVAL;
+ const size_t offset = protocol_offset(protocol);
+ struct domain_hdr *hdr = (void *)(reply + offset);
- hdr = (void *)(reply + offset);
- dns_id = reply[offset] | reply[offset + 1] << 8;
+ debug("Received %zd bytes (id 0x%04x)", reply_len, hdr->id);
- debug("Received %d bytes (id 0x%04x)", reply_len, dns_id);
+ if (reply_len < sizeof(struct domain_hdr) + offset)
+ return -EINVAL;
- req = find_request(dns_id);
+ struct request_data *req = find_request(hdr->id);
if (!req)
return -EINVAL;
debug("req %p dstid 0x%04x altid 0x%04x rcode %d",
req, req->dstid, req->altid, hdr->rcode);
- reply[offset] = req->srcid & 0xff;
- reply[offset + 1] = req->srcid >> 8;
+ /* replace with original request ID from our client */
+ hdr->id = req->srcid;
req->numresp++;
if (hdr->rcode == ns_r_noerror || !req->resp) {
- unsigned char *new_reply = NULL;
+ char *new_reply = NULL;
/*
- * If the domain name was append
- * remove it before forwarding the reply.
- * If there were more than one question, then this
- * domain name ripping can be hairy so avoid that
- * and bail out in that that case.
+ * If the domain name was appended remove it before forwarding
+ * the reply. If there were more than one question, then this
+ * domain name ripping can be hairy so avoid that and bail out
+ * in that that case.
*
- * The reason we are doing this magic is that if the
- * user's DNS client tries to resolv hostname without
- * domain part, it also expects to get the result without
- * a domain name part.
+ * The reason we are doing this magic is that if the user's
+ * DNS client tries to resolv hostname without domain part, it
+ * also expects to get the result without a domain name part.
*/
if (req->append_domain && ntohs(hdr->qdcount) == 1) {
- uint16_t domain_len = 0;
- uint16_t header_len, payload_len;
- uint16_t dns_type, dns_class;
- uint8_t host_len, dns_type_pos;
- char uncompressed[NS_MAXDNAME], *uptr;
- const char *ptr, *eom = (char *)reply + reply_len;
- const char *domain;
+ const char *eom = reply + reply_len;
+ const uint16_t header_len = offset + DNS_HEADER_SIZE;
+ if (reply_len < header_len)
+ return -EINVAL;
+ const uint16_t payload_len = reply_len - header_len;
+ if (payload_len < 1)
+ return -EINVAL;
/*
* ptr points to the first char of the hostname.
* ->hostname.domain.net
*/
- header_len = offset + sizeof(struct domain_hdr);
- if (reply_len < header_len)
- return -EINVAL;
- payload_len = reply_len - header_len;
-
- ptr = (char *)reply + header_len;
+ const char *ptr = reply + header_len;
- host_len = *ptr;
- domain = ptr + 1 + host_len;
- if (domain > eom)
+ const uint8_t host_len = *ptr;
+ const char *domain = ptr + 1 + host_len;
+ if (domain >= eom)
return -EINVAL;
- if (host_len > 0)
- domain_len = strnlen(domain, eom - domain);
+ const uint16_t domain_len = host_len ?
+ strnlen(domain, eom - domain) : 0;
/*
* If the query type is anything other than A or AAAA,
* then bail out and pass the message as is.
* We only want to deal with IPv4 or IPv6 addresses.
*/
- dns_type_pos = host_len + 1 + domain_len + 1;
-
- if (ptr + (dns_type_pos + 3) > eom)
+ const struct qtype_qclass *qtc =
+ (void*)(domain + domain_len + 1);
+ if (((const char*)(qtc + 1)) > eom)
return -EINVAL;
- dns_type = ptr[dns_type_pos] << 8 |
- ptr[dns_type_pos + 1];
- dns_class = ptr[dns_type_pos + 2] << 8 |
- ptr[dns_type_pos + 3];
+
+ const uint16_t dns_type = ntohs(qtc->qtype);
+ const uint16_t dns_class = ntohs(qtc->qclass);
+
+ /* TODO: this condition looks wrong it should be
+ * (dns_type != A && dns_type != AAAA) || dns_class != IN)
+ * however then the behaviour of dnsproxy changes,
+ * e.g. MX records will be passed back to the client,
+ * but without adjustment of the appended DNS name. */
if (dns_type != DNS_TYPE_A && dns_type != DNS_TYPE_AAAA &&
dns_class != DNS_CLASS_IN) {
debug("Pass msg dns type %d class %d",
@@ -2034,17 +2040,17 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
* case we end up in this branch.
*/
if (domain_len > 0) {
- int len = host_len + 1;
- int new_len, fixed_len;
- char *answers;
+ char uncompressed[NS_MAXDNAME];
+ const size_t len = host_len + 1;
+
+ /* NOTE: length checks up and including to
+ * qtype_qclass have already been done above */
- if (len > payload_len)
- return -EINVAL;
/*
* First copy host (without domain name) into
* tmp buffer.
*/
- uptr = &uncompressed[0];
+ char *uptr = &uncompressed[0];
memcpy(uptr, ptr, len);
uptr[len] = '\0'; /* host termination */
@@ -2053,20 +2059,15 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
/*
* Copy type and class fields of the question.
*/
- ptr += len + domain_len + 1;
- if (ptr + NS_QFIXEDSZ > eom)
- return -EINVAL;
- memcpy(uptr, ptr, NS_QFIXEDSZ);
+ memcpy(uptr, qtc, sizeof(*qtc));
/*
* ptr points to answers after this
*/
- ptr += NS_QFIXEDSZ;
- uptr += NS_QFIXEDSZ;
- answers = uptr;
- fixed_len = answers - uncompressed;
- if (ptr + offset > eom)
- return -EINVAL;
+ ptr = (void*)(qtc + 1);
+ uptr += sizeof(*qtc);
+ char *answers = uptr;
+ const size_t fixed_len = answers - uncompressed;
/*
* We then uncompress the result to buffer
@@ -2075,41 +2076,36 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
* then name server (authority) information,
* and finally additional record info.
*/
-
- ptr = uncompress(ntohs(hdr->ancount),
- (char *)reply + offset, eom,
- ptr, uncompressed, NS_MAXDNAME,
- &uptr);
- if (!ptr)
- goto out;
-
- ptr = uncompress(ntohs(hdr->nscount),
- (char *)reply + offset, eom,
- ptr, uncompressed, NS_MAXDNAME,
- &uptr);
- if (!ptr)
- goto out;
-
- ptr = uncompress(ntohs(hdr->arcount),
- (char *)reply + offset, eom,
- ptr, uncompressed, NS_MAXDNAME,
- &uptr);
- if (!ptr)
- goto out;
+ const uint16_t section_counts[] = {
+ hdr->ancount,
+ hdr->nscount,
+ hdr->arcount
+ };
+
+ for (size_t i = 0; i < sizeof(section_counts) / sizeof(uint16_t); i++) {
+ ptr = uncompress(ntohs(section_counts[i]),
+ reply + offset, eom,
+ ptr, uncompressed, NS_MAXDNAME,
+ &uptr);
+ if (!ptr)
+ goto out;
+ }
/*
- * The uncompressed buffer now contains almost
- * valid response. Final step is to get rid of
- * the domain name because at least glibc
- * gethostbyname() implementation does extra
- * checks and expects to find an answer without
- * domain name if we asked a query without
- * domain part. Note that glibc getaddrinfo()
- * works differently and accepts FQDN in answer
+ * The uncompressed buffer now contains an
+ * almost valid response. Final step is to get
+ * rid of the domain name because at least
+ * glibc gethostbyname() implementation does
+ * extra checks and expects to find an answer
+ * without domain name if we asked a query
+ * without domain part. Note that glibc
+ * getaddrinfo() works differently and accepts
+ * FQDN in answer
*/
- new_len = strip_domains(uncompressed, answers,
+ const int new_an_len = strip_domains(
+ uncompressed, answers,
uptr - answers);
- if (new_len < 0) {
+ if (new_an_len < 0) {
debug("Corrupted packet");
return -EINVAL;
}
@@ -2118,9 +2114,13 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
* Because we have now uncompressed the answers
* we might have to create a bigger buffer to
* hold all that data.
+ *
+ * TODO: only create bigger buffer if
+ * actually necessary, pass allocation size of
+ * buffer via additional parameter.
*/
- reply_len = header_len + new_len + fixed_len;
+ reply_len = header_len + new_an_len + fixed_len;
new_reply = g_try_malloc(reply_len);
if (!new_reply)
@@ -2128,7 +2128,7 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
memcpy(new_reply, reply, header_len);
memcpy(new_reply + header_len, uncompressed,
- new_len + fixed_len);
+ new_an_len + fixed_len);
reply = new_reply;
}
@@ -2161,6 +2161,8 @@ out:
request_list = g_slist_remove(request_list, req);
+ int err, sk;
+
if (protocol == IPPROTO_UDP) {
sk = get_req_udp_socket(req);
if (sk < 0) {
@@ -2170,7 +2172,7 @@ out:
err = sendto(sk, req->resp, req->resplen, 0,
&req->sa, req->sa_len);
} else {
- uint16_t tcp_len = htons(req->resplen - 2);
+ const uint16_t tcp_len = htons(req->resplen - DNS_HEADER_TCP_EXTRA_BYTES);
/* correct TCP message length */
memcpy(req->resp, &tcp_len, sizeof(tcp_len));
sk = req->client_sk;
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 10/16] dnsproxy: uncompress: replace unnecessary goto with return statements
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (8 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 09/16] dnsproxy: refactor ns_resolv() and forwards_dns_reply() Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 11/16] dnsproxy: forward_dns_reply: pull out separate dns_reply_fixup_domains() Matthias Gerstner
` (5 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
---
src/dnsproxy.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index d76cb0d2e..f88e501f1 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1756,7 +1756,7 @@ static const char* uncompress(int16_t field_count, const char *start, const char
if (!convert_label(start, end, ptr, name, NS_MAXLABEL,
&pos, &comp_pos))
- goto out;
+ return NULL;
/*
* Copy the uncompressed resource record, type, class and \0 to
@@ -1765,7 +1765,7 @@ static const char* uncompress(int16_t field_count, const char *start, const char
ulen = strlen(name) + 1;
if ((uptr + ulen) > uncomp_end)
- goto out;
+ return NULL;
memcpy(uptr, name, ulen);
debug("pos %d ulen %d left %d name %s", pos, ulen,
@@ -1781,7 +1781,7 @@ static const char* uncompress(int16_t field_count, const char *start, const char
*/
if ((uptr + NS_RRFIXEDSZ) > uncomp_end) {
debug("uncompressed data too large for buffer");
- goto out;
+ return NULL;
}
memcpy(uptr, ptr, NS_RRFIXEDSZ);
@@ -1789,7 +1789,7 @@ static const char* uncompress(int16_t field_count, const char *start, const char
dns_class = uptr[2] << 8 | uptr[3];
if (dns_class != DNS_CLASS_IN)
- goto out;
+ return NULL;
ptr += NS_RRFIXEDSZ;
uptr += NS_RRFIXEDSZ;
@@ -1803,7 +1803,7 @@ static const char* uncompress(int16_t field_count, const char *start, const char
if (!convert_label(start, end, ptr, uptr,
uncomp_len - (uptr - uncompressed),
&pos, &comp_pos))
- goto out;
+ return NULL;
uptr[-2] = comp_pos << 8;
uptr[-1] = comp_pos & 0xff;
@@ -1816,7 +1816,7 @@ static const char* uncompress(int16_t field_count, const char *start, const char
if ((ptr + dlen) > end || (uptr + dlen) > uncomp_end) {
debug("data len %d too long", dlen);
- goto out;
+ return NULL;
}
memcpy(uptr, ptr, dlen);
@@ -1831,7 +1831,7 @@ static const char* uncompress(int16_t field_count, const char *start, const char
if (!convert_label(start, end, ptr, uptr,
uncomp_len - (uptr - uncompressed),
&pos, &comp_pos))
- goto out;
+ return NULL;
total_len += comp_pos;
len_ptr = &uptr[-2];
@@ -1842,7 +1842,7 @@ static const char* uncompress(int16_t field_count, const char *start, const char
if (!convert_label(start, end, ptr, uptr,
uncomp_len - (uptr - uncompressed),
&pos, &comp_pos))
- goto out;
+ return NULL;
total_len += comp_pos;
ptr += pos;
@@ -1855,7 +1855,7 @@ static const char* uncompress(int16_t field_count, const char *start, const char
*/
if ((uptr + 20) > uncomp_end || (ptr + 20) > end) {
debug("soa record too long");
- goto out;
+ return NULL;
}
memcpy(uptr, ptr, 20);
uptr += 20;
@@ -1873,9 +1873,6 @@ static const char* uncompress(int16_t field_count, const char *start, const char
}
return ptr;
-
-out:
- return NULL;
}
/*
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 11/16] dnsproxy: forward_dns_reply: pull out separate dns_reply_fixup_domains()
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (9 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 10/16] dnsproxy: uncompress: replace unnecessary goto with return statements Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 12/16] dnsproxy: finish first pass of refactoring the compilation unit Matthias Gerstner
` (4 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
This should make the code logic a bit clearer and less convoluted.
---
src/dnsproxy.c | 331 ++++++++++++++++++++++++++-----------------------
1 file changed, 176 insertions(+), 155 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index f88e501f1..81085fae9 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -47,6 +47,8 @@
# define debug(fmt...) do { } while (0)
#endif
+#define NUM_ARRAY_ELEMENTS(a) sizeof(a) / sizeof(a[0])
+
#if __BYTE_ORDER == __LITTLE_ENDIAN
struct domain_hdr {
uint16_t id;
@@ -1939,6 +1941,167 @@ static int strip_domains(const char *name, char *answers, size_t length)
return length;
}
+/*
+ * Removes domain names from replies, if one has been appended during
+ * forwarding to the real DNS server.
+ *
+ * Returns:
+ * < 0 on error (abort processing reply)
+ * == 0 if the reply should be forwarded unmodified
+ * > 0 and new buffer in *new_reply on success. The return value indicates the
+ * new length of the data in *new_reply.
+ */
+static int dns_reply_fixup_domains(
+ const char *reply, size_t reply_len,
+ const size_t offset,
+ struct request_data *req,
+ char **new_reply)
+{
+ const struct domain_hdr *hdr = (void *)(reply + offset);
+ const char *eom = reply + reply_len;
+ const uint16_t header_len = offset + DNS_HEADER_SIZE;
+ /* full header plus at least one byte for the hostname length */
+ if (reply_len < header_len + 1)
+ return -EINVAL;
+
+ /*
+ * length octet of the hostname.
+ * ->hostname.domain.net
+ */
+ const char *ptr = reply + header_len;
+ const uint8_t host_len = *ptr;
+ const char *domain = ptr + host_len + 1;
+ if (domain >= eom)
+ return -EINVAL;
+
+ const uint16_t domain_len = host_len ?
+ strnlen(domain, eom - domain) : 0;
+
+ /*
+ * If the query type is anything other than A or AAAA, then bail out
+ * and pass the message as is. We only want to deal with IPv4 or IPv6
+ * addresses.
+ */
+ const struct qtype_qclass *qtc = (void*)(domain + domain_len + 1);
+ if (((const char*)(qtc + 1)) > eom)
+ return -EINVAL;
+
+ const uint16_t dns_type = ntohs(qtc->qtype);
+ const uint16_t dns_class = ntohs(qtc->qclass);
+
+ if (domain_len == 0) {
+ /* nothing to do */
+ return 0;
+ }
+
+ /* TODO: This condition looks wrong. It should probably be
+ *
+ * (dns_type != A && dns_type != AAAA) || dns_class != IN
+ *
+ * doing so, however, changes the behaviour of dnsproxy, e.g. MX
+ * records will be passed back to the client, but without the
+ * adjustment of the appended domain name.
+ */
+ if (dns_type != DNS_TYPE_A && dns_type != DNS_TYPE_AAAA &&
+ dns_class != DNS_CLASS_IN) {
+ debug("Pass msg dns type %d class %d", dns_type, dns_class);
+ return 0;
+ }
+
+ /*
+ * Remove the domain name and replace it by the end of reply. Check if
+ * the domain is really there before trying to copy the data. We also
+ * need to uncompress the answers if necessary. The domain_len can be
+ * 0 because if the original query did not contain a domain name, then
+ * we are sending two packets, first without the domain name and the
+ * second packet with domain name. The append_domain is set to true
+ * even if we sent the first packet without domain name. In this case
+ * we end up in this branch.
+ */
+
+ char uncompressed[NS_MAXDNAME];
+
+ /* NOTE: length checks up and including to qtype_qclass have already
+ been done above */
+
+ /*
+ * First copy host (without domain name) into tmp buffer.
+ */
+ char *uptr = &uncompressed[0];
+ memcpy(uptr, ptr, host_len + 1);
+
+ uptr[host_len + 1] = '\0'; /* host termination */
+ uptr += host_len + 2;
+
+ /*
+ * Copy type and class fields of the question.
+ */
+ memcpy(uptr, qtc, sizeof(*qtc));
+
+ /*
+ * ptr points to answers after this
+ */
+ ptr = (void*)(qtc + 1);
+ uptr += sizeof(*qtc);
+ char *answers = uptr;
+ const size_t fixed_len = answers - uncompressed;
+
+ /*
+ * We then uncompress the result to buffer so that we can rip off the
+ * domain name part from the question. First answers, then name server
+ * (authority) information, and finally additional record info.
+ */
+ const uint16_t section_counts[] = {
+ hdr->ancount,
+ hdr->nscount,
+ hdr->arcount
+ };
+
+ for (size_t i = 0; i < NUM_ARRAY_ELEMENTS(section_counts); i++) {
+ ptr = uncompress(ntohs(section_counts[i]), reply + offset, eom,
+ ptr, uncompressed, NS_MAXDNAME, &uptr);
+ if (!ptr) {
+ /* failed to uncompress, pass on as is
+ * (TODO: good idea?) */
+ return 0;
+ }
+ }
+
+ /*
+ * The uncompressed buffer now contains an almost valid response.
+ * Final step is to get rid of the domain name because at least glibc
+ * gethostbyname() implementation does extra checks and expects to
+ * find an answer without domain name if we asked a query without
+ * domain part. Note that glibc getaddrinfo() works differently and
+ * accepts FQDN in answer
+ */
+ const int new_an_len = strip_domains(uncompressed, answers,
+ uptr - answers);
+ if (new_an_len < 0) {
+ debug("Corrupted packet");
+ return -EINVAL;
+ }
+
+ /*
+ * Because we have now uncompressed the answers we might have to
+ * create a bigger buffer to hold all that data.
+ *
+ * TODO: only create a bigger buffer if actually necessary, pass
+ * allocation size of input buffer via additional parameter.
+ */
+
+ reply_len = header_len + new_an_len + fixed_len;
+
+ *new_reply = g_try_malloc(reply_len);
+ if (!*new_reply)
+ return -ENOMEM;
+
+ memcpy(*new_reply, reply, header_len);
+ memcpy(*new_reply + header_len, uncompressed, new_an_len + fixed_len);
+
+ return reply_len;
+}
+
static int forward_dns_reply(char *reply, size_t reply_len, int protocol,
struct server_data *data)
{
@@ -1963,8 +2126,6 @@ static int forward_dns_reply(char *reply, size_t reply_len, int protocol,
req->numresp++;
if (hdr->rcode == ns_r_noerror || !req->resp) {
- char *new_reply = NULL;
-
/*
* If the domain name was appended remove it before forwarding
* the reply. If there were more than one question, then this
@@ -1975,163 +2136,24 @@ static int forward_dns_reply(char *reply, size_t reply_len, int protocol,
* DNS client tries to resolv hostname without domain part, it
* also expects to get the result without a domain name part.
*/
- if (req->append_domain && ntohs(hdr->qdcount) == 1) {
- const char *eom = reply + reply_len;
- const uint16_t header_len = offset + DNS_HEADER_SIZE;
- if (reply_len < header_len)
- return -EINVAL;
- const uint16_t payload_len = reply_len - header_len;
- if (payload_len < 1)
- return -EINVAL;
-
- /*
- * ptr points to the first char of the hostname.
- * ->hostname.domain.net
- */
- const char *ptr = reply + header_len;
-
- const uint8_t host_len = *ptr;
- const char *domain = ptr + 1 + host_len;
- if (domain >= eom)
- return -EINVAL;
-
- const uint16_t domain_len = host_len ?
- strnlen(domain, eom - domain) : 0;
-
- /*
- * If the query type is anything other than A or AAAA,
- * then bail out and pass the message as is.
- * We only want to deal with IPv4 or IPv6 addresses.
- */
- const struct qtype_qclass *qtc =
- (void*)(domain + domain_len + 1);
- if (((const char*)(qtc + 1)) > eom)
- return -EINVAL;
-
- const uint16_t dns_type = ntohs(qtc->qtype);
- const uint16_t dns_class = ntohs(qtc->qclass);
-
- /* TODO: this condition looks wrong it should be
- * (dns_type != A && dns_type != AAAA) || dns_class != IN)
- * however then the behaviour of dnsproxy changes,
- * e.g. MX records will be passed back to the client,
- * but without adjustment of the appended DNS name. */
- if (dns_type != DNS_TYPE_A && dns_type != DNS_TYPE_AAAA &&
- dns_class != DNS_CLASS_IN) {
- debug("Pass msg dns type %d class %d",
- dns_type, dns_class);
- goto pass;
- }
-
- /*
- * Remove the domain name and replace it by the end
- * of reply. Check if the domain is really there
- * before trying to copy the data. We also need to
- * uncompress the answers if necessary.
- * The domain_len can be 0 because if the original
- * query did not contain a domain name, then we are
- * sending two packets, first without the domain name
- * and the second packet with domain name.
- * The append_domain is set to true even if we sent
- * the first packet without domain name. In this
- * case we end up in this branch.
- */
- if (domain_len > 0) {
- char uncompressed[NS_MAXDNAME];
- const size_t len = host_len + 1;
-
- /* NOTE: length checks up and including to
- * qtype_qclass have already been done above */
-
- /*
- * First copy host (without domain name) into
- * tmp buffer.
- */
- char *uptr = &uncompressed[0];
- memcpy(uptr, ptr, len);
-
- uptr[len] = '\0'; /* host termination */
- uptr += len + 1;
-
- /*
- * Copy type and class fields of the question.
- */
- memcpy(uptr, qtc, sizeof(*qtc));
-
- /*
- * ptr points to answers after this
- */
- ptr = (void*)(qtc + 1);
- uptr += sizeof(*qtc);
- char *answers = uptr;
- const size_t fixed_len = answers - uncompressed;
-
- /*
- * We then uncompress the result to buffer
- * so that we can rip off the domain name
- * part from the question. First answers,
- * then name server (authority) information,
- * and finally additional record info.
- */
- const uint16_t section_counts[] = {
- hdr->ancount,
- hdr->nscount,
- hdr->arcount
- };
-
- for (size_t i = 0; i < sizeof(section_counts) / sizeof(uint16_t); i++) {
- ptr = uncompress(ntohs(section_counts[i]),
- reply + offset, eom,
- ptr, uncompressed, NS_MAXDNAME,
- &uptr);
- if (!ptr)
- goto out;
- }
-
- /*
- * The uncompressed buffer now contains an
- * almost valid response. Final step is to get
- * rid of the domain name because at least
- * glibc gethostbyname() implementation does
- * extra checks and expects to find an answer
- * without domain name if we asked a query
- * without domain part. Note that glibc
- * getaddrinfo() works differently and accepts
- * FQDN in answer
- */
- const int new_an_len = strip_domains(
- uncompressed, answers,
- uptr - answers);
- if (new_an_len < 0) {
- debug("Corrupted packet");
- return -EINVAL;
- }
-
- /*
- * Because we have now uncompressed the answers
- * we might have to create a bigger buffer to
- * hold all that data.
- *
- * TODO: only create bigger buffer if
- * actually necessary, pass allocation size of
- * buffer via additional parameter.
- */
-
- reply_len = header_len + new_an_len + fixed_len;
-
- new_reply = g_try_malloc(reply_len);
- if (!new_reply)
- return -ENOMEM;
-
- memcpy(new_reply, reply, header_len);
- memcpy(new_reply + header_len, uncompressed,
- new_an_len + fixed_len);
+ char *new_reply = NULL;
+ if (req->append_domain && ntohs(hdr->qdcount) == 1) {
+ const int fixup_res = dns_reply_fixup_domains(
+ reply, reply_len,
+ offset, req, &new_reply);
+ if (fixup_res < 0) {
+ /* error occured */
+ return fixup_res;
+ } else if (fixup_res > 0 && new_reply) {
+ /* new reply length */
+ reply_len = fixup_res;
reply = new_reply;
+ } else {
+ /* keep message as is */
}
}
- pass:
g_free(req->resp);
req->resplen = 0;
@@ -2147,7 +2169,6 @@ static int forward_dns_reply(char *reply, size_t reply_len, int protocol,
g_free(new_reply);
}
-out:
if (req->numresp < req->numserv) {
if (hdr->rcode > ns_r_noerror) {
return -EINVAL;
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 12/16] dnsproxy: finish first pass of refactoring the compilation unit
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (10 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 11/16] dnsproxy: forward_dns_reply: pull out separate dns_reply_fixup_domains() Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 13/16] dnsproxy: fix TCP server reply handling if domain name is appended Matthias Gerstner
` (3 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
- make variable declarations more local, if possible
- use more const variables where suitable
- more harmonized use of integer types (especially use size_t for buffer
lengths)
- avoid duplicate or difficult to read code portions
---
src/dnsproxy.c | 540 +++++++++++++++++++++----------------------------
1 file changed, 236 insertions(+), 304 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 81085fae9..758337ec2 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -242,7 +242,8 @@ enum dns_type {
};
enum dns_class {
- DNS_CLASS_IN = ns_c_in
+ DNS_CLASS_IN = ns_c_in,
+ DNS_CLASS_ANY = ns_c_any /* only valid for QCLASS fields */
};
static int cache_size;
@@ -284,6 +285,31 @@ static size_t protocol_offset(int protocol)
}
}
+static const char* protocol_label(int protocol)
+{
+ switch(protocol) {
+ case IPPROTO_UDP:
+ return "UDP";
+ case IPPROTO_TCP:
+ return "TCP";
+ default:
+ return "BAD_PROTOCOL";
+ }
+}
+
+static int socket_type(int protocol, int extra_flags)
+{
+ switch (protocol) {
+ case IPPROTO_UDP:
+ return SOCK_DGRAM | extra_flags;
+ case IPPROTO_TCP:
+ return SOCK_STREAM | extra_flags;
+ default:
+ /* this should never happen */
+ abort();
+ }
+}
+
/*
* There is a power and efficiency benefit to have entries
* in our cache expire at the same time. To this extend,
@@ -440,32 +466,22 @@ static void update_cached_ttl(unsigned char *ptr, int len, int new_ttl)
}
}
-static void send_cached_response(int sk, unsigned char *ptr, int len,
+static void send_cached_response(int sk, const unsigned char *ptr, size_t len,
const struct sockaddr *to, socklen_t tolen,
int protocol, int id, uint16_t answers, int ttl)
{
- int offset, dns_len;
-
+ const size_t offset = protocol_offset(protocol);
/*
* The cached packet contains always the TCP offset (two bytes)
* so skip them for UDP.
*/
- switch (protocol) {
- case IPPROTO_UDP:
- ptr += 2;
- len -= 2;
- dns_len = len;
- offset = 0;
- break;
- case IPPROTO_TCP:
- offset = 2;
- dns_len = ptr[0] * 256 + ptr[1];
- break;
- default:
- return;
- }
+ const size_t skip_bytes = offset ? 0 : DNS_HEADER_TCP_EXTRA_BYTES;
+ ptr += skip_bytes;
+ len -= skip_bytes;
+ const size_t dns_len = protocol == IPPROTO_UDP ? len : ntohs(*((uint16_t*)ptr));
+
- if (len < 12)
+ if (len < DNS_HEADER_SIZE)
return;
struct domain_hdr *hdr = (void *) (ptr + offset);
@@ -485,7 +501,7 @@ static void send_cached_response(int sk, unsigned char *ptr, int len,
update_cached_ttl((unsigned char *)hdr, adj_len, ttl);
}
- debug("sk %d id 0x%04x answers %d ptr %p length %d dns %d",
+ debug("sk %d id 0x%04x answers %d ptr %p length %zd dns %zd",
sk, hdr->id, answers, ptr, len, dns_len);
const int res = sendto(sk, ptr, len, MSG_NOSIGNAL, to, tolen);
@@ -494,7 +510,7 @@ static void send_cached_response(int sk, unsigned char *ptr, int len,
strerror(errno));
}
else if (res != len || dns_len != (len - offset))
- debug("Packet length mismatch, sent %d wanted %d dns %d",
+ debug("Packet length mismatch, sent %d wanted %zd dns %zd",
res, len, dns_len);
}
@@ -2289,10 +2305,8 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
static gboolean tcp_server_event(GIOChannel *channel, GIOCondition condition,
gpointer user_data)
{
- int sk;
struct server_data *server = user_data;
-
- sk = g_io_channel_unix_get_fd(channel);
+ int sk = g_io_channel_unix_get_fd(channel);
if (sk == 0)
return FALSE;
@@ -2311,13 +2325,11 @@ hangup:
list = request_list;
while (list) {
struct request_data *req = list->data;
- struct domain_hdr *hdr;
list = list->next;
if (req->protocol == IPPROTO_UDP)
continue;
-
- if (!req->request)
+ else if (!req->request)
continue;
/*
@@ -2328,7 +2340,8 @@ hangup:
if (req->numserv && --(req->numserv))
continue;
- hdr = (void *) (req->request + 2);
+ struct domain_hdr *hdr = (void *) (req->request +
+ DNS_HEADER_TCP_EXTRA_BYTES);
hdr->id = req->srcid;
send_response(req->client_sk, req->request,
req->request_len, NULL, 0, IPPROTO_TCP);
@@ -2342,17 +2355,13 @@ hangup:
}
if ((condition & G_IO_OUT) && !server->connected) {
- GSList *list;
- GList *domains;
- bool no_request_sent = true;
- struct server_data *udp_server;
-
- udp_server = find_server(server->index, server->server,
- IPPROTO_UDP);
+ struct server_data *udp_server = find_server(
+ server->index, server->server,
+ IPPROTO_UDP);
if (udp_server) {
- for (domains = udp_server->domains; domains;
+ for (GList *domains = udp_server->domains; domains;
domains = domains->next) {
- char *dom = domains->data;
+ const char *dom = domains->data;
debug("Adding domain %s to %s",
dom, server->server);
@@ -2377,9 +2386,12 @@ hangup:
server->connected = true;
server_list = g_slist_append(server_list, server);
- for (list = request_list; list; ) {
+ bool no_request_sent = true;
+
+ /* don't advance the list in the for loop, because we might
+ * need to delete elements while iterating through it */
+ for (GSList *list = request_list; list; ) {
struct request_data *req = list->data;
- int status;
if (req->protocol == IPPROTO_UDP) {
list = list->next;
@@ -2388,7 +2400,7 @@ hangup:
debug("Sending req %s over TCP", (char *)req->name);
- status = ns_resolv(server, req,
+ const int status = ns_resolv(server, req,
req->request, req->name);
if (status > 0) {
/*
@@ -2399,9 +2411,7 @@ hangup:
request_list = g_slist_remove(request_list, req);
destroy_request_data(req);
continue;
- }
-
- if (status < 0) {
+ } else if (status < 0) {
list = list->next;
continue;
}
@@ -2426,10 +2436,9 @@ hangup:
int bytes_recv;
if (!reply) {
- unsigned char reply_len_buf[2];
uint16_t reply_len;
- bytes_recv = recv(sk, reply_len_buf, 2, MSG_PEEK);
+ bytes_recv = recv(sk, &reply_len, sizeof(reply_len), MSG_PEEK);
if (!bytes_recv) {
goto hangup;
} else if (bytes_recv < 0) {
@@ -2442,8 +2451,9 @@ hangup:
} else if (bytes_recv < 2)
return TRUE;
- reply_len = reply_len_buf[1] | reply_len_buf[0] << 8;
- reply_len += 2;
+ /* the header contains the length of the message
+ * excluding the two length bytes */
+ reply_len = ntohs(reply_len) + DNS_HEADER_TCP_EXTRA_BYTES;
debug("TCP reply %d bytes from %d", reply_len, sk);
@@ -2504,13 +2514,11 @@ static gboolean tcp_idle_timeout(gpointer user_data)
static int server_create_socket(struct server_data *data)
{
- int sk, err;
- char *interface;
-
debug("index %d server %s proto %d", data->index,
data->server, data->protocol);
- sk = socket(data->server_addr->sa_family,
+ int err;
+ const int sk = socket(data->server_addr->sa_family,
data->protocol == IPPROTO_TCP ? SOCK_STREAM : SOCK_DGRAM,
data->protocol);
if (sk < 0) {
@@ -2523,7 +2531,7 @@ static int server_create_socket(struct server_data *data)
debug("sk %d", sk);
- interface = connman_inet_ifname(data->index);
+ char *interface = connman_inet_ifname(data->index);
if (interface) {
if (setsockopt(sk, SOL_SOCKET, SO_BINDTODEVICE,
interface,
@@ -2583,9 +2591,7 @@ static int server_create_socket(struct server_data *data)
static void enable_fallback(bool enable)
{
- GSList *list;
-
- for (list = server_list; list; list = list->next) {
+ for (GSList *list = server_list; list; list = list->next) {
struct server_data *data = list->data;
if (data->index != -1)
@@ -2604,13 +2610,9 @@ static struct server_data *create_server(int index,
const char *domain, const char *server,
int protocol)
{
- struct server_data *data;
- struct addrinfo hints, *rp;
- int ret;
-
DBG("index %d server %s", index, server);
- data = g_try_new0(struct server_data, 1);
+ struct server_data *data = g_try_new0(struct server_data, 1);
if (!data) {
connman_error("Failed to allocate server %s data", server);
return NULL;
@@ -2622,25 +2624,14 @@ static struct server_data *create_server(int index,
data->server = g_strdup(server);
data->protocol = protocol;
+ struct addrinfo hints;
memset(&hints, 0, sizeof(hints));
-
- switch (protocol) {
- case IPPROTO_UDP:
- hints.ai_socktype = SOCK_DGRAM;
- break;
-
- case IPPROTO_TCP:
- hints.ai_socktype = SOCK_STREAM;
- break;
-
- default:
- destroy_server(data);
- return NULL;
- }
+ hints.ai_socktype = socket_type(protocol, 0);
hints.ai_family = AF_UNSPEC;
hints.ai_flags = AI_NUMERICSERV | AI_NUMERICHOST;
- ret = getaddrinfo(data->server, "53", &hints, &rp);
+ struct addrinfo *rp;
+ const int ret = getaddrinfo(data->server, "53", &hints, &rp);
if (ret) {
connman_error("Failed to parse server %s address: %s\n",
data->server, gai_strerror(ret));
@@ -2699,9 +2690,7 @@ static struct server_data *create_server(int index,
static bool resolv(struct request_data *req,
gpointer request, gpointer name)
{
- GSList *list;
-
- for (list = server_list; list; list = list->next) {
+ for (GSList *list = server_list; list; list = list->next) {
struct server_data *data = list->data;
if (data->protocol == IPPROTO_TCP) {
@@ -2730,26 +2719,22 @@ static bool resolv(struct request_data *req,
static void update_domain(int index, const char *domain, bool append)
{
- GSList *list;
-
DBG("index %d domain %s", index, domain);
if (!domain)
return;
- for (list = server_list; list; list = list->next) {
+ for (GSList *list = server_list; list; list = list->next) {
struct server_data *data = list->data;
- GList *dom_list;
- char *dom = NULL;
- bool dom_found = false;
if (data->index < 0)
continue;
-
- if (data->index != index)
+ else if (data->index != index)
continue;
- for (dom_list = data->domains; dom_list;
+ char *dom = NULL;
+ bool dom_found = false;
+ for (GList *dom_list = data->domains; dom_list;
dom_list = dom_list->next) {
dom = dom_list->data;
@@ -2782,9 +2767,7 @@ static void remove_domain(int index, const char *domain)
static void flush_requests(struct server_data *server)
{
- GSList *list;
-
- list = request_list;
+ GSList *list = request_list;
while (list) {
struct request_data *req = list->data;
@@ -2811,26 +2794,23 @@ static void flush_requests(struct server_data *server)
int __connman_dnsproxy_append(int index, const char *domain,
const char *server)
{
- struct server_data *data;
-
DBG("index %d server %s", index, server);
- if (!server && !domain)
- return -EINVAL;
-
if (!server) {
- append_domain(index, domain);
-
- return 0;
+ if (!domain) {
+ return -EINVAL;
+ } else {
+ append_domain(index, domain);
+ return 0;
+ }
}
if (g_str_equal(server, "127.0.0.1"))
return -ENODEV;
-
- if (g_str_equal(server, "::1"))
+ else if (g_str_equal(server, "::1"))
return -ENODEV;
- data = find_server(index, server, IPPROTO_UDP);
+ struct server_data *data = find_server(index, server, IPPROTO_UDP);
if (data) {
append_domain(index, domain);
return 0;
@@ -2847,16 +2827,13 @@ int __connman_dnsproxy_append(int index, const char *domain,
static void remove_server(int index, const char *server, int protocol)
{
- struct server_data *data;
- GSList *list;
-
- data = find_server(index, server, protocol);
+ struct server_data *data = find_server(index, server, protocol);
if (!data)
return;
destroy_server(data);
- for (list = server_list; list; list = list->next) {
+ for (GSList *list = server_list; list; list = list->next) {
data = list->data;
if (data->index != -1 && data->enabled == true)
@@ -2871,19 +2848,18 @@ int __connman_dnsproxy_remove(int index, const char *domain,
{
DBG("index %d server %s", index, server);
- if (!server && !domain)
- return -EINVAL;
-
if (!server) {
- remove_domain(index, domain);
-
- return 0;
+ if (!domain) {
+ return -EINVAL;
+ } else {
+ remove_domain(index, domain);
+ return 0;
+ }
}
if (g_str_equal(server, "127.0.0.1"))
return -ENODEV;
-
- if (g_str_equal(server, "::1"))
+ else if (g_str_equal(server, "::1"))
return -ENODEV;
remove_server(index, server, IPPROTO_UDP);
@@ -2894,11 +2870,9 @@ int __connman_dnsproxy_remove(int index, const char *domain,
static void dnsproxy_offline_mode(bool enabled)
{
- GSList *list;
-
DBG("enabled %d", enabled);
- for (list = server_list; list; list = list->next) {
+ for (GSList *list = server_list; list; list = list->next) {
struct server_data *data = list->data;
if (!enabled) {
@@ -2916,11 +2890,6 @@ static void dnsproxy_offline_mode(bool enabled)
static void dnsproxy_default_changed(struct connman_service *service)
{
- bool server_enabled = false;
- GSList *list;
- int index;
- int vpn_index;
-
DBG("service %p", service);
/* DNS has changed, invalidate the cache */
@@ -2932,7 +2901,7 @@ static void dnsproxy_default_changed(struct connman_service *service)
return;
}
- index = __connman_service_get_index(service);
+ const int index = __connman_service_get_index(service);
if (index < 0)
return;
@@ -2941,9 +2910,10 @@ static void dnsproxy_default_changed(struct connman_service *service)
* the VPN must be enabled as well, when the transport becomes the
* default service.
*/
- vpn_index = __connman_connection_get_vpn_index(index);
+ const int vpn_index = __connman_connection_get_vpn_index(index);
+ bool server_enabled = false;
- for (list = server_list; list; list = list->next) {
+ for (GSList *list = server_list; list; list = list->next) {
struct server_data *data = list->data;
if (data->index == index) {
@@ -2968,9 +2938,6 @@ static void dnsproxy_default_changed(struct connman_service *service)
static void dnsproxy_service_state_changed(struct connman_service *service,
enum connman_service_state state)
{
- GSList *list;
- int index;
-
switch (state) {
case CONNMAN_SERVICE_STATE_DISCONNECT:
case CONNMAN_SERVICE_STATE_IDLE:
@@ -2984,8 +2951,8 @@ static void dnsproxy_service_state_changed(struct connman_service *service,
return;
}
- index = __connman_service_get_index(service);
- list = server_list;
+ const int index = __connman_service_get_index(service);
+ GSList *list = server_list;
while (list) {
struct server_data *data = list->data;
@@ -3007,45 +2974,56 @@ static const struct connman_notifier dnsproxy_notifier = {
.service_state_changed = dnsproxy_service_state_changed,
};
-static const unsigned char opt_edns0_type[2] = { 0x00, 0x29 };
-
+/*
+ * Parses the given request buffer. `buf´ is expected to be the start of the
+ * domain_hdr structure i.e. the TCP length header is not handled by this
+ * function.
+ * Returns the ascii string dot representation of the query in `name´, which
+ * must be able to hold `size´ bytes.
+ *
+ * Returns < 0 on error (errno) or zero on success.
+ */
static int parse_request(unsigned char *buf, size_t len,
- char *name, unsigned int size)
+ char *name, size_t size)
{
+ static const unsigned char opt_edns0_type[2] = { 0x00, 0x29 };
struct domain_hdr *hdr = (void *) buf;
- uint16_t qdcount = ntohs(hdr->qdcount);
- uint16_t ancount = ntohs(hdr->ancount);
- uint16_t nscount = ntohs(hdr->nscount);
- uint16_t arcount = ntohs(hdr->arcount);
- unsigned char *ptr;
- unsigned int remain, used = 0;
-
- if (len < sizeof(*hdr) + sizeof(struct qtype_qclass) ||
- hdr->qr || qdcount != 1 || ancount || nscount) {
- DBG("Dropped DNS request qr %d with len %zd qdcount %d "
- "ancount %d nscount %d", hdr->qr, len, qdcount, ancount,
- nscount);
-
+ if (len < sizeof(*hdr) + sizeof(struct qtype_qclass)) {
+ DBG("Dropped DNS request with short length %zd", len);
return -EINVAL;
}
if (!name || !size)
return -EINVAL;
+ const uint16_t qdcount = ntohs(hdr->qdcount);
+ const uint16_t ancount = ntohs(hdr->ancount);
+ const uint16_t nscount = ntohs(hdr->nscount);
+ const uint16_t arcount = ntohs(hdr->arcount);
+
+ if (hdr->qr || qdcount != 1 || ancount || nscount) {
+ DBG("Dropped DNS request with bad flags/counts qr %d "
+ "with len %zd qdcount %d ancount %d nscount %d",
+ hdr->qr, len, qdcount, ancount, nscount);
+
+ return -EINVAL;
+ }
+
debug("id 0x%04x qr %d opcode %d qdcount %d arcount %d",
hdr->id, hdr->qr, hdr->opcode,
qdcount, arcount);
name[0] = '\0';
- ptr = buf + sizeof(struct domain_hdr);
- remain = len - sizeof(struct domain_hdr);
+ unsigned char *ptr = buf + sizeof(struct domain_hdr);
+ size_t remain = len - sizeof(struct domain_hdr);
+ size_t used = 0;
+ /* parse DNS query string into `name' out parameter */
while (remain > 0) {
uint8_t label_len = *ptr;
if (label_len == 0x00) {
- uint8_t class;
struct qtype_qclass *q =
(struct qtype_qclass *)(ptr + 1);
@@ -3054,8 +3032,8 @@ static int parse_request(unsigned char *buf, size_t len,
return -EINVAL;
}
- class = ntohs(q->qclass);
- if (class != 1 && class != 255) {
+ const uint16_t class = ntohs(q->qclass);
+ if (class != DNS_CLASS_IN && class != DNS_CLASS_ANY) {
DBG("Dropped non-IN DNS class %d", class);
return -EINVAL;
}
@@ -3072,7 +3050,6 @@ static int parse_request(unsigned char *buf, size_t len,
strcat(name, ".");
used += label_len + 1;
-
ptr += label_len + 1;
remain -= label_len + 1;
}
@@ -3083,7 +3060,7 @@ static int parse_request(unsigned char *buf, size_t len,
DBG("EDNS0 buffer size %u", ntohs(edns0->class));
} else if (!arcount && remain) {
- DBG("DNS request with %d garbage bytes", remain);
+ DBG("DNS request with %zd garbage bytes", remain);
}
debug("query %s", name);
@@ -3120,7 +3097,7 @@ static void client_reset(struct tcp_partial_client_data *client)
client->buf_end = 0;
}
-static unsigned int get_msg_len(unsigned char *buf)
+static size_t get_msg_len(const unsigned char *buf)
{
return buf[0]<<8 | buf[1];
}
@@ -3130,15 +3107,7 @@ static bool read_tcp_data(struct tcp_partial_client_data *client,
int read_len)
{
char query[TCP_MAX_BUF_LEN];
- struct request_data *req;
- int client_sk, err;
- unsigned int msg_len;
- GSList *list;
- bool waiting_for_connect = false;
- uint16_t qtype = 0;
- struct cache_entry *entry;
-
- client_sk = g_io_channel_unix_get_fd(client->channel);
+ int client_sk = g_io_channel_unix_get_fd(client->channel);
if (read_len == 0) {
debug("client %d closed, pending %d bytes",
@@ -3152,31 +3121,32 @@ static bool read_tcp_data(struct tcp_partial_client_data *client,
client->buf_end += read_len;
+ /* we need at least the message length header */
if (client->buf_end < 2)
return true;
- msg_len = get_msg_len(client->buf);
+ size_t msg_len = get_msg_len(client->buf);
if (msg_len > TCP_MAX_BUF_LEN) {
- debug("client %d sent too much data %d", client_sk, msg_len);
+ debug("client %d sent too much data %zd", client_sk, msg_len);
g_hash_table_remove(partial_tcp_req_table,
GINT_TO_POINTER(client_sk));
return false;
}
read_another:
- debug("client %d msg len %d end %d past end %d", client_sk, msg_len,
+ debug("client %d msg len %zd end %d past end %zd", client_sk, msg_len,
client->buf_end, client->buf_end - (msg_len + 2));
if (client->buf_end < (msg_len + 2)) {
- debug("client %d still missing %d bytes",
+ debug("client %d still missing %zd bytes",
client_sk,
msg_len + 2 - client->buf_end);
return true;
}
- debug("client %d all data %d received", client_sk, msg_len);
+ debug("client %d all data %zd received", client_sk, msg_len);
- err = parse_request(client->buf + 2, msg_len,
+ const int err = parse_request(client->buf + 2, msg_len,
query, sizeof(query));
if (err < 0 || (g_slist_length(server_list) == 0)) {
send_response(client_sk, client->buf, msg_len + 2,
@@ -3184,7 +3154,7 @@ read_another:
return true;
}
- req = g_try_new0(struct request_data, 1);
+ struct request_data *req = g_try_new0(struct request_data, 1);
if (!req)
return true;
@@ -3194,13 +3164,15 @@ read_another:
req->protocol = IPPROTO_TCP;
req->family = client->family;
- req->srcid = client->buf[2] | (client->buf[3] << 8);
+ struct domain_hdr *hdr = (void*)(client->buf + 2);
+
+ memcpy(&req->srcid, &hdr->id, sizeof(req->srcid));
req->dstid = get_id();
req->altid = get_id();
req->request_len = msg_len + 2;
- client->buf[2] = req->dstid & 0xff;
- client->buf[3] = req->dstid >> 8;
+ /* replace ID the request for forwarding */
+ memcpy(&hdr->id, &req->dstid, sizeof(hdr->id));
req->numserv = 0;
req->ifdata = client->ifdata;
@@ -3210,19 +3182,15 @@ read_another:
* Check if the answer is found in the cache before
* creating sockets to the server.
*/
- entry = cache_check(client->buf, &qtype, IPPROTO_TCP);
+ uint16_t qtype = 0;
+ struct cache_entry *entry = cache_check(client->buf, &qtype, IPPROTO_TCP);
if (entry) {
- int ttl_left = 0;
- struct cache_data *data;
-
- debug("cache hit %s type %s", query, qtype == 1 ? "A" : "AAAA");
- if (qtype == 1)
- data = entry->ipv4;
- else
- data = entry->ipv6;
+ debug("cache hit %s type %s", query, qtype == DNS_TYPE_A ? "A" : "AAAA");
+ struct cache_data *data = qtype == DNS_TYPE_A ?
+ entry->ipv4 : entry->ipv6;
if (data) {
- ttl_left = data->valid_until - time(NULL);
+ int ttl_left = data->valid_until - time(NULL);
entry->hits++;
send_cached_response(client_sk, data->data,
@@ -3235,7 +3203,9 @@ read_another:
debug("data missing, ignoring cache for this query");
}
- for (list = server_list; list; list = list->next) {
+ bool waiting_for_connect = false;
+
+ for (GSList *list = server_list; list; list = list->next) {
struct server_data *data = list->data;
if (data->protocol != IPPROTO_UDP || !data->enabled)
@@ -3287,7 +3257,7 @@ read_another:
out:
if (client->buf_end > (msg_len + 2)) {
- debug("client %d buf %p -> %p end %d len %d new %d",
+ debug("client %d buf %p -> %p end %d len %d new %zd",
client_sk,
client->buf + msg_len + 2,
client->buf, client->buf_end,
@@ -3303,7 +3273,7 @@ out:
*/
msg_len = get_msg_len(client->buf);
if ((msg_len + 2) == client->buf_end) {
- debug("client %d reading another %d bytes", client_sk,
+ debug("client %d reading another %zd bytes", client_sk,
msg_len + 2);
goto read_another;
}
@@ -3329,15 +3299,7 @@ static gboolean tcp_client_event(GIOChannel *channel, GIOCondition condition,
gpointer user_data)
{
struct tcp_partial_client_data *client = user_data;
- struct sockaddr_in6 client_addr6;
- socklen_t client_addr6_len = sizeof(client_addr6);
- struct sockaddr_in client_addr4;
- socklen_t client_addr4_len = sizeof(client_addr4);
- void *client_addr;
- socklen_t *client_addr_len;
- int len, client_sk;
-
- client_sk = g_io_channel_unix_get_fd(channel);
+ int client_sk = g_io_channel_unix_get_fd(channel);
if (condition & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
g_hash_table_remove(partial_tcp_req_table,
@@ -3347,6 +3309,13 @@ static gboolean tcp_client_event(GIOChannel *channel, GIOCondition condition,
return FALSE;
}
+ struct sockaddr_in6 client_addr6;
+ socklen_t client_addr6_len = sizeof(client_addr6);
+ struct sockaddr_in client_addr4;
+ socklen_t client_addr4_len = sizeof(client_addr4);
+ void *client_addr;
+ socklen_t *client_addr_len;
+
switch (client->family) {
case AF_INET:
client_addr = &client_addr4;
@@ -3363,7 +3332,7 @@ static gboolean tcp_client_event(GIOChannel *channel, GIOCondition condition,
return FALSE;
}
- len = recvfrom(client_sk, client->buf + client->buf_end,
+ const int len = recvfrom(client_sk, client->buf + client->buf_end,
TCP_MAX_BUF_LEN - client->buf_end, 0,
client_addr, client_addr_len);
if (len < 0) {
@@ -3383,9 +3352,7 @@ static gboolean tcp_client_event(GIOChannel *channel, GIOCondition condition,
static gboolean client_timeout(gpointer user_data)
{
struct tcp_partial_client_data *client = user_data;
- int sock;
-
- sock = g_io_channel_unix_get_fd(client->channel);
+ int sock = g_io_channel_unix_get_fd(client->channel);
debug("client %d timeout pending %d bytes", sock, client->buf_end);
@@ -3398,18 +3365,6 @@ static bool tcp_listener_event(GIOChannel *channel, GIOCondition condition,
struct listener_data *ifdata, int family,
guint *listener_watch)
{
- int sk, client_sk, len;
- unsigned int msg_len;
- struct tcp_partial_client_data *client;
- struct sockaddr_in6 client_addr6;
- socklen_t client_addr6_len = sizeof(client_addr6);
- struct sockaddr_in client_addr4;
- socklen_t client_addr4_len = sizeof(client_addr4);
- void *client_addr;
- socklen_t *client_addr_len;
- struct timeval tv;
- fd_set readfds;
-
debug("condition 0x%02x channel %p ifdata %p family %d",
condition, channel, ifdata, family);
@@ -3423,7 +3378,12 @@ static bool tcp_listener_event(GIOChannel *channel, GIOCondition condition,
return false;
}
- sk = g_io_channel_unix_get_fd(channel);
+ struct sockaddr_in6 client_addr6;
+ socklen_t client_addr6_len = sizeof(client_addr6);
+ struct sockaddr_in client_addr4;
+ socklen_t client_addr4_len = sizeof(client_addr4);
+ void *client_addr;
+ socklen_t *client_addr_len;
if (family == AF_INET) {
client_addr = &client_addr4;
@@ -3433,29 +3393,32 @@ static bool tcp_listener_event(GIOChannel *channel, GIOCondition condition,
client_addr_len = &client_addr6_len;
}
+ struct timeval tv;
tv.tv_sec = tv.tv_usec = 0;
+ int sk = g_io_channel_unix_get_fd(channel);
+ fd_set readfds;
FD_ZERO(&readfds);
FD_SET(sk, &readfds);
+ /* TODO: check select return code */
select(sk + 1, &readfds, NULL, NULL, &tv);
- if (FD_ISSET(sk, &readfds)) {
- client_sk = accept(sk, client_addr, client_addr_len);
- debug("client %d accepted", client_sk);
- } else {
+ if (!FD_ISSET(sk, &readfds)) {
debug("No data to read from master %d, waiting.", sk);
return true;
}
+ int client_sk = accept(sk, client_addr, client_addr_len);
if (client_sk < 0) {
connman_error("Accept failure on TCP listener");
*listener_watch = 0;
return false;
}
+ debug("client %d accepted", client_sk);
fcntl(client_sk, F_SETFL, O_NONBLOCK);
- client = g_hash_table_lookup(partial_tcp_req_table,
- GINT_TO_POINTER(client_sk));
+ struct tcp_partial_client_data *client = g_hash_table_lookup(
+ partial_tcp_req_table, GINT_TO_POINTER(client_sk));
if (!client) {
client = g_try_new0(struct tcp_partial_client_data, 1);
if (!client) {
@@ -3499,7 +3462,7 @@ static bool tcp_listener_event(GIOChannel *channel, GIOCondition condition,
* proceed normally, otherwise read the bits until everything
* is received or timeout occurs.
*/
- len = recv(client_sk, client->buf, TCP_MAX_BUF_LEN, 0);
+ const int len = recv(client_sk, client->buf, TCP_MAX_BUF_LEN, 0);
if (len < 0) {
if (errno == EAGAIN || errno == EWOULDBLOCK) {
debug("client %d no data to read, waiting", client_sk);
@@ -3519,9 +3482,9 @@ static bool tcp_listener_event(GIOChannel *channel, GIOCondition condition,
return true;
}
- msg_len = get_msg_len(client->buf);
+ const size_t msg_len = get_msg_len(client->buf);
if (msg_len > TCP_MAX_BUF_LEN) {
- debug("client %d invalid message length %u ignoring packet",
+ debug("client %d invalid message length %zd ignoring packet",
client_sk, msg_len);
g_hash_table_remove(partial_tcp_req_table,
GINT_TO_POINTER(client_sk));
@@ -3532,8 +3495,8 @@ static bool tcp_listener_event(GIOChannel *channel, GIOCondition condition,
* The packet length bytes do not contain the total message length,
* that is the reason to -2 below.
*/
- if (msg_len != (unsigned int)(len - 2)) {
- debug("client %d sent %d bytes but expecting %u pending %d",
+ if (msg_len != (size_t)(len - DNS_HEADER_TCP_EXTRA_BYTES)) {
+ debug("client %d sent %d bytes but expecting %zd pending %zd",
client_sk, len, msg_len + 2, msg_len + 2 - len);
client->buf_end += len;
@@ -3565,24 +3528,18 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition,
struct listener_data *ifdata, int family,
guint *listener_watch)
{
- unsigned char buf[768];
- char query[512];
- struct request_data *req;
- struct sockaddr_in6 client_addr6;
- socklen_t client_addr6_len = sizeof(client_addr6);
- struct sockaddr_in client_addr4;
- socklen_t client_addr4_len = sizeof(client_addr4);
- void *client_addr;
- socklen_t *client_addr_len;
- int sk, err, len;
-
if (condition & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
connman_error("Error with UDP listener channel");
*listener_watch = 0;
return false;
}
- sk = g_io_channel_unix_get_fd(channel);
+ struct sockaddr_in6 client_addr6;
+ socklen_t client_addr6_len = sizeof(client_addr6);
+ struct sockaddr_in client_addr4;
+ socklen_t client_addr4_len = sizeof(client_addr4);
+ void *client_addr;
+ socklen_t *client_addr_len;
if (family == AF_INET) {
client_addr = &client_addr4;
@@ -3593,20 +3550,23 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition,
}
memset(client_addr, 0, *client_addr_len);
- len = recvfrom(sk, buf, sizeof(buf), 0, client_addr, client_addr_len);
+ unsigned char buf[768];
+ int sk = g_io_channel_unix_get_fd(channel);
+ const int len = recvfrom(sk, buf, sizeof(buf), 0, client_addr, client_addr_len);
if (len < 2)
return true;
debug("Received %d bytes (id 0x%04x)", len, buf[0] | buf[1] << 8);
- err = parse_request(buf, len, query, sizeof(query));
+ char query[512];
+ const int err = parse_request(buf, len, query, sizeof(query));
if (err < 0 || (g_slist_length(server_list) == 0)) {
send_response(sk, buf, len, client_addr,
*client_addr_len, IPPROTO_UDP);
return true;
}
- req = g_try_new0(struct request_data, 1);
+ struct request_data *req = g_try_new0(struct request_data, 1);
if (!req)
return true;
@@ -3616,13 +3576,14 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition,
req->protocol = IPPROTO_UDP;
req->family = family;
- req->srcid = buf[0] | (buf[1] << 8);
+ struct domain_hdr *hdr = (void*)buf;
+
+ req->srcid = hdr->id;
req->dstid = get_id();
req->altid = get_id();
req->request_len = len;
- buf[0] = req->dstid & 0xff;
- buf[1] = req->dstid >> 8;
+ hdr->id = req->dstid;
req->numserv = 0;
req->ifdata = ifdata;
@@ -3663,46 +3624,22 @@ static gboolean udp6_listener_event(GIOChannel *channel, GIOCondition condition,
static GIOChannel *get_listener(int family, int protocol, int index)
{
- GIOChannel *channel;
- const char *proto;
- union {
- struct sockaddr sa;
- struct sockaddr_in6 sin6;
- struct sockaddr_in sin;
- } s;
- socklen_t slen;
- int sk, type;
- char *interface;
-
debug("family %d protocol %d index %d", family, protocol, index);
- switch (protocol) {
- case IPPROTO_UDP:
- proto = "UDP";
- type = SOCK_DGRAM | SOCK_CLOEXEC;
- break;
-
- case IPPROTO_TCP:
- proto = "TCP";
- type = SOCK_STREAM | SOCK_CLOEXEC;
- break;
-
- default:
- return NULL;
- }
-
- sk = socket(family, type, protocol);
- if (sk < 0 && family == AF_INET6 && errno == EAFNOSUPPORT) {
- connman_error("No IPv6 support");
- return NULL;
- }
+ const char *proto = protocol_label(protocol);
+ const int type = socket_type(protocol, SOCK_CLOEXEC);
+ int sk = socket(family, type, protocol);
if (sk < 0) {
- connman_error("Failed to create %s listener socket", proto);
+ if (family == AF_INET6 && errno == EAFNOSUPPORT) {
+ connman_error("No IPv6 support");
+ } else {
+ connman_error("Failed to create %s listener socket", proto);
+ }
return NULL;
}
- interface = connman_inet_ifname(index);
+ char *interface = connman_inet_ifname(index);
if (!interface || setsockopt(sk, SOL_SOCKET, SO_BINDTODEVICE,
interface,
strlen(interface) + 1) < 0) {
@@ -3716,6 +3653,13 @@ static GIOChannel *get_listener(int family, int protocol, int index)
}
g_free(interface);
+ union {
+ struct sockaddr sa;
+ struct sockaddr_in6 sin6;
+ struct sockaddr_in sin;
+ } s;
+ socklen_t slen;
+
if (family == AF_INET6) {
memset(&s.sin6, 0, sizeof(s.sin6));
s.sin6.sin6_family = AF_INET6;
@@ -3768,7 +3712,7 @@ static GIOChannel *get_listener(int family, int protocol, int index)
fcntl(sk, F_SETFL, O_NONBLOCK);
}
- channel = g_io_channel_unix_new(sk);
+ GIOChannel *channel = g_io_channel_unix_new(sk);
if (!channel) {
connman_error("Failed to create %s listener channel", proto);
close(sk);
@@ -3871,9 +3815,7 @@ static void destroy_tcp_listener(struct listener_data *ifdata)
static int create_listener(struct listener_data *ifdata)
{
- int err, index;
-
- err = create_dns_listener(IPPROTO_UDP, ifdata);
+ int err = create_dns_listener(IPPROTO_UDP, ifdata);
if ((err & UDP_FAILED) == UDP_FAILED)
return -EIO;
@@ -3883,7 +3825,7 @@ static int create_listener(struct listener_data *ifdata)
return -EIO;
}
- index = connman_inet_ifindex("lo");
+ int index = connman_inet_ifindex("lo");
if (ifdata->index == index) {
if ((err & IPv6_FAILED) != IPv6_FAILED)
__connman_resolvfile_append(index, NULL, "::1");
@@ -3897,16 +3839,14 @@ static int create_listener(struct listener_data *ifdata)
static void destroy_listener(struct listener_data *ifdata)
{
- int index;
- GSList *list;
- index = connman_inet_ifindex("lo");
+ int index = connman_inet_ifindex("lo");
if (ifdata->index == index) {
__connman_resolvfile_remove(index, NULL, "127.0.0.1");
__connman_resolvfile_remove(index, NULL, "::1");
}
- for (list = request_list; list; list = list->next) {
+ for (GSList *list = request_list; list; list = list->next) {
struct request_data *req = list->data;
debug("Dropping request (id 0x%04x -> 0x%04x)",
@@ -3924,9 +3864,6 @@ static void destroy_listener(struct listener_data *ifdata)
int __connman_dnsproxy_add_listener(int index)
{
- struct listener_data *ifdata;
- int err;
-
DBG("index %d", index);
if (index < 0)
@@ -3938,7 +3875,7 @@ int __connman_dnsproxy_add_listener(int index)
if (g_hash_table_lookup(listener_table, GINT_TO_POINTER(index)))
return 0;
- ifdata = g_try_new0(struct listener_data, 1);
+ struct listener_data *ifdata = g_try_new0(struct listener_data, 1);
if (!ifdata)
return -ENOMEM;
@@ -3952,7 +3889,7 @@ int __connman_dnsproxy_add_listener(int index)
ifdata->tcp6_listener_channel = NULL;
ifdata->tcp6_listener_watch = 0;
- err = create_listener(ifdata);
+ const int err = create_listener(ifdata);
if (err < 0) {
connman_error("Couldn't create listener for index %d err %d",
index, err);
@@ -3966,14 +3903,13 @@ int __connman_dnsproxy_add_listener(int index)
void __connman_dnsproxy_remove_listener(int index)
{
- struct listener_data *ifdata;
-
DBG("index %d", index);
if (!listener_table)
return;
- ifdata = g_hash_table_lookup(listener_table, GINT_TO_POINTER(index));
+ struct listener_data *ifdata = g_hash_table_lookup(
+ listener_table, GINT_TO_POINTER(index));
if (!ifdata)
return;
@@ -4002,8 +3938,6 @@ static void free_partial_reqs(gpointer value)
int __connman_dnsproxy_init(void)
{
- int err, index;
-
DBG("");
listener_table = g_hash_table_new_full(g_direct_hash, g_direct_equal,
@@ -4014,23 +3948,21 @@ int __connman_dnsproxy_init(void)
NULL,
free_partial_reqs);
- index = connman_inet_ifindex("lo");
- err = __connman_dnsproxy_add_listener(index);
+ int index = connman_inet_ifindex("lo");
+ int err = __connman_dnsproxy_add_listener(index);
if (err < 0)
return err;
err = connman_notifier_register(&dnsproxy_notifier);
- if (err < 0)
- goto destroy;
-
- return 0;
+ if (err < 0) {
+ __connman_dnsproxy_remove_listener(index);
+ g_hash_table_destroy(listener_table);
+ g_hash_table_destroy(partial_tcp_req_table);
-destroy:
- __connman_dnsproxy_remove_listener(index);
- g_hash_table_destroy(listener_table);
- g_hash_table_destroy(partial_tcp_req_table);
+ return err;
+ }
- return err;
+ return 0;
}
int __connman_dnsproxy_set_mdns(int index, bool enabled)
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 13/16] dnsproxy: fix TCP server reply handling if domain name is appended
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (11 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 12/16] dnsproxy: finish first pass of refactoring the compilation unit Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 14/16] dnsproxy: harmonize use of sizeof() for message size calculations Matthias Gerstner
` (2 subsequent siblings)
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
The code path for TCP if the domain name is attached never worked. There
is a bug in the `hdr` pointer calculation in `ns_resolv`. Furthermore if
the first response from the server is negative or erroneous then the TCP
connection is terminated unconditionally, even if further responses are
pending.
This change splits off the initial part of forward_dns_reply() into a
new lookup_request() function. The information from the request_data
structure is used by the UDP and TCP processing code to determine
whether to keep the request (and TCP connection) around or not.
Furthermore errors in the `alt` message creation are fixed.
---
src/dnsproxy.c | 98 ++++++++++++++++++++++++++++++++++----------------
1 file changed, 68 insertions(+), 30 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 758337ec2..1371cbe36 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1688,15 +1688,14 @@ static int ns_resolv(struct server_data *server, struct request_data *req,
const size_t offset = protocol_offset(server->protocol);
unsigned char alt[1024];
- /* TODO: is this a bug? the offset isn't considered here... */
- struct domain_hdr *hdr = (void *) &alt;
+ struct domain_hdr *hdr = (void *) (&alt[0] + offset);
memcpy(alt + offset, &req->altid, sizeof(req->altid));
- memcpy(alt + offset + 2, request + offset + 2, 10);
+ memcpy(alt + offset + 2, request + offset + 2, DNS_HEADER_SIZE - 2);
hdr->qdcount = htons(1);
- int altlen = append_query(alt + offset + DNS_HEADER_SIZE, sizeof(alt) - DNS_HEADER_SIZE,
+ int altlen = append_query(alt + offset + DNS_HEADER_SIZE, sizeof(alt) - DNS_HEADER_SIZE - offset,
name, domain);
if (altlen < 0)
return -EINVAL;
@@ -2118,29 +2117,39 @@ static int dns_reply_fixup_domains(
return reply_len;
}
-static int forward_dns_reply(char *reply, size_t reply_len, int protocol,
- struct server_data *data)
+static struct request_data* lookup_request(
+ const char *reply, size_t len, int protocol)
{
const size_t offset = protocol_offset(protocol);
struct domain_hdr *hdr = (void *)(reply + offset);
- debug("Received %zd bytes (id 0x%04x)", reply_len, hdr->id);
+ debug("Received %zd bytes (id 0x%04x)", len, hdr->id);
- if (reply_len < sizeof(struct domain_hdr) + offset)
- return -EINVAL;
+ if (len < sizeof(struct domain_hdr) + offset)
+ return NULL;
struct request_data *req = find_request(hdr->id);
+
if (!req)
- return -EINVAL;
+ return NULL;
debug("req %p dstid 0x%04x altid 0x%04x rcode %d",
req, req->dstid, req->altid, hdr->rcode);
+ req->numresp++;
+
+ return req;
+}
+
+static int forward_dns_reply(char *reply, size_t reply_len, int protocol,
+ struct server_data *data, struct request_data *req)
+{
+ const size_t offset = protocol_offset(protocol);
+ struct domain_hdr *hdr = (void *)(reply + offset);
+
/* replace with original request ID from our client */
hdr->id = req->srcid;
- req->numresp++;
-
if (hdr->rcode == ns_r_noerror || !req->resp) {
/*
* If the domain name was appended remove it before forwarding
@@ -2219,8 +2228,6 @@ static int forward_dns_reply(char *reply, size_t reply_len, int protocol,
else
debug("proto %d sent %d bytes to %d", protocol, err, sk);
- destroy_request_data(req);
-
return err;
}
@@ -2295,9 +2302,20 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
int sk = g_io_channel_unix_get_fd(channel);
ssize_t len = recv(sk, buf, sizeof(buf), 0);
- if (len > 0) {
- forward_dns_reply(buf, len, IPPROTO_UDP, data);
- }
+ if (len <= 0)
+ return TRUE;
+
+ struct request_data *req = lookup_request(buf, len, IPPROTO_UDP);
+
+ if (!req)
+ /* invalid / corrupt request */
+ return TRUE;
+
+ const int res = forward_dns_reply(buf, len, IPPROTO_UDP, data, req);
+
+ /* on success or no further responses are expected, destroy the req */
+ if (res == 0 || req->numresp >= req->numserv)
+ destroy_request_data(req);
return TRUE;
}
@@ -2448,7 +2466,7 @@ hangup:
connman_error("DNS proxy error %s",
strerror(errno));
goto hangup;
- } else if (bytes_recv < 2)
+ } else if (bytes_recv < sizeof(reply_len))
return TRUE;
/* the header contains the length of the message
@@ -2462,6 +2480,8 @@ hangup:
return TRUE;
reply->len = reply_len;
+ /* we only peeked the two length bytes, so we have to
+ receive the complete message below proper. */
reply->received = 0;
server->incoming_reply = reply;
@@ -2484,15 +2504,32 @@ hangup:
reply->received += bytes_recv;
}
- forward_dns_reply(reply->buf, reply->received, IPPROTO_TCP,
- server);
+ struct request_data *req = lookup_request(
+ reply->buf, reply->received, IPPROTO_TCP);
+
+ if (!req)
+ /* invalid / corrupt request */
+ return TRUE;
+
+ const int res = forward_dns_reply(
+ reply->buf, reply->received, IPPROTO_TCP, server, req);
g_free(reply);
server->incoming_reply = NULL;
- destroy_server(server);
+ /* on success or if no further responses are expected close
+ * connection */
+ if (res == 0 || req->numresp >= req->numserv) {
+ destroy_request_data(req);
+ destroy_server(server);
+ return FALSE;
+ }
- return FALSE;
+ /*
+ * keep the TCP connection open, there are more
+ * requests to be answered
+ */
+ return TRUE;
}
return TRUE;
@@ -3122,7 +3159,7 @@ static bool read_tcp_data(struct tcp_partial_client_data *client,
client->buf_end += read_len;
/* we need at least the message length header */
- if (client->buf_end < 2)
+ if (client->buf_end < DNS_HEADER_TCP_EXTRA_BYTES)
return true;
size_t msg_len = get_msg_len(client->buf);
@@ -3146,10 +3183,11 @@ read_another:
debug("client %d all data %zd received", client_sk, msg_len);
- const int err = parse_request(client->buf + 2, msg_len,
- query, sizeof(query));
+ const int err = parse_request(client->buf + DNS_HEADER_TCP_EXTRA_BYTES,
+ msg_len, query, sizeof(query));
if (err < 0 || (g_slist_length(server_list) == 0)) {
- send_response(client_sk, client->buf, msg_len + 2,
+ send_response(client_sk, client->buf,
+ msg_len + DNS_HEADER_TCP_EXTRA_BYTES,
NULL, 0, IPPROTO_TCP);
return true;
}
@@ -3164,12 +3202,12 @@ read_another:
req->protocol = IPPROTO_TCP;
req->family = client->family;
- struct domain_hdr *hdr = (void*)(client->buf + 2);
+ struct domain_hdr *hdr = (void*)(client->buf + DNS_HEADER_TCP_EXTRA_BYTES);
memcpy(&req->srcid, &hdr->id, sizeof(req->srcid));
req->dstid = get_id();
req->altid = get_id();
- req->request_len = msg_len + 2;
+ req->request_len = msg_len + DNS_HEADER_TCP_EXTRA_BYTES;
/* replace ID the request for forwarding */
memcpy(&hdr->id, &req->dstid, sizeof(hdr->id));
@@ -3256,7 +3294,7 @@ read_another:
request_list = g_slist_append(request_list, req);
out:
- if (client->buf_end > (msg_len + 2)) {
+ if (client->buf_end > (msg_len + DNS_HEADER_TCP_EXTRA_BYTES)) {
debug("client %d buf %p -> %p end %d len %d new %zd",
client_sk,
client->buf + msg_len + 2,
@@ -3476,7 +3514,7 @@ static bool tcp_listener_event(GIOChannel *channel, GIOCondition condition,
return true;
}
- if (len < 2) {
+ if (len < DNS_HEADER_TCP_EXTRA_BYTES) {
debug("client %d not enough data to read, waiting", client_sk);
client->buf_end += len;
return true;
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 14/16] dnsproxy: harmonize use of sizeof() for message size calculations
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (12 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 13/16] dnsproxy: fix TCP server reply handling if domain name is appended Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 15/16] dnsproxy: add my copyright statement covering the larger refactoring changes Matthias Gerstner
2022-06-10 12:33 ` [PATCH 16/16] dnsproxy: fix compiler warnings (differing signedness, empty format string) Matthias Gerstner
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
---
src/dnsproxy.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 1371cbe36..e3f768da8 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -229,6 +229,9 @@ struct domain_rr {
#define DNS_HEADER_SIZE sizeof(struct domain_hdr)
#define DNS_HEADER_TCP_EXTRA_BYTES 2
#define DNS_TCP_HEADER_SIZE DNS_HEADER_SIZE + DNS_HEADER_TCP_EXTRA_BYTES
+#define DNS_QUESTION_SIZE sizeof(struct domain_question)
+#define DNS_RR_SIZE sizeof(struct domain_rr)
+#define DNS_QTYPE_QCLASS_SIZE sizeof(struct qtype_qclass)
enum dns_type {
/* IPv4 address 32-bit */
@@ -429,14 +432,14 @@ static void update_cached_ttl(unsigned char *ptr, int len, int new_ttl)
ptr += DNS_HEADER_SIZE;
len -= DNS_HEADER_SIZE;
- if (len < sizeof(struct domain_question) + 1)
+ if (len < DNS_QUESTION_SIZE + 1)
return;
/* skip the query, which is a name and a struct domain_question */
size_t name_len = dns_name_length(ptr);
- ptr += name_len + sizeof(struct domain_question);
- len -= name_len + sizeof(struct domain_question);;
+ ptr += name_len + DNS_QUESTION_SIZE;
+ len -= name_len + DNS_QUESTION_SIZE;
const uint32_t raw_ttl = ntohl((uint32_t)new_ttl);
struct domain_rr *rr = NULL;
@@ -962,10 +965,10 @@ static int parse_rr(const unsigned char *buf, const unsigned char *start,
if (*ttl < 0)
return -EINVAL;
- memcpy(response + offset, *end, sizeof(struct domain_rr));
+ memcpy(response + offset, *end, DNS_RR_SIZE);
- offset += sizeof(struct domain_rr);
- *end += sizeof(struct domain_rr);
+ offset += DNS_RR_SIZE;
+ *end += DNS_RR_SIZE;
if ((offset + *rdlen) > *response_size)
return -ENOBUFS;
@@ -1035,7 +1038,7 @@ static int parse_response(const unsigned char *buf, size_t buflen,
qlen = strlen(question);
ptr += qlen + 1; /* skip \0 */
- if ((eptr - ptr) < sizeof(struct domain_question))
+ if ((eptr - ptr) < DNS_QUESTION_SIZE)
return -EINVAL;
const struct domain_question *q = (void *) ptr;
@@ -1045,7 +1048,7 @@ static int parse_response(const unsigned char *buf, size_t buflen,
if (qtype != DNS_TYPE_A && qtype != DNS_TYPE_AAAA)
return -ENOMSG;
- ptr += sizeof(struct domain_question); /* advance to answers section */
+ ptr += DNS_QUESTION_SIZE; /* advance to answers section */
int err = -ENOMSG;
const uint16_t ancount = ntohs(hdr->ancount);
@@ -1567,7 +1570,7 @@ static int cache_update(struct server_data *srv, const char *msg, size_t msg_len
struct domain_question *q = (void *)ptr;
q->type = htons(type);
q->class = htons(class);
- ptr += sizeof(struct domain_question);
+ ptr += DNS_QUESTION_SIZE;
memcpy(ptr, response, rsplen);
@@ -2125,7 +2128,7 @@ static struct request_data* lookup_request(
debug("Received %zd bytes (id 0x%04x)", len, hdr->id);
- if (len < sizeof(struct domain_hdr) + offset)
+ if (len < DNS_HEADER_SIZE + offset)
return NULL;
struct request_data *req = find_request(hdr->id);
@@ -3025,7 +3028,7 @@ static int parse_request(unsigned char *buf, size_t len,
{
static const unsigned char opt_edns0_type[2] = { 0x00, 0x29 };
struct domain_hdr *hdr = (void *) buf;
- if (len < sizeof(*hdr) + sizeof(struct qtype_qclass)) {
+ if (len < DNS_HEADER_SIZE + DNS_QTYPE_QCLASS_SIZE) {
DBG("Dropped DNS request with short length %zd", len);
return -EINVAL;
}
@@ -3052,8 +3055,8 @@ static int parse_request(unsigned char *buf, size_t len,
name[0] = '\0';
- unsigned char *ptr = buf + sizeof(struct domain_hdr);
- size_t remain = len - sizeof(struct domain_hdr);
+ unsigned char *ptr = buf + DNS_HEADER_SIZE;
+ size_t remain = len - DNS_HEADER_SIZE;
size_t used = 0;
/* parse DNS query string into `name' out parameter */
@@ -3091,7 +3094,7 @@ static int parse_request(unsigned char *buf, size_t len,
remain -= label_len + 1;
}
- if (arcount && remain >= sizeof(struct domain_rr) + 1 && !ptr[0] &&
+ if (arcount && remain >= DNS_RR_SIZE + 1 && !ptr[0] &&
ptr[1] == opt_edns0_type[0] && ptr[2] == opt_edns0_type[1]) {
struct domain_rr *edns0 = (struct domain_rr *)(ptr + 1);
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 15/16] dnsproxy: add my copyright statement covering the larger refactoring changes
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (13 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 14/16] dnsproxy: harmonize use of sizeof() for message size calculations Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
2022-06-10 12:33 ` [PATCH 16/16] dnsproxy: fix compiler warnings (differing signedness, empty format string) Matthias Gerstner
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
---
src/dnsproxy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index e3f768da8..1f873eeec 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -3,6 +3,7 @@
* Connection Manager
*
* Copyright (C) 2007-2014 Intel Corporation. All rights reserved.
+ * Copyright (C) 2022 Matthias Gerstner of SUSE. All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 16/16] dnsproxy: fix compiler warnings (differing signedness, empty format string)
2022-06-10 12:33 dnsproxy: first round of refactoring, TCP bugfix Matthias Gerstner
` (14 preceding siblings ...)
2022-06-10 12:33 ` [PATCH 15/16] dnsproxy: add my copyright statement covering the larger refactoring changes Matthias Gerstner
@ 2022-06-10 12:33 ` Matthias Gerstner
15 siblings, 0 replies; 19+ messages in thread
From: Matthias Gerstner @ 2022-06-10 12:33 UTC (permalink / raw)
To: connman
There are some invocations of functions with implicit casts from
signed/unsigned char or vice versa. This has been inconsisent in the
beginning and future refactoring should choose a harmonized data type
for message buffers, maybe something like `uint8_t*`.
---
src/dnsproxy.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 1f873eeec..f90428482 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1363,10 +1363,10 @@ static void cache_refresh(void)
g_hash_table_foreach(cache, cache_refresh_iterator, NULL);
}
-static int reply_query_type(const unsigned char *msg, int len)
+static int reply_query_type(const char *msg, size_t len)
{
/* skip the header */
- const unsigned char *c = msg + DNS_HEADER_SIZE;
+ const unsigned char *c = (const unsigned char*)msg + DNS_HEADER_SIZE;
len -= DNS_HEADER_SIZE;
if (len < 0)
@@ -1417,7 +1417,8 @@ static int cache_update(struct server_data *srv, const char *msg, size_t msg_len
question[sizeof(question) - 1] = '\0';
int ttl = 0;
uint16_t answers = 0, type = 0, class = 0;
- const int err = parse_response(msg + offset, msg_len - offset,
+ const int err = parse_response(
+ (unsigned char*)(msg + offset), msg_len - offset,
question, sizeof(question) - 1,
&type, &class, &ttl,
response, &rsplen, &answers);
@@ -2294,7 +2295,7 @@ static void destroy_server(struct server_data *server)
static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
gpointer user_data)
{
- unsigned char buf[4096];
+ char buf[4096];
struct server_data *data = user_data;
if (condition & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
@@ -2509,14 +2510,14 @@ hangup:
}
struct request_data *req = lookup_request(
- reply->buf, reply->received, IPPROTO_TCP);
+ (const char*)reply->buf, reply->received, IPPROTO_TCP);
if (!req)
/* invalid / corrupt request */
return TRUE;
const int res = forward_dns_reply(
- reply->buf, reply->received, IPPROTO_TCP, server, req);
+ (char*)reply->buf, reply->received, IPPROTO_TCP, server, req);
g_free(reply);
server->incoming_reply = NULL;
@@ -2543,7 +2544,7 @@ static gboolean tcp_idle_timeout(gpointer user_data)
{
struct server_data *server = user_data;
- debug("");
+ debug("\n");
if (!server)
return FALSE;
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread