dnsproxy: first round of refactoring, TCP bugfix

dnsproxy: first round of refactoring, TCP bugfix

[Matthias Gerstner]

This is a first iteration of refactoring the dnsproxy codebase. It is no major
change of the code structure yet, more a local refactoring of each function
trying to make the existing code more readable and less redundant.

Part of this is also improved testing capability, minor fixes of bugs
encountered during refactoring and a bugfix of the TCP protocol operation if
domain name appending is in effect.

[PATCH 01/12] dnsproxy-simple-test: improve test coverage and test flexibility

[Matthias Gerstner]

- 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..fd14c8e0a 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 { } while (0)
+#else
+#	define debug(fmt...) do { fprintf(stderr, fmt); fprintf(stderr, "\n"); } 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

[PATCH 02/12] dnsproxy: first bits of refactoring data types, global variables, simpler functions

[Matthias Gerstner]

---
 src/dnsproxy.c | 474 +++++++++++++++++++++++--------------------------
 1 file changed, 223 insertions(+), 251 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index fd14c8e0a..59b1c2251 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,32 @@ 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) {
+		g_free(entry->ipv4->data);
+		g_free(entry->ipv4);
+		entry->ipv4 = NULL;
+	}
+}
+
+static void cache_free_ipv6(struct cache_entry *entry) {
+	if (entry->ipv6) {
+		g_free(entry->ipv6->data);
+		g_free(entry->ipv6);
+		entry->ipv6 = NULL;
+	}
+}
+
 /*
  * remove stale cached entries so that they can be refreshed
  */
@@ -680,76 +709,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) {
+	default:
+		return false;
+	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;
+	}
 
-	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 +776,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 +796,7 @@ static gboolean try_remove_cache(gpointer user_data)
 
 		g_hash_table_destroy(cache);
 		cache = NULL;
+		cache_size = 0;
 	}
 
 	return FALSE;
@@ -792,32 +804,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 +832,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 +851,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 +879,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 +910,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 +949,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 +981,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 +1040,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 +1100,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 +1172,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 +1218,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 +1242,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 +1272,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 +1297,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 +1338,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 +1583,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 +1700,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 +1779,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 +1790,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 +1802,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 +1814,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 +1871,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 +1894,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 +1960,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 +1995,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 +2230,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 +2238,12 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
 		return FALSE;
 	}
 
-	sk = g_io_channel_unix_get_fd(channel);
+	int sk = g_io_channel_unix_get_fd(channel);
+	ssize_t len = recv(sk, buf, sizeof(buf), 0);
 
-	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 +3097,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

Re: [PATCH 02/12] dnsproxy: first bits of refactoring data types, global variables, simpler functions

[Daniel Wagner]

> +	default:
> +		return false;
> +	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;

Please move the default to last position in the switch.

Re: [PATCH 02/12] dnsproxy: first bits of refactoring data types, global variables, simpler functions

[Daniel Wagner]

> +static void cache_free_ipv4(struct cache_entry *entry) {
> +	if (entry->ipv4) {
> +		g_free(entry->ipv4->data);
> +		g_free(entry->ipv4);
> +		entry->ipv4 = NULL;
> +	}
> +}
> +
> +static void cache_free_ipv6(struct cache_entry *entry) {
> +	if (entry->ipv6) {
> +		g_free(entry->ipv6->data);
> +		g_free(entry->ipv6);
> +		entry->ipv6 = NULL;
> +	}
> +}

Use early exist here and move the opening bracket to a new line

static void cache_free_ipv6(struct cache_entry *entry)
{
	if (!entry->ipv4)
        	return;

	g_free(...);
}

[PATCH 03/12] dnsproxy: refactoring of update_cached_ttl() and append_data()

[Matthias Gerstner]

---
 src/dnsproxy.c | 137 ++++++++++++++++++++++++-------------------------
 1 file changed, 66 insertions(+), 71 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 59b1c2251..1b464a5fb 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

Re: [PATCH 03/12] dnsproxy: refactoring of update_cached_ttl() and append_data()

[Daniel Wagner]

On Tue, Apr 19, 2022 at 12:34:52PM +0200, Matthias Gerstner wrote:
> +	ptr += DNS_HEADER_SIZE;
> +	len -= DNS_HEADER_SIZE;

here the  'sizeof(struct ...)' is hidden in the define

> +
> +	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);

and here we have the sizeof(struct ...). Personally I prefer the later
over the former because I don't have to lookup what the define is
doing. But then I do understand that it a good name of a define helps to
read. So just pick one version of it.

Re: [PATCH 03/12] dnsproxy: refactoring of update_cached_ttl() and append_data()

[Matthias Gerstner]

[-- Attachment #1: Type: text/plain, Size: 1196 bytes --]

Hi Daniel,

On Wed, May 25, 2022 at 08:45:09AM +0200, Daniel Wagner wrote:
> On Tue, Apr 19, 2022 at 12:34:52PM +0200, Matthias Gerstner wrote:
> > +	ptr += DNS_HEADER_SIZE;
> > +	len -= DNS_HEADER_SIZE;
> 
> here the  'sizeof(struct ...)' is hidden in the define
> 
> > +
> > +	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);
> 
> and here we have the sizeof(struct ...). Personally I prefer the later
> over the former because I don't have to lookup what the define is
> doing. But then I do understand that it a good name of a define helps to
> read. So just pick one version of it.

I mostly wanted the define because of the frequently occuring and
complex `sizeof(domain_hdr)` calculations. For the sake of unity I will
add a few additional defines for the other sizeof() occurences.

Cheers

Matthias

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 04/12] dnsproxy: refactor parse_response()

[Matthias Gerstner]

---
 src/dnsproxy.c | 123 +++++++++++++++++++++++++------------------------
 1 file changed, 63 insertions(+), 60 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 1b464a5fb..92ea1615f 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -907,7 +907,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)
@@ -968,55 +968,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
@@ -1024,7 +1036,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
@@ -1032,24 +1044,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
@@ -1061,8 +1076,6 @@ static int parse_response(unsigned char *buf, int buflen,
 		 * looking for.
 		 */
 		if (*class != qclass) {
-			ptr = next;
-			next = NULL;
 			continue;
 		}
 
@@ -1087,7 +1100,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
@@ -1111,8 +1124,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;
 			}
 
@@ -1124,12 +1135,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)
 			 */
@@ -1146,7 +1153,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;
@@ -1154,13 +1161,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);
 
@@ -1363,7 +1366,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

[PATCH 05/12] dnsproxy: further refactoring of cache_update()

[Matthias Gerstner]

---
 src/dnsproxy.c | 150 +++++++++++++++++++++++--------------------------
 1 file changed, 70 insertions(+), 80 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 92ea1615f..621a857d0 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 */
@@ -1353,30 +1354,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) {
@@ -1384,6 +1373,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) */
@@ -1393,10 +1385,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);
@@ -1408,26 +1403,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);
@@ -1436,9 +1434,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;
 		}
 	}
@@ -1446,8 +1442,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
@@ -1455,7 +1449,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)
@@ -1472,37 +1470,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;
 
@@ -1510,14 +1499,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
@@ -1528,36 +1524,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

Re: [PATCH 05/12] dnsproxy: further refactoring of cache_update()

[Daniel Wagner]

Generally a few words what the aim of the refactoring wouldn't hurt (also in
the previous patches).

[PATCH 06/12] dnsproxy: strip_domains(): fix out of bounds read access

[Matthias Gerstner]

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 621a857d0..9cb92627a 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1877,6 +1877,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

[PATCH 07/12] dnsproxy: refactor and document strip_domains() to make it less confusing

[Matthias Gerstner]

---
 src/dnsproxy.c | 60 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 18 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 9cb92627a..fcdf56c04 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1857,44 +1857,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

[PATCH 08/12] dnsproxy: refactor larger functions ns_resolv() and forwards_dns_reply()

[Matthias Gerstner]

---
 src/dnsproxy.c | 304 +++++++++++++++++++++++++------------------------
 1 file changed, 153 insertions(+), 151 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index fcdf56c04..4b834d2f0 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1567,39 +1567,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)
@@ -1613,7 +1614,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);
@@ -1628,51 +1646,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,
@@ -1921,94 +1938,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",
@@ -2030,17 +2036,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 */
@@ -2049,20 +2055,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
@@ -2071,41 +2072,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;
 				}
@@ -2114,9 +2110,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)
@@ -2124,7 +2124,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;
 			}
@@ -2157,6 +2157,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) {
@@ -2166,7 +2168,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

[PATCH 09/12] dnsproxy: uncompress: replace unnecessary goto with return statements

[Matthias Gerstner]

---
 src/dnsproxy.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 4b834d2f0..78855e27d 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1752,7 +1752,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
@@ -1761,7 +1761,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,
@@ -1777,7 +1777,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);
 
@@ -1785,7 +1785,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;
@@ -1799,7 +1799,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;
@@ -1812,7 +1812,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);
@@ -1827,7 +1827,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];
@@ -1838,7 +1838,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;
@@ -1851,7 +1851,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;
@@ -1869,9 +1869,6 @@ static const char* uncompress(int16_t field_count, const char *start, const char
 	}
 
 	return ptr;
-
-out:
-	return NULL;
 }
 
 /*
-- 
2.35.1

[PATCH 10/12] dnsproxy: forward_dns_reply: pull out separate dns_reply_fixup_domains()

[Matthias Gerstner]

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 78855e27d..7a3cbe9d7 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -47,6 +47,8 @@
 #	define debug(fmt...) do { fprintf(stderr, fmt); fprintf(stderr, "\n"); } while (0)
 #endif
 
+#define NUM_ARRAY_ELEMENTS(a) sizeof(a) / sizeof(a[0])
+
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 struct domain_hdr {
 	uint16_t id;
@@ -1935,6 +1937,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)
 {
@@ -1959,8 +2122,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
@@ -1971,163 +2132,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;
 
@@ -2143,7 +2165,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

[PATCH 11/12] dnsproxy: finish first pass of refactoring the compilation unit

[Matthias Gerstner]

- 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 7a3cbe9d7..04c88ef3c 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);
 }
 
@@ -2285,10 +2301,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;
 
@@ -2307,13 +2321,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;
 
 			/*
@@ -2324,7 +2336,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);
@@ -2338,17 +2351,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);
@@ -2373,9 +2382,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;
@@ -2384,7 +2396,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) {
 				/*
@@ -2395,9 +2407,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;
 			}
@@ -2422,10 +2432,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) {
@@ -2438,8 +2447,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);
 
@@ -2500,13 +2510,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) {
@@ -2519,7 +2527,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,
@@ -2579,9 +2587,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)
@@ -2600,13 +2606,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;
@@ -2618,25 +2620,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));
@@ -2695,9 +2686,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) {
@@ -2726,26 +2715,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;
 
@@ -2778,9 +2763,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;
 
@@ -2807,26 +2790,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;
@@ -2843,16 +2823,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)
@@ -2867,19 +2844,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);
@@ -2890,11 +2866,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) {
@@ -2912,11 +2886,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 */
@@ -2928,7 +2897,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;
 
@@ -2937,9 +2906,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) {
@@ -2964,9 +2934,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:
@@ -2980,8 +2947,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;
@@ -3003,45 +2970,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);
 
@@ -3050,8 +3028,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;
 			}
@@ -3068,7 +3046,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;
 	}
@@ -3079,7 +3056,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);
@@ -3116,7 +3093,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];
 }
@@ -3126,15 +3103,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",
@@ -3148,31 +3117,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,
@@ -3180,7 +3150,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;
 
@@ -3190,13 +3160,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;
@@ -3206,19 +3178,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,
@@ -3231,7 +3199,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)
@@ -3283,7 +3253,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,
@@ -3299,7 +3269,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;
 		}
@@ -3325,15 +3295,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,
@@ -3343,6 +3305,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;
@@ -3359,7 +3328,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) {
@@ -3379,9 +3348,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);
 
@@ -3394,18 +3361,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);
 
@@ -3419,7 +3374,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;
@@ -3429,29 +3389,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) {
@@ -3495,7 +3458,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);
@@ -3515,9 +3478,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));
@@ -3528,8 +3491,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;
@@ -3561,24 +3524,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;
@@ -3589,20 +3546,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;
 
@@ -3612,13 +3572,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;
@@ -3659,46 +3620,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) {
@@ -3712,6 +3649,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;
@@ -3764,7 +3708,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);
@@ -3867,9 +3811,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;
 
@@ -3879,7 +3821,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");
@@ -3893,16 +3835,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)",
@@ -3920,9 +3860,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)
@@ -3934,7 +3871,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;
 
@@ -3948,7 +3885,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);
@@ -3962,14 +3899,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;
 
@@ -3998,8 +3934,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,
@@ -4010,23 +3944,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

[PATCH 12/12] dnsproxy: fix TCP server reply handling if domain name is appended

[Matthias Gerstner]

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 04c88ef3c..73c6cbd1b 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1684,15 +1684,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;
@@ -2114,29 +2113,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
@@ -2215,8 +2224,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;
 }
 
@@ -2291,9 +2298,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;
 }
@@ -2444,7 +2462,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
@@ -2458,6 +2476,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;
@@ -2480,15 +2500,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;
@@ -3118,7 +3155,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);
@@ -3142,10 +3179,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;
 	}
@@ -3160,12 +3198,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));
@@ -3252,7 +3290,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,
@@ -3472,7 +3510,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

Re: dnsproxy: first round of refactoring, TCP bugfix

[Daniel Wagner]

Hi Matthias,

On Tue, Apr 19, 2022 at 12:34:49PM +0200, Matthias Gerstner wrote:
> This is a first iteration of refactoring the dnsproxy codebase. It is no major
> change of the code structure yet, more a local refactoring of each function
> trying to make the existing code more readable and less redundant.
> 
> Part of this is also improved testing capability, minor fixes of bugs
> encountered during refactoring and a bugfix of the TCP protocol operation if
> domain name appending is in effect.

Nice work. A few nits from my side. The cleanup generally looks good.

Though we need set the C standard to C99 I suppose:


src/dnsproxy.c: In function ‘refresh_dns_entry’:
src/dnsproxy.c:394:9: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
  394 |         int age = 1;
      |         ^~~

and my compiler complained about a few things:

src/dnsproxy.c:410:25: error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
  410 |         if (entry->hits > age)
      |                         ^
src/dnsproxy.c: In function ‘update_cached_ttl’:
src/dnsproxy.c:432:17: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
  432 |         if (len < sizeof(struct domain_question) + 1)
      |                 ^
[...]

BTW, since this contribution is major I would suggest you add your
copyright statement to the file as well.

Thanks,
Daniel

Re: dnsproxy: first round of refactoring, TCP bugfix

[Matthias Gerstner]

[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

Hi Daniel,

On Wed, May 25, 2022 at 09:01:46AM +0200, Daniel Wagner wrote:
> Though we need set the C standard to C99 I suppose:
> 
> 
> src/dnsproxy.c: In function ‘refresh_dns_entry’:
> src/dnsproxy.c:394:9: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
>   394 |         int age = 1;
>       |         ^~~

I will switch to AC_PROG_CC_C99 in autoconf, hopefully that is enough.

> and my compiler complained about a few things:
> 
> src/dnsproxy.c:410:25: error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
>   410 |         if (entry->hits > age)

I used a slightly older GCC for the patches, the next round will have
them fixed, hopefully.

> BTW, since this contribution is major I would suggest you add your
> copyright statement to the file as well.

Will do, although it feels a bit weird to me ;-)

Cheers

Matthias

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]