dnsproxy: first round of refactoring, TCP bugfix

dnsproxy: first round of refactoring, TCP bugfix

[Matthias Gerstner]

Sorry about the messed up previous set of patches.

So this round features the following changes:

- every commit should now successfully compile and also compile without
  warnings
- as requested I let go off most of the mixed code/declarations at the loss of
  some const variables that now need to be declared non-const at the start
  of the functions. In a few spots I left a little mixed code / declarations
  just at the beginning of the function block to allow for early exits when
  e.g. insufficient input data is supplied. Declarations at the beginning of
  for/while/if blocks still remain where suitable.

[PATCH 01/16] 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..458694e60 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -41,7 +41,11 @@
 
 #include "connman.h"
 
-#define debug(fmt...) do { } while (0)
+#ifdef DNSPROXY_DEBUG
+#	define debug(fmt...) do { fprintf(stderr, fmt); fprintf(stderr, "\n"); } while (0)
+#else
+#	define debug(fmt...) do { } while (0)
+#endif
 
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 struct domain_hdr {
diff --git a/tools/dnsproxy-simple-test b/tools/dnsproxy-simple-test
index b8fd7f77d..5c2f72925 100755
--- a/tools/dnsproxy-simple-test
+++ b/tools/dnsproxy-simple-test
@@ -6,9 +6,25 @@
 
 echoerr() {
 	echo $@ 1>&2
+	echo -e "\n   >>> ERROR OCCURED <<<   \n" 1>&2
 	exit 1
 }
 
+
+showlog() {
+	if [ -z "$SHOW_LOG" -o -z "$logfile" ]; then
+		return
+	fi
+
+	echo
+	echo "======== debug log ==========="
+	cat "$logfile"
+	echo "===== end debug log =========="
+	echo
+}
+
+TRANSPORTS="-U -T"
+
 while [ $# -gt 0 ]; do
 	case "$1" in
 	"--valgrind")
@@ -16,10 +32,39 @@ while [ $# -gt 0 ]; do
 		if [ -z "$VALGRIND" ]; then
 			echoerr "no valgrind executable found"
 		fi
+		# log valgrind output to stdout, since stderr is used for
+		# debug output from dnsproxy.c already and we want to parse
+		# that.
+		# also cause an error exit it valgrind error occur so that
+		# they're easily noticed.
+		VALGRIND="$VALGRIND --log-fd=1 --error-exitcode=10"
+		;;
+	"--gdb")
+		WAIT_GDB=1
+		# wait forever to avoid timeout conditions during debugging
+		HOST_OPTS="-w"
+		;;
+	"--show-log")
+		SHOW_LOG=1
+		;;
+	"--testdomain="*)
+		TESTDOMAIN=`echo $1 | cut -d '=' -f2-`
+		CUSTOM_TESTDOMAIN=1
+		;;
+	"--only-tcp")
+		TRANSPORTS="-T"
+		;;
+	"--only-udp")
+		TRANSPORTS="-U"
 		;;
 	"-h")
-		echo "$0 [--valgrind]"
+		echo "$0 [--valgrind] [--gdb] [--show-log] [--only-tcp] [--only-udp] [--testdomain=<mydomain>]"
 		echo "--valgrind: run dnsproxy-standalone in valgrind"
+		echo "--gdb: allows you to attach via GDB before tests are started"
+		echo "--show-log: dump debug log from dnsproxy at end of test"
+		echo "--only-tcp: only perform TCP protocol based tests"
+		echo "--only-udp: only perform UDP protocol based tests"
+		echo "--testdomain=<mydomain>: the domain name to resolve"
 		exit 2
 		;;
 	*)
@@ -29,6 +74,11 @@ while [ $# -gt 0 ]; do
 	shift
 done
 
+if [ -n "$VALGRIND" -a -n "$WAIT_GDB" ]; then
+	echo "Cannot mix valgrind frontend and GDB attachment" 1>&2
+	exit 2
+fi
+
 if [ -e "Makefile" ]; then
 	BUILDROOT="$PWD"
 else
@@ -66,7 +116,8 @@ fi
 PORT=8053
 
 # run the proxy in the background
-$VALGRIND $DNSPROXY $PORT "$DOMAIN1" "$NS1" &
+logfile=`mktemp`
+$VALGRIND $DNSPROXY $PORT "$DOMAIN1" "$NS1" 2>"$logfile" &
 proxy_pid=$!
 
 cleanup() {
@@ -77,6 +128,13 @@ cleanup() {
 	wait $proxy_pid
 	ret=$?
 	proxy_pid=-1
+	if [ -n "$logfile" ]; then
+		if [ -n "$SHOW_LOG" ]; then
+			showlog
+		fi
+		rm -f "$logfile"
+		unset logfile
+	fi
 	return $ret
 }
 
@@ -85,24 +143,49 @@ trap cleanup err exit
 sleep 1
 echo -e "\n\n"
 
-# test both UDP and TCP mode
-for TRANSPORT in -U -T; do
-	# test both IPv4 and IPv6
-	for IP in -4 -6; do
-		echo "Testing resolution using transport $TRANSPORT and IP${IP}"
-		set -x
-		$HOST $TRANSPORT $IP -p$PORT www.example.com 127.0.0.1
-		RES=$?
-		set +x
-		if [ $RES -ne 0 ]; then
-			echoerr "resolution failed"
-		fi
+if [ -n "$WAIT_GDB" ]; then
+	echo "You can now attach to the dnsproxy process at PID $proxy_pid."
+	echo "Press ENTER to continue test execution"
+	read _
+fi
+
+if [ -z "$TESTDOMAIN" ]; then
+	TESTDOMAIN="www.example.com"
+fi
 
-		echo -e "\n\n"
+# perform each test twice to actually get cached responses served for each
+# combination
+for I in `seq 2`; do
+	# test both UDP and TCP mode
+	for TRANSPORT in $TRANSPORTS; do
+		# test both IPv4 and IPv6
+		for IP in -4 -6; do
+			echo "Testing resolution using transport $TRANSPORT and IP${IP}"
+			set -x
+			$HOST $HOST_OPTS $TRANSPORT $IP -p$PORT $TESTDOMAIN 127.0.0.1
+			RES=$?
+			set +x
+			if [ $RES -ne 0 ]; then
+				echoerr "resolution failed"
+			fi
+
+			echo -e "\n\n"
+		done
 	done
 done
 
+NUM_HITS=`grep "cache hit.*$TESTDOMAIN" "$logfile" | wc -l`
+
 echo -e "\n\nDNS resolution succeeded for all test combinations"
+echo -e "\nNumber of cache hits: $NUM_HITS\n"
+# assert we have seen the expected number of cache hits in the log
+# this is the amount of cache hits for the default domain tests as seen before
+# refactoring of dnsproxy started.
+if [ -z "$CUSTOM_TESTDOMAIN" -a "$NUM_HITS" -ne 15 ]; then
+	echoerr "Unexpected number of cache hits encountered"
+elif [ "$NUM_HITS" -lt 8 ]; then
+	echoerr "Too low number of cache hits encountered"
+fi
 cleanup
 if [ $? -eq 0 ]; then
 	exit 0
-- 
2.37.3

[PATCH 02/16] autoconf: require C99 compiler and set C99 mode

[Matthias Gerstner]

For refactoring the dnsproxy codebase using C99 language features will
come in handy (mostly for using more localized variable declarations).
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index a573cefd7..ea7a2facd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -22,7 +22,7 @@ AC_SUBST(abs_top_builddir)
 AC_LANG_C
 AC_USE_SYSTEM_EXTENSIONS
 
-AC_PROG_CC
+AC_PROG_CC_C99
 AM_PROG_CC_C_O
 AC_PROG_CC_PIE
 AC_PROG_INSTALL
-- 
2.37.3

[PATCH 03/16] dnsproxy: fix compiler warning about zero length printf format string

[Matthias Gerstner]

---
 src/dnsproxy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 458694e60..cbe03038f 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -2482,7 +2482,7 @@ static gboolean tcp_idle_timeout(gpointer user_data)
 {
 	struct server_data *server = user_data;
 
-	debug("");
+	debug("\n");
 
 	if (!server)
 		return FALSE;
-- 
2.37.3

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

[Matthias Gerstner]

- move all type declarations to the top of the unit to have them all in
  one place, same for global variables
- introduce enums for having more descriptive identifiers for some of
  the DNS header constants
- remove unnecessary zero initializations for global variables
- move variable declarations into more local scopes where possible (e.g.
  in for loops). Shorter lifetimes of variables can make the code more
  easy to follow.
- avoid some repetitive code sequences like `cache_free_ipv4()` by
  moving them into separate functions
- use const variables in parameters where possible to make certain
  guarantees of function calls more clear and avoid erroneous
  assignments.
---
 src/dnsproxy.c | 441 ++++++++++++++++++++++++-------------------------
 1 file changed, 214 insertions(+), 227 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index cbe03038f..02fb3c78c 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)
 {
@@ -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,12 @@ 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;
+	struct domain_hdr *hdr = NULL;
+	int offset, dns_len, err;
 
 	/*
 	 * The cached packet contains always the TCP offset (two bytes)
@@ -461,8 +481,10 @@ 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);
@@ -471,11 +493,8 @@ static void send_cached_response(int sk, unsigned char *buf, int len,
 	if (err < 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 (err != len || dns_len != (len - offset))
 		debug("Packet length mismatch, sent %d wanted %d dns %d",
 			err, len, dns_len);
 }
@@ -486,17 +505,18 @@ static void send_response(int sk, unsigned char *buf, size_t len,
 {
 	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);
 	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 +529,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);
+	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;
 	}
 }
 
@@ -547,7 +566,7 @@ static gboolean request_timeout(gpointer user_data)
 {
 	struct request_data *req = user_data;
 	struct sockaddr *sa;
-	int sk;
+	int sk = -1;
 
 	if (!req)
 		return FALSE;
@@ -562,7 +581,9 @@ static gboolean request_timeout(gpointer user_data)
 	} 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 +592,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 +678,36 @@ static int append_query(unsigned char *buf, unsigned int size,
 	return ptr - buf;
 }
 
-static bool cache_check_is_valid(struct cache_data *data,
-				time_t current_time)
+static bool cache_check_is_valid(struct cache_data *data, time_t current_time)
 {
 	if (!data)
 		return false;
-
-	if (data->cache_until < current_time)
+	else if (data->cache_until < current_time)
 		return false;
 
 	return true;
 }
 
+static void cache_free_ipv4(struct cache_entry *entry)
+{
+	if (!entry->ipv4)
+		return;
+
+	g_free(entry->ipv4->data);
+	g_free(entry->ipv4);
+	entry->ipv4 = NULL;
+}
+
+static void cache_free_ipv6(struct cache_entry *entry)
+{
+	if (!entry->ipv6)
+		return;
+
+	g_free(entry->ipv6->data);
+	g_free(entry->ipv6);
+	entry->ipv6 = NULL;
+}
+
 /*
  * remove stale cached entries so that they can be refreshed
  */
@@ -680,76 +715,65 @@ 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;
+	struct cache_data *cached_ip = NULL, *other_ip = NULL;
+	const time_t current_time = time(NULL);
+	bool want_refresh;
 
 	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);
-
-			if (want_refresh)
-				entry->want_refresh = true;
+	case DNS_TYPE_A: /* IPv4 */
+		cached_ip = entry->ipv4;
+		other_ip = entry->ipv6;
+		break;
 
-			/*
-			 * We do not remove cache entry if there is still
-			 * valid IPv6 entry found in the cache.
-			 */
-			if (!cache_check_is_valid(entry->ipv6, current_time) && !want_refresh) {
-				g_hash_table_remove(cache, question);
-				type = 0;
-			}
-		}
+	case DNS_TYPE_AAAA: /* IPv6 */
+		cached_ip = entry->ipv6;
+		other_ip = entry->ipv4;
 		break;
+	default:
+		return false;
+	}
 
-	case 28:	/* IPv6 */
-		if (!cache_check_is_valid(entry->ipv6, current_time)) {
-			debug("cache %s \"%s\" type AAAA", entry->ipv6 ?
-					"timeout" : "entry missing", question);
+	/*
+	 * if we have a popular entry, we want a refresh instead of
+	 * total destruction of the entry.
+	 */
+	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 +783,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 +803,7 @@ static gboolean try_remove_cache(gpointer user_data)
 
 		g_hash_table_destroy(cache);
 		cache = NULL;
+		cache_size = 0;
 	}
 
 	return FALSE;
@@ -792,32 +811,28 @@ 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);
+	struct cache_entry *entry;
 
 	/* 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) {
@@ -829,8 +844,7 @@ static struct cache_entry *cache_check(gpointer request, int *qtype, int proto)
 	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 +859,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;
+	const unsigned char *p = start;
 
 	/* Limit recursion to 10 (this means up to 10 labels in domain name) */
 	if (counter > 10)
 		return -EINVAL;
 
-	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 +887,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,25 +918,25 @@ 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;
+	size_t offset;
 	int name_len = 0, output_len = 0, max_rsp = *response_size;
+	int err = get_name(0, buf, start, max, response, max_rsp,
+		&output_len, end, name, max_name, &name_len);
 
-	err = get_name(0, buf, start, max, response, max_rsp,
-			&output_len, end, name, max_name, &name_len);
 	if (err < 0)
 		return err;
 
 	offset = output_len;
 
-	if ((unsigned int) offset > *response_size)
+	if (offset > *response_size)
 		return -ENOBUFS;
 
 	rr = (void *) (*end);
@@ -946,26 +957,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 +989,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 +1048,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 +1108,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 +1180,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 +1226,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 +1250,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 +1280,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 +1305,24 @@ 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];
+		char *c;
+
 		entry->want_refresh = false;
 
 		/* 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;
+		while (*c) {
+			/* fetch the size of the current component and replace
+			   it by a dot */
+			int jump = *c;
 			*c = '.';
 			c += jump + 1;
 		}
@@ -1358,22 +1348,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;
+	int type;
+	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;
+	/* now the query, which is a name and 2 16 bit words for type and class */
+	c += dns_name_length(c);
+
 	type = c[0] << 8 | c[1];
 
 	return type;
@@ -1607,7 +1594,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,17 +1711,17 @@ 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;
+	int 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;
 	}
 
 	/*
@@ -1743,20 +1731,17 @@ static char *convert_label(char *start, char *end, char *ptr, char *uptr,
 	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 +1791,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 +1802,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 +1814,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 +1826,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;
 
@@ -1899,16 +1884,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,
@@ -1988,8 +1973,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 +2008,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 +2243,8 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
 							gpointer user_data)
 {
 	unsigned char buf[4096];
-	int sk, len;
+	int sk;
+	ssize_t len;
 	struct server_data *data = user_data;
 
 	if (condition & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
@@ -2268,10 +2254,11 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
 	}
 
 	sk = g_io_channel_unix_get_fd(channel);
-
 	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 +3112,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.37.3

[PATCH 05/16] dnsproxy: refactoring of update_cached_ttl() and append_data()

[Matthias Gerstner]

- use size_t for sizes and lengths where possible
- use named constants in favor of literal numbers
- more localized variable declarations
- prefer byte order macros over explicit byte operations
- add some comments and use early exits to simplify the code
---
 src/dnsproxy.c | 143 +++++++++++++++++++++++++------------------------
 1 file changed, 72 insertions(+), 71 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 02fb3c78c..5ba68b52f 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -391,52 +391,52 @@ 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;
+	size_t name_len;
+	const uint32_t raw_ttl = ntohl((uint32_t)new_ttl);
+
+	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 */
+	name_len = dns_name_length(ptr);
+
+	ptr += name_len + sizeof(struct domain_question);
+	len -= name_len + sizeof(struct domain_question);;
 
 	/* now we get the answer records */
 
 	while (len > 0) {
+		struct domain_rr *rr = NULL;
+		size_t rr_len;
+
 		/* 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 */
+		rr_len = sizeof(*rr) + ntohs(rr->rdlen);
+		ptr += rr_len;
+		len -= rr_len;
 	}
 }
 
@@ -623,59 +623,60 @@ 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)
+{
+	size_t added;
+	size_t left_size = size;
+	int res;
 
-		*ptr = tmp - domain;
-		memcpy(ptr + 1, domain, tmp - domain);
-		ptr += tmp - domain + 1;
+	debug("query %s domain %s", query, domain);
 
-		domain = tmp + 1;
-	}
+	res = append_data(buf, left_size, query);
+	if (res < 0)
+		return -1;
+	left_size -= res;
+
+	res = append_data(buf + res, left_size, domain);
+	if (res < 0)
+		return -1;
+	left_size -= res;
+
+	if (left_size == 0)
+		return -1;
 
-	*ptr++ = 0x00;
+	added = size - left_size;
+	*(buf + added) = 0x00;
 
-	return ptr - buf;
+	return added;
 }
 
 static bool cache_check_is_valid(struct cache_data *data, time_t current_time)
-- 
2.37.3

[PATCH 06/16] dnsproxy: refactor parse_response()

[Matthias Gerstner]

- add a descriptive comment to make clear what the function does
- use const pointers and size_t where possible
- move stack variables into more localized scopes
- use named constants over literal numbers where applicable
---
 src/dnsproxy.c | 110 +++++++++++++++++++++++++------------------------
 1 file changed, 57 insertions(+), 53 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 5ba68b52f..796b33fc3 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -921,7 +921,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)
@@ -982,55 +982,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)
+	uint16_t qtype;
+	int err = -ENOMSG;
+	uint16_t ancount, qclass;
+	GSList *aliases = NULL;
+	const size_t maxlen = *response_len;
+
+	*response_len = 0;
+	*answers = 0;
+
+	if (buflen < DNS_HEADER_SIZE)
 		return -EINVAL;
 
+	const uint16_t qdcount = ntohs(hdr->qdcount);
+	const unsigned char *ptr = buf + DNS_HEADER_SIZE;
+	const unsigned char *eptr = buf + buflen;
+
 	debug("qr %d qdcount %d", hdr->qr, qdcount);
 
 	/* We currently only cache responses where question count is 1 */
 	if (hdr->qr != 1 || qdcount != 1)
 		return -EINVAL;
 
-	ptr = buf + sizeof(struct domain_hdr);
-
-	strncpy(question, (char *) ptr, qlen);
+	/*
+	 * 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 */
 
+	if ((eptr - ptr) < sizeof(struct domain_question))
+		return -EINVAL;
+
 	q = (void *) ptr;
 	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 += 2 + 2; /* ptr points now to answers */
-
-	err = -ENOMSG;
-	*response_len = 0;
-	*answers = 0;
+	ptr += sizeof(struct domain_question); /* advance to answers section */
 
-	memset(name, 0, sizeof(name));
+	ancount = ntohs(hdr->ancount);
+	qclass = ntohs(q->class);
 
 	/*
 	 * We have a bunch of answers (like A, AAAA, CNAME etc) to
@@ -1038,7 +1050,8 @@ 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++) {
+		char name[NS_MAXDNAME + 1] = {0};
 		/*
 		 * Get one address at a time to this buffer.
 		 * The max size of the answer is
@@ -1046,24 +1059,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
@@ -1075,8 +1091,6 @@ static int parse_response(unsigned char *buf, int buflen,
 		 * looking for.
 		 */
 		if (*class != qclass) {
-			ptr = next;
-			next = NULL;
 			continue;
 		}
 
@@ -1101,7 +1115,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
@@ -1125,8 +1139,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;
 			}
 
@@ -1138,12 +1150,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)
 			 */
@@ -1160,7 +1168,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;
@@ -1168,13 +1176,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);
 
@@ -1380,7 +1384,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.37.3

[PATCH 07/16] dnsproxy: refactoring of cache_update()

[Matthias Gerstner]

- move stack variables into more localized scopes
- use const parameters and variables where possible
- use named constants over literal numbers
- simplify some parsing details by using byte order macros or adding
  some comments to make the intentions clearer
---
 src/dnsproxy.c | 142 ++++++++++++++++++++++++-------------------------
 1 file changed, 69 insertions(+), 73 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 796b33fc3..c98e04eb6 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 */
@@ -1371,22 +1372,26 @@ 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 unsigned char *msg, size_t msg_len)
 {
-	size_t offset = protocol_offset(srv->protocol);
-	int err, qlen, ttl = 0;
+	const size_t offset = protocol_offset(srv->protocol);
+	int err, ttl = 0;
+	uint16_t *lenhdr;
+	size_t qlen;
+	bool is_new_entry = false;
 	uint16_t answers = 0, type = 0, class = 0;
 	struct domain_hdr *hdr = (void *)(msg + offset);
-	struct domain_question *q;
+	struct domain_question *q = NULL;
 	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;
+	unsigned char *ptr = NULL;
+	size_t rsplen = sizeof(response) - 1;
+	const time_t current_time = time(NULL);
 
 	if (cache_size >= MAX_CACHE_SIZE) {
 		cache_cleanup();
@@ -1394,8 +1399,6 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
 			return 0;
 	}
 
-	current_time = time(NULL);
-
 	/* don't do a cache refresh more than twice a minute */
 	if (next_refresh < current_time) {
 		cache_refresh();
@@ -1411,9 +1414,7 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
 	if (!cache)
 		create_cache();
 
-	rsplen = sizeof(response) - 1;
 	question[sizeof(question) - 1] = '\0';
-
 	err = parse_response(msg + offset, msg_len - offset,
 				question, sizeof(question) - 1,
 				&type, &class, &ttl,
@@ -1426,26 +1427,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) {
+					msg_len - offset) == DNS_TYPE_AAAA) {
 		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);
+			ptr = data->data;
+			if (srv->protocol == IPPROTO_UDP) {
+				/* add the two bytes length header also for
+				 * UDP responses */
+				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);
@@ -1454,9 +1458,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;
 		}
 	}
@@ -1464,8 +1466,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
@@ -1473,7 +1473,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);
+	data = NULL;
+	is_new_entry = !entry;
+
 	if (!entry) {
 		entry = g_try_new(struct cache_entry, 1);
 		if (!entry)
@@ -1490,37 +1494,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;
 
@@ -1528,14 +1523,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;
+
+	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
@@ -1546,45 +1548,39 @@ 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;
-	}
+	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);
+	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);
+	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) {
+	if (is_new_entry) {
 		g_hash_table_replace(cache, entry->key, entry);
 		cache_size++;
 	}
 
 	debug("cache %d %squestion \"%s\" type %d ttl %d size %zd packet %u "
 								"dns len %u",
-		cache_size, new_entry ? "new " : "old ",
+		cache_size, is_new_entry ? "new " : "old ",
 		question, type, ttl,
 		sizeof(*entry) + sizeof(*data) + data->data_len + qlen,
 		data->data_len,
-- 
2.37.3

[PATCH 08/16] 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 c98e04eb6..d32fbd6db 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1903,6 +1903,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.37.3

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

[Matthias Gerstner]

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

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index d32fbd6db..c4f4796c2 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1882,45 +1882,70 @@ 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)
 {
 	uint16_t data_len;
+	struct domain_rr *rr;
+	/* 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 */
-
-		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 */
+		rr = (void*)answers;
+		if (answers + sizeof(*rr) > end)
 			return -EINVAL;
-
+		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.37.3

[PATCH 10/16] dnsproxy: refactor ns_resolv() and forwards_dns_reply()

[Matthias Gerstner]

- document function behaviour in comments
- use early exits where possible to reduce indentation levels
- move stack variables into more localized scopes
- reduce some duplicate code in uncompress() calls
- add TODO about likely logical error that could have ramifications
  when fixing.
---
 src/dnsproxy.c | 296 +++++++++++++++++++++++++------------------------
 1 file changed, 153 insertions(+), 143 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index c4f4796c2..b34b952ba 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1591,39 +1591,41 @@ static int cache_update(struct server_data *srv, const unsigned char *msg, size_
 	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;
+	int ttl_left;
+	struct cache_data *data;
+	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;
+	data = type == DNS_TYPE_A ? entry->ipv4 : entry->ipv6;
 
-		if (data) {
-			ttl_left = data->valid_until - time(NULL);
-			entry->hits++;
-		}
+	if (!data)
+		return 0;
 
-		if (data && req->protocol == IPPROTO_TCP) {
+	ttl_left = data->valid_until - time(NULL);
+	entry->hits++;
+
+	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)
@@ -1637,6 +1639,24 @@ static int ns_resolv(struct server_data *server, struct request_data *req,
 		}
 	}
 
+	return -EINVAL;
+}
+
+static int ns_resolv(struct server_data *server, struct request_data *req,
+				gpointer request, gpointer name)
+{
+	int sk = -1;
+	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 */
 	sk = g_io_channel_unix_get_fd(server->channel);
 
 	err = sendto(sk, request, req->request_len, MSG_NOSIGNAL,
@@ -1652,51 +1672,52 @@ 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;
+	for (GList *list = server->domains; list; list = list->next) {
+		int domlen, altlen;
 		unsigned char alt[1024];
+		/* TODO: is this a bug? the offset isn't considered here... */
 		struct domain_hdr *hdr = (void *) &alt;
-		int altlen, domlen;
-		size_t offset = protocol_offset(server->protocol);
-
-		domain = list->data;
+		const char *domain = list->data;
+		const size_t offset = protocol_offset(server->protocol);
 
 		if (!domain)
 			continue;
 
 		domlen = strlen(domain) + 1;
+
 		if (domlen < 5)
 			return -EINVAL;
 
-		alt[offset] = req->altid & 0xff;
-		alt[offset + 1] = req->altid >> 8;
+		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,
+		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,
@@ -1948,94 +1969,87 @@ 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;
+	const size_t offset = protocol_offset(protocol);
 	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;
+	struct domain_hdr *hdr = (void *)(reply + offset);
+	int err, sk;
 
-	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);
+	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;
+			uint8_t host_len;
+			uint16_t domain_len, dns_type, dns_class;
 			const char *domain;
+			struct qtype_qclass *qtc;
+			const char *eom = reply + reply_len;
+			const uint16_t header_len = offset + DNS_HEADER_SIZE;
+			const uint16_t payload_len = reply_len - header_len;
+			const char *ptr = reply + header_len;
+
+			if (reply_len < header_len)
+				return -EINVAL;
+			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;
-
 			host_len = *ptr;
 			domain = ptr + 1 + host_len;
-			if (domain > eom)
+			if (domain >= eom)
 				return -EINVAL;
 
-			if (host_len > 0)
-				domain_len = strnlen(domain, eom - domain);
+			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)
+			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];
+
+			dns_type = ntohs(qtc->qtype);
+			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",
@@ -2057,17 +2071,25 @@ 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 uncompressed[NS_MAXDNAME];
+				size_t fixed_len;
+				int new_an_len;
+				char *uptr = &uncompressed[0];
 				char *answers;
+				const size_t len = host_len + 1;
+				const uint16_t section_counts[] = {
+					hdr->ancount,
+					hdr->nscount,
+					hdr->arcount
+				};
+
+				/* 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];
 				memcpy(uptr, ptr, len);
 
 				uptr[len] = '\0'; /* host termination */
@@ -2076,20 +2098,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;
+				ptr = (void*)(qtc + 1);
+				uptr += sizeof(*qtc);
 				answers = uptr;
 				fixed_len = answers - uncompressed;
-				if (ptr + offset > eom)
-					return -EINVAL;
 
 				/*
 				 * We then uncompress the result to buffer
@@ -2099,40 +2116,29 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
 				 * 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;
+				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,
-							uptr - answers);
-				if (new_len < 0) {
+				new_an_len = strip_domains(uncompressed, answers,
+							   uptr - answers);
+				if (new_an_len < 0) {
 					debug("Corrupted packet");
 					return -EINVAL;
 				}
@@ -2141,9 +2147,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)
@@ -2151,7 +2161,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;
 			}
@@ -2168,7 +2178,7 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
 		memcpy(req->resp, reply, reply_len);
 		req->resplen = reply_len;
 
-		cache_update(data, reply, reply_len);
+		cache_update(data, (unsigned char*)reply, reply_len);
 
 		g_free(new_reply);
 	}
@@ -2193,7 +2203,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;
@@ -2285,7 +2295,7 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
 	len = recv(sk, buf, sizeof(buf), 0);
 
 	if (len > 0) {
-		forward_dns_reply(buf, len, IPPROTO_UDP, data);
+		forward_dns_reply((char*)buf, len, IPPROTO_UDP, data);
 	}
 
 	return TRUE;
@@ -2479,7 +2489,7 @@ hangup:
 			reply->received += bytes_recv;
 		}
 
-		forward_dns_reply(reply->buf, reply->received, IPPROTO_TCP,
+		forward_dns_reply((char*)reply->buf, reply->received, IPPROTO_TCP,
 					server);
 
 		g_free(reply);
-- 
2.37.3

[PATCH 11/16] 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 b34b952ba..23873a381 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1781,7 +1781,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
@@ -1790,7 +1790,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,
@@ -1806,7 +1806,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);
 
@@ -1814,7 +1814,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;
@@ -1828,7 +1828,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;
@@ -1841,7 +1841,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);
@@ -1856,7 +1856,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];
@@ -1867,7 +1867,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;
@@ -1880,7 +1880,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;
@@ -1898,9 +1898,6 @@ static const char* uncompress(int16_t field_count, const char *start, const char
 	}
 
 	return ptr;
-
-out:
-	return NULL;
 }
 
 /*
-- 
2.37.3

[PATCH 12/16] 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 | 336 ++++++++++++++++++++++++++-----------------------
 1 file changed, 177 insertions(+), 159 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 23873a381..27a761241 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -47,6 +47,8 @@
 #	define debug(fmt...) do { } while (0)
 #endif
 
+#define NUM_ARRAY_ELEMENTS(a) sizeof(a) / sizeof(a[0])
+
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 struct domain_hdr {
 	uint16_t id;
@@ -1966,6 +1968,168 @@ 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 returns a new reply 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)
+{
+	char uncompressed[NS_MAXDNAME];
+	char *uptr, *answers;
+	size_t fixed_len;
+	int new_an_len;
+	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;
+
+	const uint16_t section_counts[] = {
+		hdr->ancount,
+		hdr->nscount,
+		hdr->arcount
+	};
+
+	/*
+	 * 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.
+	 */
+
+	/* NOTE: length checks up and including to qtype_qclass have already
+	   been done above */
+
+	/*
+	 * First copy host (without domain name) into tmp buffer.
+	 */
+	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);
+	answers = uptr;
+	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.
+	 */
+
+	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
+	 */
+	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)
 {
@@ -1992,8 +2156,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
@@ -2004,167 +2166,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) {
-			uint8_t host_len;
-			uint16_t domain_len, dns_type, dns_class;
-			const char *domain;
-			struct qtype_qclass *qtc;
-			const char *eom = reply + reply_len;
-			const uint16_t header_len = offset + DNS_HEADER_SIZE;
-			const uint16_t payload_len = reply_len - header_len;
-			const char *ptr = reply + header_len;
-
-			if (reply_len < header_len)
-				return -EINVAL;
-			if (payload_len < 1)
-				return -EINVAL;
-
-			/*
-			 * ptr points to the first char of the hostname.
-			 * ->hostname.domain.net
-			 */
-			host_len = *ptr;
-			domain = ptr + 1 + host_len;
-			if (domain >= eom)
-				return -EINVAL;
-
-			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.
-			 */
-			qtc = (void*)(domain + domain_len + 1);
-			if (((const char*)(qtc + 1)) > eom)
-				return -EINVAL;
-
-			dns_type = ntohs(qtc->qtype);
-			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];
-				size_t fixed_len;
-				int new_an_len;
-				char *uptr = &uncompressed[0];
-				char *answers;
-				const size_t len = host_len + 1;
-				const uint16_t section_counts[] = {
-					hdr->ancount,
-					hdr->nscount,
-					hdr->arcount
-				};
-
-				/* NOTE: length checks up and including to
-				 * qtype_qclass have already been done above */
-
-				/*
-				 * First copy host (without domain name) into
-				 * tmp buffer.
-				 */
-				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);
-				answers = uptr;
-				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.
-				 */
-
-				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
-				 */
-				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;
 
@@ -2180,7 +2199,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.37.3

[PATCH 13/16] dnsproxy: finish first full 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 | 457 ++++++++++++++++++++++---------------------------
 1 file changed, 208 insertions(+), 249 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 27a761241..d31ccd571 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,
@@ -443,33 +469,24 @@ 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)
 {
 	struct domain_hdr *hdr = NULL;
-	int offset, dns_len, err;
-
+	int err;
+	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;
 
 	hdr = (void *) (ptr + offset);
@@ -489,7 +506,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);
 
 	err = sendto(sk, ptr, len, MSG_NOSIGNAL, to, tolen);
@@ -498,7 +515,7 @@ static void send_cached_response(int sk, unsigned char *ptr, int len,
 				strerror(errno));
 	}
 	else if (err != 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",
 			err, len, dns_len);
 }
 
@@ -2319,10 +2336,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;
 
@@ -2346,8 +2361,7 @@ hangup:
 
 			if (req->protocol == IPPROTO_UDP)
 				continue;
-
-			if (!req->request)
+			else if (!req->request)
 				continue;
 
 			/*
@@ -2358,7 +2372,7 @@ hangup:
 			if (req->numserv && --(req->numserv))
 				continue;
 
-			hdr = (void *) (req->request + 2);
+			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);
@@ -2372,17 +2386,14 @@ 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);
@@ -2407,9 +2418,11 @@ hangup:
 		server->connected = true;
 		server_list = g_slist_append(server_list, server);
 
-		for (list = request_list; list; ) {
-			struct request_data *req = list->data;
+		/* 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; ) {
 			int status;
+			struct request_data *req = list->data;
 
 			if (req->protocol == IPPROTO_UDP) {
 				list = list->next;
@@ -2429,9 +2442,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;
 			}
@@ -2456,10 +2467,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) {
@@ -2472,8 +2482,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);
 
@@ -2534,15 +2545,15 @@ static gboolean tcp_idle_timeout(gpointer user_data)
 
 static int server_create_socket(struct server_data *data)
 {
-	int sk, err;
+	int err;
 	char *interface;
+	int sk = socket(data->server_addr->sa_family,
+		data->protocol == IPPROTO_TCP ? SOCK_STREAM : SOCK_DGRAM,
+		data->protocol);
 
 	debug("index %d server %s proto %d", data->index,
 					data->server, data->protocol);
 
-	sk = socket(data->server_addr->sa_family,
-		data->protocol == IPPROTO_TCP ? SOCK_STREAM : SOCK_DGRAM,
-		data->protocol);
 	if (sk < 0) {
 		err = errno;
 		connman_error("Failed to create server %s socket",
@@ -2613,9 +2624,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)
@@ -2634,13 +2643,12 @@ static struct server_data *create_server(int index,
 					const char *domain, const char *server,
 					int protocol)
 {
-	struct server_data *data;
+	struct server_data *data = g_try_new0(struct server_data, 1);
 	struct addrinfo hints, *rp;
 	int ret;
 
 	DBG("index %d server %s", index, server);
 
-	data = g_try_new0(struct server_data, 1);
 	if (!data) {
 		connman_error("Failed to allocate server %s data", server);
 		return NULL;
@@ -2653,20 +2661,7 @@ static struct server_data *create_server(int index,
 	data->protocol = protocol;
 
 	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;
 
@@ -2729,9 +2724,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) {
@@ -2760,26 +2753,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;
+		for (GList *dom_list = data->domains; dom_list;
 				dom_list = dom_list->next) {
 			dom = dom_list->data;
 
@@ -2812,9 +2801,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;
 
@@ -2842,22 +2829,20 @@ 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);
@@ -2877,16 +2862,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)
@@ -2901,19 +2883,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);
@@ -2924,11 +2905,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) {
@@ -2946,10 +2925,8 @@ 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;
+	bool any_server_enabled = false;
+	int index, vpn_index;
 
 	DBG("service %p", service);
 
@@ -2973,13 +2950,13 @@ static void dnsproxy_default_changed(struct connman_service *service)
 	 */
 	vpn_index = __connman_connection_get_vpn_index(index);
 
-	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) {
 			DBG("Enabling DNS server %s", data->server);
 			data->enabled = true;
-			server_enabled = true;
+			any_server_enabled = true;
 		} else if (data->index == vpn_index) {
 			DBG("Enabling DNS server of VPN %s", data->server);
 			data->enabled = true;
@@ -2989,7 +2966,7 @@ static void dnsproxy_default_changed(struct connman_service *service)
 		}
 	}
 
-	if (!server_enabled)
+	if (!any_server_enabled)
 		enable_fallback(true);
 
 	cache_refresh();
@@ -3037,47 +3014,59 @@ 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);
+	uint16_t qdcount, ancount, nscount, arcount;
+	unsigned char *ptr = buf + sizeof(struct domain_hdr);
+	size_t remain = len - sizeof(struct domain_hdr);
+	size_t used = 0;
 
+	if (len < sizeof(*hdr) + sizeof(struct qtype_qclass)) {
+		DBG("Dropped DNS request with short length %zd", len);
 		return -EINVAL;
 	}
 
 	if (!name || !size)
 		return -EINVAL;
 
+	qdcount = ntohs(hdr->qdcount);
+	ancount = ntohs(hdr->ancount);
+	nscount = ntohs(hdr->nscount);
+	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);
-
+	/* 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);
+			struct qtype_qclass *q = (struct qtype_qclass *)(ptr + 1);
+			uint16_t class;
 
 			if (remain < sizeof(*q)) {
 				DBG("Dropped malformed DNS query");
@@ -3085,7 +3074,7 @@ static int parse_request(unsigned char *buf, size_t len,
 			}
 
 			class = ntohs(q->qclass);
-			if (class != 1 && class != 255) {
+			if (class != DNS_CLASS_IN && class != DNS_CLASS_ANY) {
 				DBG("Dropped non-IN DNS class %d", class);
 				return -EINVAL;
 			}
@@ -3102,18 +3091,17 @@ static int parse_request(unsigned char *buf, size_t len,
 		strcat(name, ".");
 
 		used += label_len + 1;
-
 		ptr += label_len + 1;
 		remain -= label_len + 1;
 	}
 
 	if (arcount && remain >= sizeof(struct domain_rr) + 1 && !ptr[0] &&
-		ptr[1] == opt_edns0_type[0] && ptr[2] == opt_edns0_type[1]) {
+		ptr[1] == OPT_EDNS0_TYPE[0] && ptr[2] == OPT_EDNS0_TYPE[1]) {
 		struct domain_rr *edns0 = (struct domain_rr *)(ptr + 1);
 
 		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);
@@ -3150,7 +3138,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];
 }
@@ -3161,15 +3149,14 @@ static bool read_tcp_data(struct tcp_partial_client_data *client,
 {
 	char query[TCP_MAX_BUF_LEN];
 	struct request_data *req;
-	int client_sk, err;
-	unsigned int msg_len;
-	GSList *list;
+	struct domain_hdr *hdr;
+	int client_sk = g_io_channel_unix_get_fd(client->channel);
+	int err;
+	size_t msg_len;
 	bool waiting_for_connect = false;
 	uint16_t qtype = 0;
 	struct cache_entry *entry;
 
-	client_sk = g_io_channel_unix_get_fd(client->channel);
-
 	if (read_len == 0) {
 		debug("client %d closed, pending %d bytes",
 			client_sk, client->buf_end);
@@ -3182,32 +3169,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);
 	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,
-			query, sizeof(query));
+	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,
 			NULL, 0, IPPROTO_TCP);
@@ -3224,13 +3211,15 @@ read_another:
 	req->protocol = IPPROTO_TCP;
 	req->family = client->family;
 
-	req->srcid = client->buf[2] | (client->buf[3] << 8);
+	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;
@@ -3242,17 +3231,12 @@ read_another:
 	 */
 	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,
@@ -3265,7 +3249,7 @@ read_another:
 			debug("data missing, ignoring cache for this query");
 	}
 
-	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_UDP || !data->enabled)
@@ -3317,7 +3301,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,
@@ -3333,7 +3317,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;
 		}
@@ -3359,15 +3343,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,
@@ -3377,6 +3353,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;
@@ -3393,7 +3376,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) {
@@ -3413,9 +3396,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);
 
@@ -3428,8 +3409,12 @@ 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;
+	int sk = -1, client_sk = -1;
+	int recv_len;
+	size_t msg_len;
+	fd_set readfds;
+	struct timeval tv = {.tv_sec = 0, .tv_usec = 0};
+
 	struct tcp_partial_client_data *client;
 	struct sockaddr_in6 client_addr6;
 	socklen_t client_addr6_len = sizeof(client_addr6);
@@ -3437,8 +3422,6 @@ static bool tcp_listener_event(GIOChannel *channel, GIOCondition condition,
 	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);
@@ -3463,29 +3446,27 @@ static bool tcp_listener_event(GIOChannel *channel, GIOCondition condition,
 		client_addr_len = &client_addr6_len;
 	}
 
-	tv.tv_sec = tv.tv_usec = 0;
 	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;
 	}
 
+	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));
+	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) {
@@ -3529,8 +3510,8 @@ 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);
-	if (len < 0) {
+	recv_len = recv(client_sk, client->buf, TCP_MAX_BUF_LEN, 0);
+	if (recv_len < 0) {
 		if (errno == EAGAIN || errno == EWOULDBLOCK) {
 			debug("client %d no data to read, waiting", client_sk);
 			return true;
@@ -3543,15 +3524,15 @@ static bool tcp_listener_event(GIOChannel *channel, GIOCondition condition,
 		return true;
 	}
 
-	if (len < 2) {
+	if (recv_len < 2) {
 		debug("client %d not enough data to read, waiting", client_sk);
-		client->buf_end += len;
+		client->buf_end += recv_len;
 		return true;
 	}
 
 	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));
@@ -3562,15 +3543,15 @@ 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",
-			client_sk, len, msg_len + 2, msg_len + 2 - len);
+	if (msg_len != (size_t)(recv_len - DNS_HEADER_TCP_EXTRA_BYTES)) {
+		debug("client %d sent %d bytes but expecting %zd pending %zd",
+			client_sk, recv_len, msg_len + 2, msg_len + 2 - recv_len);
 
-		client->buf_end += len;
+		client->buf_end += recv_len;
 		return true;
 	}
 
-	return read_tcp_data(client, client_addr, *client_addr_len, len);
+	return read_tcp_data(client, client_addr, *client_addr_len, recv_len);
 }
 
 static gboolean tcp4_listener_event(GIOChannel *channel, GIOCondition condition,
@@ -3597,14 +3578,16 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition,
 {
 	unsigned char buf[768];
 	char query[512];
-	struct request_data *req;
+	struct request_data *req = NULL;
+	struct domain_hdr *hdr = NULL;
+	int sk = -1, err, len;
+
 	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");
@@ -3612,8 +3595,6 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition,
 		return false;
 	}
 
-	sk = g_io_channel_unix_get_fd(channel);
-
 	if (family == AF_INET) {
 		client_addr = &client_addr4;
 		client_addr_len = &client_addr4_len;
@@ -3623,6 +3604,7 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition,
 	}
 
 	memset(client_addr, 0, *client_addr_len);
+	sk = g_io_channel_unix_get_fd(channel);
 	len = recvfrom(sk, buf, sizeof(buf), 0, client_addr, client_addr_len);
 	if (len < 2)
 		return true;
@@ -3646,13 +3628,14 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition,
 	req->protocol = IPPROTO_UDP;
 	req->family = family;
 
-	req->srcid = buf[0] | (buf[1] << 8);
+	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;
@@ -3693,42 +3676,26 @@ static gboolean udp6_listener_event(GIOChannel *channel, GIOCondition condition,
 
 static GIOChannel *get_listener(int family, int protocol, int index)
 {
-	GIOChannel *channel;
-	const char *proto;
+	GIOChannel *channel = NULL;
 	union {
 		struct sockaddr sa;
 		struct sockaddr_in6 sin6;
 		struct sockaddr_in sin;
 	} s;
 	socklen_t slen;
-	int sk, type;
+	const char *proto = protocol_label(protocol);
+	const int type = socket_type(protocol, SOCK_CLOEXEC);
 	char *interface;
+	int sk = socket(family, type, protocol);
 
 	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;
-	}
-
 	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;
 	}
 
@@ -3787,7 +3754,6 @@ static GIOChannel *get_listener(int family, int protocol, int index)
 	}
 
 	if (protocol == IPPROTO_TCP) {
-
 		if (listen(sk, 10) < 0) {
 			connman_error("Failed to listen on TCP socket %d/%s",
 				-errno, strerror(errno));
@@ -3901,9 +3867,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;
 
@@ -3913,7 +3877,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");
@@ -3927,16 +3891,14 @@ static int create_listener(struct listener_data *ifdata)
 
 static void destroy_listener(struct listener_data *ifdata)
 {
-	int index;
-	GSList *list;
+	int index = connman_inet_ifindex("lo");
 
-	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)",
@@ -3997,7 +3959,6 @@ 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)
@@ -4050,17 +4011,15 @@ int __connman_dnsproxy_init(void)
 		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.37.3

[PATCH 14/16] 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 | 106 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 33 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index d31ccd571..5e2df1000 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1703,10 +1703,9 @@ static int ns_resolv(struct server_data *server, struct request_data *req,
 	for (GList *list = server->domains; list; list = list->next) {
 		int domlen, altlen;
 		unsigned char alt[1024];
-		/* TODO: is this a bug? the offset isn't considered here... */
-		struct domain_hdr *hdr = (void *) &alt;
 		const char *domain = list->data;
 		const size_t offset = protocol_offset(server->protocol);
+		struct domain_hdr *hdr = (void *) (&alt[0] + offset);
 
 		if (!domain)
 			continue;
@@ -1718,10 +1717,10 @@ static int ns_resolv(struct server_data *server, struct request_data *req,
 
 		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);
 
-		altlen = append_query(alt + offset + DNS_HEADER_SIZE, sizeof(alt) - DNS_HEADER_SIZE,
+		altlen = append_query(alt + offset + DNS_HEADER_SIZE, sizeof(alt) - DNS_HEADER_SIZE - offset,
 					name, domain);
 		if (altlen < 0)
 			return -EINVAL;
@@ -2147,31 +2146,41 @@ 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 unsigned char *reply, size_t len, int protocol)
 {
 	const size_t offset = protocol_offset(protocol);
 	struct request_data *req;
 	struct domain_hdr *hdr = (void *)(reply + offset);
-	int err, sk;
 
-	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;
 
 	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);
+	int err, sk;
+
 	/* 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
@@ -2248,8 +2257,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;
 }
 
@@ -2313,9 +2320,10 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
 							gpointer user_data)
 {
 	unsigned char buf[4096];
-	int sk;
+	int sk, res;
 	ssize_t len;
 	struct server_data *data = user_data;
+	struct request_data *req;
 
 	if (condition & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
 		connman_error("Error with UDP server %s", data->server);
@@ -2326,9 +2334,20 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
 	sk = g_io_channel_unix_get_fd(channel);
 	len = recv(sk, buf, sizeof(buf), 0);
 
-	if (len > 0) {
-		forward_dns_reply((char*)buf, len, IPPROTO_UDP, data);
-	}
+	if (len <= 0)
+		return TRUE;
+
+	req = lookup_request(buf, len, IPPROTO_UDP);
+
+	if (!req)
+		/* invalid / corrupt request */
+		return TRUE;
+
+	res = forward_dns_reply((char*)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;
 }
@@ -2336,6 +2355,7 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
 static gboolean tcp_server_event(GIOChannel *channel, GIOCondition condition,
 							gpointer user_data)
 {
+	struct request_data *req;
 	struct server_data *server = user_data;
 	int sk = g_io_channel_unix_get_fd(channel);
 	if (sk == 0)
@@ -2355,8 +2375,8 @@ hangup:
 
 		list = request_list;
 		while (list) {
-			struct request_data *req = list->data;
 			struct domain_hdr *hdr;
+			req = list->data;
 			list = list->next;
 
 			if (req->protocol == IPPROTO_UDP)
@@ -2422,7 +2442,7 @@ hangup:
 		 * need to delete elements while iterating through it */
 		for (GSList *list = request_list; list; ) {
 			int status;
-			struct request_data *req = list->data;
+			req = list->data;
 
 			if (req->protocol == IPPROTO_UDP) {
 				list = list->next;
@@ -2465,6 +2485,7 @@ hangup:
 	} else if (condition & G_IO_IN) {
 		struct partial_reply *reply = server->incoming_reply;
 		int bytes_recv;
+		int res;
 
 		if (!reply) {
 			uint16_t reply_len;
@@ -2479,7 +2500,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
@@ -2493,6 +2514,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;
@@ -2515,15 +2538,30 @@ hangup:
 			reply->received += bytes_recv;
 		}
 
-		forward_dns_reply((char*)reply->buf, reply->received, IPPROTO_TCP,
-					server);
+		req = lookup_request(reply->buf, reply->received, IPPROTO_TCP);
+
+		if (!req)
+			/* invalid / corrupt request */
+			return TRUE;
+
+		res = forward_dns_reply((char*)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;
@@ -3170,7 +3208,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;
 
 	msg_len = get_msg_len(client->buf);
@@ -3194,9 +3232,11 @@ read_another:
 
 	debug("client %d all data %zd received", client_sk, msg_len);
 
-	err = parse_request(client->buf + 2, msg_len, query, sizeof(query));
+	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;
 	}
@@ -3211,12 +3251,12 @@ read_another:
 	req->protocol = IPPROTO_TCP;
 	req->family = client->family;
 
-	hdr = (void*)(client->buf + 2);
+	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));
@@ -3300,7 +3340,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,
@@ -3524,7 +3564,7 @@ static bool tcp_listener_event(GIOChannel *channel, GIOCondition condition,
 		return true;
 	}
 
-	if (recv_len < 2) {
+	if (recv_len < DNS_HEADER_TCP_EXTRA_BYTES) {
 		debug("client %d not enough data to read, waiting", client_sk);
 		client->buf_end += recv_len;
 		return true;
-- 
2.37.3

[PATCH 15/16] dnsproxy: harmonize use of sizeof() for message size calculations

[Matthias Gerstner]

---
 src/dnsproxy.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 5e2df1000..f0ecd1b28 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -229,6 +229,9 @@ struct domain_rr {
 #define DNS_HEADER_SIZE sizeof(struct domain_hdr)
 #define DNS_HEADER_TCP_EXTRA_BYTES 2
 #define DNS_TCP_HEADER_SIZE DNS_HEADER_SIZE + DNS_HEADER_TCP_EXTRA_BYTES
+#define DNS_QUESTION_SIZE sizeof(struct domain_question)
+#define DNS_RR_SIZE sizeof(struct domain_rr)
+#define DNS_QTYPE_QCLASS_SIZE sizeof(struct qtype_qclass)
 
 enum dns_type {
 	/* IPv4 address 32-bit */
@@ -432,14 +435,14 @@ static void update_cached_ttl(unsigned char *ptr, int len, int new_ttl)
 	ptr += DNS_HEADER_SIZE;
 	len -= DNS_HEADER_SIZE;
 
-	if (len < sizeof(struct domain_question) + 1)
+	if (len < DNS_QUESTION_SIZE + 1)
 		return;
 
 	/* skip the query, which is a name and a struct domain_question */
 	name_len = dns_name_length(ptr);
 
-	ptr += name_len + sizeof(struct domain_question);
-	len -= name_len + sizeof(struct domain_question);;
+	ptr += name_len + DNS_QUESTION_SIZE;
+	len -= name_len + DNS_QUESTION_SIZE;
 
 	/* now we get the answer records */
 
@@ -973,10 +976,10 @@ static int parse_rr(const unsigned char *buf, const unsigned char *start,
 	if (*ttl < 0)
 		return -EINVAL;
 
-	memcpy(response + offset, *end, sizeof(struct domain_rr));
+	memcpy(response + offset, *end, DNS_RR_SIZE);
 
-	offset += sizeof(struct domain_rr);
-	*end += sizeof(struct domain_rr);
+	offset += DNS_RR_SIZE;
+	*end += DNS_RR_SIZE;
 
 	if ((offset + *rdlen) > *response_size)
 		return -ENOBUFS;
@@ -1049,7 +1052,7 @@ static int parse_response(const unsigned char *buf, size_t buflen,
 	qlen = strlen(question);
 	ptr += qlen + 1; /* skip \0 */
 
-	if ((eptr - ptr) < sizeof(struct domain_question))
+	if ((eptr - ptr) < DNS_QUESTION_SIZE)
 		return -EINVAL;
 
 	q = (void *) ptr;
@@ -1059,7 +1062,7 @@ static int parse_response(const unsigned char *buf, size_t buflen,
 	if (qtype != DNS_TYPE_A && qtype != DNS_TYPE_AAAA)
 		return -ENOMSG;
 
-	ptr += sizeof(struct domain_question); /* advance to answers section */
+	ptr += DNS_QUESTION_SIZE; /* advance to answers section */
 
 	ancount = ntohs(hdr->ancount);
 	qclass = ntohs(q->class);
@@ -1588,7 +1591,7 @@ static int cache_update(struct server_data *srv, const unsigned char *msg, size_
 	q = (void *)ptr;
 	q->type = htons(type);
 	q->class = htons(class);
-	ptr += sizeof(struct domain_question);
+	ptr += DNS_QUESTION_SIZE;
 
 	memcpy(ptr, response, rsplen);
 
@@ -2155,7 +2158,7 @@ static struct request_data* lookup_request(
 
 	debug("Received %zd bytes (id 0x%04x)", len, hdr->id);
 
-	if (len < sizeof(struct domain_hdr) + offset)
+	if (len < DNS_HEADER_SIZE + offset)
 		return NULL;
 
 	req = find_request(hdr->id);
@@ -3067,11 +3070,11 @@ static int parse_request(unsigned char *buf, size_t len,
 	static const unsigned char OPT_EDNS0_TYPE[2] = { 0x00, 0x29 };
 	struct domain_hdr *hdr = (void *) buf;
 	uint16_t qdcount, ancount, nscount, arcount;
-	unsigned char *ptr = buf + sizeof(struct domain_hdr);
-	size_t remain = len - sizeof(struct domain_hdr);
+	unsigned char *ptr = buf + DNS_HEADER_SIZE;
+	size_t remain = len - DNS_HEADER_SIZE;
 	size_t used = 0;
 
-	if (len < sizeof(*hdr) + sizeof(struct qtype_qclass)) {
+	if (len < DNS_HEADER_SIZE + DNS_QTYPE_QCLASS_SIZE) {
 		DBG("Dropped DNS request with short length %zd", len);
 		return -EINVAL;
 	}
@@ -3133,7 +3136,7 @@ static int parse_request(unsigned char *buf, size_t len,
 		remain -= label_len + 1;
 	}
 
-	if (arcount && remain >= sizeof(struct domain_rr) + 1 && !ptr[0] &&
+	if (arcount && remain >= DNS_RR_SIZE + 1 && !ptr[0] &&
 		ptr[1] == OPT_EDNS0_TYPE[0] && ptr[2] == OPT_EDNS0_TYPE[1]) {
 		struct domain_rr *edns0 = (struct domain_rr *)(ptr + 1);
 
-- 
2.37.3

[PATCH 16/16] dnsproxy: add my copyright statement covering the larger refactoring changes

[Matthias Gerstner]

---
 src/dnsproxy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index f0ecd1b28..c4d1e3935 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -3,6 +3,7 @@
  *  Connection Manager
  *
  *  Copyright (C) 2007-2014  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2022 Matthias Gerstner of SUSE. All rights reserved.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
-- 
2.37.3

Re: dnsproxy: first round of refactoring, TCP bugfix

[Daniel Wagner]

Hi Matthias,

On Thu, Oct 27, 2022 at 12:32:43PM +0200, Matthias Gerstner wrote:
> Sorry about the messed up previous set of patches.

No worries. Thanks for keeping it going.

> So this round features the following changes:
> 
> - every commit should now successfully compile and also compile without
>   warnings

One patch didn't apply cleanly due to the latest changes in dnsproxy.c
but it was easy to fix.

> - as requested I let go off most of the mixed code/declarations at the loss of
>   some const variables that now need to be declared non-const at the start
>   of the functions. In a few spots I left a little mixed code / declarations
>   just at the beginning of the function block to allow for early exits when
>   e.g. insufficient input data is supplied. Declarations at the beginning of
>   for/while/if blocks still remain where suitable.

All patches applied.

Thanks a lot!
Daniel