dnsproxy: some initial refactoring and integration test

dnsproxy: some initial refactoring and integration test

[matthias.gerstner]

These are the first steps towards a bigger effort to refactor the dnsproxy
code. Some smaller fixes of compiler warnings and refactorings are already
included. Mostly this adds an integration test facility though that allows to
easily test the basic code paths of the dnsproxy without the need for root
privileges or extensive configuration. With this future changes to the
codebase can be more easily checked for regressions.

[PATCH 1/7] dnsproxy: remove unused domain parameter from `remove_server()`

[matthias.gerstner]

From: Matthias Gerstner <matthias.gerstner@suse.de>

---
 src/dnsproxy.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index cf1d36c7..ad8c5da2 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -2843,8 +2843,7 @@ int __connman_dnsproxy_append(int index, const char *domain,
 	return 0;
 }
 
-static void remove_server(int index, const char *domain,
-			const char *server, int protocol)
+static void remove_server(int index, const char *server, int protocol)
 {
 	struct server_data *data;
 	GSList *list;
@@ -2885,8 +2884,8 @@ int __connman_dnsproxy_remove(int index, const char *domain,
 	if (g_str_equal(server, "::1"))
 		return -ENODEV;
 
-	remove_server(index, domain, server, IPPROTO_UDP);
-	remove_server(index, domain, server, IPPROTO_TCP);
+	remove_server(index, server, IPPROTO_UDP);
+	remove_server(index, server, IPPROTO_TCP);
 
 	return 0;
 }
-- 
2.34.1

[PATCH 2/7] dnsproxy: remove unnecessarily shadowed variable

[matthias.gerstner]

From: Matthias Gerstner <matthias.gerstner@suse.de>

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

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index ad8c5da2..b195f447 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -2855,7 +2855,7 @@ static void remove_server(int index, const char *server, int protocol)
 	destroy_server(data);
 
 	for (list = server_list; list; list = list->next) {
-		struct server_data *data = list->data;
+		data = list->data;
 
 		if (data->index != -1 && data->enabled == true)
 			return;
-- 
2.34.1

[PATCH 3/7] dnsproxy: protocol_offset: remove error return case and return size_t

[matthias.gerstner]

From: Matthias Gerstner <matthias.gerstner@suse.de>

There is never any socket created that is not UDP or TCP, so this error
should never occur unless some memory corruption or similar occured.
Therefore `abort()` on unexpected values instead.

This simplifies the calling code since the error case no longer needs to
be covered (which can also be dangerous when forgotten, because then
negative error codes are used in calculations).
---
 src/dnsproxy.c | 41 ++++++++++++++---------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index b195f447..d04057fa 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -233,7 +233,7 @@ static guint16 get_id(void)
 	return rand;
 }
 
-static int protocol_offset(int protocol)
+static size_t protocol_offset(int protocol)
 {
 	switch (protocol) {
 	case IPPROTO_UDP:
@@ -243,9 +243,9 @@ static int protocol_offset(int protocol)
 		return 2;
 
 	default:
-		return -EINVAL;
+		// this should never happen
+		abort();
 	}
-
 }
 
 /*
@@ -480,13 +480,11 @@ static void send_response(int sk, unsigned char *buf, size_t len,
 				int protocol)
 {
 	struct domain_hdr *hdr;
-	int err, offset = protocol_offset(protocol);
+	int err;
+	size_t offset = protocol_offset(protocol);
 
 	debug("sk %d", sk);
 
-	if (offset < 0)
-		return;
-
 	if (len < sizeof(*hdr) + offset)
 		return;
 
@@ -802,16 +800,12 @@ static struct cache_entry *cache_check(gpointer request, int *qtype, int proto)
 	struct cache_entry *entry;
 	struct domain_question *q;
 	uint16_t type;
-	int offset, proto_offset;
+	int offset;
 
 	if (!request)
 		return NULL;
 
-	proto_offset = protocol_offset(proto);
-	if (proto_offset < 0)
-		return NULL;
-
-	question = request + proto_offset + 12;
+	question = request + protocol_offset(proto) + 12;
 
 	offset = strlen(question) + 1;
 	q = (void *) (question + offset);
@@ -1383,7 +1377,7 @@ static int reply_query_type(unsigned char *msg, int len)
 static int cache_update(struct server_data *srv, unsigned char *msg,
 			unsigned int msg_len)
 {
-	int offset = protocol_offset(srv->protocol);
+	size_t offset = protocol_offset(srv->protocol);
 	int err, qlen, ttl = 0;
 	uint16_t answers = 0, type = 0, class = 0;
 	struct domain_hdr *hdr = (void *)(msg + offset);
@@ -1411,10 +1405,7 @@ static int cache_update(struct server_data *srv, unsigned char *msg,
 		next_refresh = current_time + 30;
 	}
 
-	if (offset < 0)
-		return 0;
-
-	debug("offset %d hdr %p msg %p rcode %d", offset, hdr, msg, hdr->rcode);
+	debug("offset %zd hdr %p msg %p rcode %d", offset, hdr, msg, hdr->rcode);
 
 	/* Continue only if response code is 0 (=ok) */
 	if (hdr->rcode != ns_r_noerror)
@@ -1678,17 +1669,14 @@ static int ns_resolv(struct server_data *server, struct request_data *req,
 		char *domain;
 		unsigned char alt[1024];
 		struct domain_hdr *hdr = (void *) &alt;
-		int altlen, domlen, offset;
+		int altlen, domlen;
+		size_t offset = protocol_offset(server->protocol);
 
 		domain = list->data;
 
 		if (!domain)
 			continue;
 
-		offset = protocol_offset(server->protocol);
-		if (offset < 0)
-			return offset;
-
 		domlen = strlen(domain) + 1;
 		if (domlen < 5)
 			return -EINVAL;
@@ -1947,13 +1935,12 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
 {
 	struct domain_hdr *hdr;
 	struct request_data *req;
-	int dns_id, sk, err, offset = protocol_offset(protocol);
+	int dns_id, sk, err;
+	size_t offset = protocol_offset(protocol);
 
-	if (offset < 0)
-		return offset;
 	if (reply_len < 0)
 		return -EINVAL;
-	if (reply_len < offset + 1)
+	if ((size_t)reply_len < offset + 1)
 		return -EINVAL;
 	if ((size_t)reply_len < sizeof(struct domain_hdr))
 		return -EINVAL;
-- 
2.34.1

[PATCH 4/7] .gitignore: also ignore backup configure script

[matthias.gerstner]

From: Matthias Gerstner <matthias.gerstner@suse.de>

---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index cd7d8810..4e0c0aa7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -15,6 +15,7 @@ config.log
 config.status
 config.sub
 configure
+configure~
 depcomp
 compile
 install-sh
-- 
2.34.1

[PATCH 5/7] dnsproxy: support programmatic configuration of the default listen port

[matthias.gerstner]

From: Matthias Gerstner <matthias.gerstner@suse.de>

This way a test instance of dnsproxy can be setup that listens on an
unprivileged port for easier testing as non-root.
---
 src/connman.h  |  1 +
 src/dnsproxy.c | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/connman.h b/src/connman.h
index 68176086..909b53ae 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -983,6 +983,7 @@ void __connman_dnsproxy_remove_listener(int index);
 int __connman_dnsproxy_append(int index, const char *domain, const char *server);
 int __connman_dnsproxy_remove(int index, const char *domain, const char *server);
 int __connman_dnsproxy_set_mdns(int index, bool enabled);
+void __connman_dnsproxy_set_listen_port(unsigned int port);
 
 int __connman_6to4_probe(struct connman_service *service);
 void __connman_6to4_remove(struct connman_ipconfig *ipconfig);
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index d04057fa..f862c788 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -223,6 +223,7 @@ static GHashTable *listener_table = NULL;
 static time_t next_refresh;
 static GHashTable *partial_tcp_req_table;
 static guint cache_timer = 0;
+static in_port_t dns_listen_port = 53;
 
 static guint16 get_id(void)
 {
@@ -3704,7 +3705,7 @@ static GIOChannel *get_listener(int family, int protocol, int index)
 	if (family == AF_INET6) {
 		memset(&s.sin6, 0, sizeof(s.sin6));
 		s.sin6.sin6_family = AF_INET6;
-		s.sin6.sin6_port = htons(53);
+		s.sin6.sin6_port = htons(dns_listen_port);
 		slen = sizeof(s.sin6);
 
 		if (__connman_inet_get_interface_address(index,
@@ -3721,7 +3722,7 @@ static GIOChannel *get_listener(int family, int protocol, int index)
 	} else if (family == AF_INET) {
 		memset(&s.sin, 0, sizeof(s.sin));
 		s.sin.sin_family = AF_INET;
-		s.sin.sin_port = htons(53);
+		s.sin.sin_port = htons(dns_listen_port);
 		slen = sizeof(s.sin);
 
 		if (__connman_inet_get_interface_address(index,
@@ -4050,3 +4051,8 @@ void __connman_dnsproxy_cleanup(void)
 	if (ipv6_resolve)
 		g_resolv_unref(ipv6_resolve);
 }
+
+void __connman_dnsproxy_set_listen_port(unsigned int port)
+{
+	dns_listen_port = port;
+}
-- 
2.34.1

[PATCH 6/7] dnsproxy-test: support command line specification of dnsproxy port

[matthias.gerstner]

From: Matthias Gerstner <matthias.gerstner@suse.de>

This way a dnsproxy running on an unprivileged port can be tested.
---
 tools/dnsproxy-test.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/dnsproxy-test.c b/tools/dnsproxy-test.c
index 371e2e23..7660db71 100644
--- a/tools/dnsproxy-test.c
+++ b/tools/dnsproxy-test.c
@@ -99,6 +99,8 @@ static unsigned char msg_invalid[] = {
 	0x31, 0xC0, /* tran id */
 };
 
+static const char *dns_port = "53";
+
 static int create_tcp_socket(int family)
 {
 	int sk, err;
@@ -139,7 +141,7 @@ static int connect_tcp_socket(char *server)
 	hints.ai_socktype = SOCK_STREAM;
 	hints.ai_family = AF_UNSPEC;
 	hints.ai_flags = AI_NUMERICSERV | AI_NUMERICHOST;
-	getaddrinfo(server, "53", &hints, &rp);
+	getaddrinfo(server, dns_port, &hints, &rp);
 
 	sk = create_tcp_socket(rp->ai_family);
 	err = sk;
@@ -201,7 +203,7 @@ static int connect_udp_socket(char *server, struct sockaddr *sa,
 	hints.ai_socktype = SOCK_DGRAM;
 	hints.ai_family = AF_UNSPEC;
 	hints.ai_flags = AI_NUMERICSERV | AI_NUMERICHOST;
-	getaddrinfo(server, "53", &hints, &rp);
+	getaddrinfo(server, dns_port, &hints, &rp);
 
 	sk = create_udp_socket(rp->ai_family);
 	err = sk;
@@ -430,6 +432,11 @@ int main(int argc, char *argv[])
 {
 	g_test_init(&argc, &argv, NULL);
 
+	if (argc == 2) {
+		// alternative dns port
+		dns_port = argv[1];
+	}
+
 	g_test_add_func("/dnsproxy/ipv4 udp msg",
 			test_ipv4_udp_msg);
 
-- 
2.34.1

[PATCH 7/7] dnsproxy: add standalone test version of dnsproxy and a test script for it

[matthias.gerstner]

From: Matthias Gerstner <matthias.gerstner@suse.de>

Since dnsproxy is not really modular this requires building complete
connman minus the original main function. It makes testing a lot easier
though, since this standalone version doesn't require root privileges or
configuration aspects like the D-Bus connection etc.

Add this as EXTRA_PROGRAMS, not as CHECK_PROGRAMS, because the test
might need some specific settings to work (e.g. a working
/etc/resolv.conf with domain entry).
---
 Makefile.am                 |  13 +++
 tools/dnsproxy-simple-test  | 112 ++++++++++++++++++++++++++
 tools/dnsproxy-standalone.c | 154 ++++++++++++++++++++++++++++++++++++
 3 files changed, 279 insertions(+)
 create mode 100755 tools/dnsproxy-simple-test
 create mode 100644 tools/dnsproxy-standalone.c

diff --git a/Makefile.am b/Makefile.am
index e5718b19..ea532071 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -106,6 +106,7 @@ builtin_cflags =
 
 noinst_PROGRAMS =
 bin_PROGRAMS =
+EXTRA_PROGRAMS =
 
 unit_objects =
 
@@ -412,6 +413,18 @@ endif
 tools_dnsproxy_test_SOURCES = tools/dnsproxy-test.c
 tools_dnsproxy_test_LDADD = @GLIB_LIBS@
 
+tools_dnsproxy_standalone_CFLAGS = $(src_connmand_CFLAGS) -I$(srcdir)/src
+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
+tools/dnsproxy-standalone.c: $(BUILT_SOURCES)
+tools_dnsproxy_standalone_LDADD = $(src_connmand_LDADD)
+# pass -zmuldefs to let the linker tolerate the duplicate definition of
+# main(), the first definition from dnsproxy-standalone should be used
+tools_dnsproxy_standalone_LDFLAGS = $(src_connmand_LDFLAGS) -Wl,-zmuldefs
+
+EXTRA_PROGRAMS += tools/dnsproxy-standalone
+
 endif
 
 test_scripts = test/get-state test/list-services \
diff --git a/tools/dnsproxy-simple-test b/tools/dnsproxy-simple-test
new file mode 100755
index 00000000..b8fd7f77
--- /dev/null
+++ b/tools/dnsproxy-simple-test
@@ -0,0 +1,112 @@
+#!/bin/bash
+
+# this script runs the dnsproxy-standalone test program and runs a couple of
+# standard DNS queries against it, using the currently configured DNS server
+# in the system as dnsproxy configuration.
+
+echoerr() {
+	echo $@ 1>&2
+	exit 1
+}
+
+while [ $# -gt 0 ]; do
+	case "$1" in
+	"--valgrind")
+		VALGRIND=`which valgrind`
+		if [ -z "$VALGRIND" ]; then
+			echoerr "no valgrind executable found"
+		fi
+		;;
+	"-h")
+		echo "$0 [--valgrind]"
+		echo "--valgrind: run dnsproxy-standalone in valgrind"
+		exit 2
+		;;
+	*)
+		echoerr "Unknown argument $1"
+		;;
+	esac
+	shift
+done
+
+if [ -e "Makefile" ]; then
+	BUILDROOT="$PWD"
+else
+	if [ ! -n "$BUILDROOT" ]; then
+		echoerr "You need to set the BUILDROOT environment variable or run this script from the connman build tree root"
+	fi
+
+	pushd "$BUILDROOT" >/dev/null || echoerr "couldn't enter $BUILDROOT"
+fi
+make tools/dnsproxy-standalone || echoerr "failed to build dnsproxy-standalone"
+
+HOST=`which host`
+if [ -z "$HOST" ]; then
+	echoerr "Couldn't find 'host' DNS utility"
+fi
+
+DNSPROXY="$BUILDROOT/tools/dnsproxy-standalone"
+
+if [ ! -f "$DNSPROXY" ]; then
+	echoerr "standalone dnsproxy does not exist at $DNSPROXY"
+fi
+
+NS1=`grep -w nameserver -m 1 /etc/resolv.conf | cut -d ' ' -f 2`
+if [ -z "$NS1" ]; then
+	echoerr "Failed to determine system's nameserver from /etc/resolv.conf"
+fi
+
+DOMAIN1=`grep -w search -m 1 /etc/resolv.conf | cut -d ' ' -f 2`
+if [ -z "$DOMAIN1" ]; then
+	echoerr "Failed to determine default DNS domain from /etc/resolv.conf"
+fi
+
+# use an unprivileged port for the proxy so we don't need special permissions
+# to run this test
+PORT=8053
+
+# run the proxy in the background
+$VALGRIND $DNSPROXY $PORT "$DOMAIN1" "$NS1" &
+proxy_pid=$!
+
+cleanup() {
+	if [ $proxy_pid -eq -1 ]; then
+		return 0
+	fi
+	kill $proxy_pid
+	wait $proxy_pid
+	ret=$?
+	proxy_pid=-1
+	return $ret
+}
+
+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
+
+		echo -e "\n\n"
+	done
+done
+
+echo -e "\n\nDNS resolution succeeded for all test combinations"
+cleanup
+if [ $? -eq 0 ]; then
+	exit 0
+else
+	echoerr "dnsproxy returned non-zero exit status $?"
+fi
+
diff --git a/tools/dnsproxy-standalone.c b/tools/dnsproxy-standalone.c
new file mode 100644
index 00000000..690b4313
--- /dev/null
+++ b/tools/dnsproxy-standalone.c
@@ -0,0 +1,154 @@
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/signalfd.h>
+#include <unistd.h>
+
+#include <glib.h>
+
+#include "connman.h"
+
+/*
+ * This is a minimal connman setup  that only runs the internal dnsproxy
+ * component for testing. The advantage is that we can do a full integration
+ * test of the dnsproxy logic without requiring root privileges or setting up
+ * other complexities like D-Bus access etc.
+ */
+
+static GMainLoop *main_loop = NULL;
+
+static void usage(const char *prog)
+{
+	fprintf(stderr, "%s: <listen-port> <dns-domain> <dns-server>\n", prog);
+	exit(1);
+}
+
+static unsigned int to_uint(const char *s)
+{
+	char *end = NULL;
+	unsigned int ret;
+
+	ret = strtoul(s, &end, 10);
+
+	if (*end != '\0') {
+		fprintf(stderr, "invalid argument: %s", s);
+		exit(1);
+	}
+
+	return ret;
+}
+
+static gboolean signal_handler(GIOChannel *channel, GIOCondition cond,
+							gpointer user_data)
+{
+	struct signalfd_siginfo si;
+	ssize_t result;
+	int fd;
+
+	if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP))
+		return FALSE;
+
+	fd = g_io_channel_unix_get_fd(channel);
+
+	result = read(fd, &si, sizeof(si));
+	if (result != sizeof(si))
+		return FALSE;
+
+	switch (si.ssi_signo) {
+	case SIGINT:
+	case SIGTERM:
+		printf("Terminating due to signal\n");
+		g_main_loop_quit(main_loop);
+		break;
+	}
+
+	return TRUE;
+}
+
+static guint setup_signalfd(void)
+{
+	GIOChannel *channel;
+	guint source;
+	sigset_t mask;
+	int fd;
+
+	sigemptyset(&mask);
+	sigaddset(&mask, SIGINT);
+	sigaddset(&mask, SIGTERM);
+
+	if (sigprocmask(SIG_BLOCK, &mask, NULL) < 0) {
+		perror("Failed to set signal mask");
+		return 0;
+	}
+
+	fd = signalfd(-1, &mask, 0);
+	if (fd < 0) {
+		perror("Failed to create signal descriptor");
+		return 0;
+	}
+
+	channel = g_io_channel_unix_new(fd);
+
+	g_io_channel_set_close_on_unref(channel, TRUE);
+	g_io_channel_set_encoding(channel, NULL, NULL);
+	g_io_channel_set_buffered(channel, FALSE);
+
+	source = g_io_add_watch(channel,
+				G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+				signal_handler, NULL);
+
+	g_io_channel_unref(channel);
+
+	return source;
+}
+
+int main(int argc, const char **argv)
+{
+	unsigned int port = 0;
+	const char *domain = argv[2];
+	const char *server = argv[3];
+	guint signal = 0;
+
+	if (argc != 4)
+	{
+		usage(argv[0]);
+	}
+
+	port = to_uint(argv[1]);
+
+	__connman_util_init();
+	printf("Listening on local port %u\n", port);
+	__connman_dnsproxy_set_listen_port(port);
+
+	if (__connman_dnsproxy_init() < 0) {
+		fprintf(stderr, "failed to initialize dnsproxy\n");
+		return 1;
+	}
+
+	printf("Using DNS server %s on domain %s\n", server, domain);
+
+	if (__connman_dnsproxy_append(-1, domain, server) < 0) {
+		fprintf(stderr, "failed to add DNS server\n");
+		return 1;
+	}
+
+	// we need to trick a bit to make the server entry enter "enabled"
+	// state in dnsproxy. Appending and removing an arbitrary entry causes
+	// "enable_fallback()" to be called which does what we want. Doesn't
+	// make much sense but it is good enough for the standalone server at
+	// the moment.
+	__connman_dnsproxy_append(15, domain, server);
+	__connman_dnsproxy_remove(15, domain, server);
+
+	signal = setup_signalfd();
+
+	main_loop = g_main_loop_new(NULL, FALSE);
+
+	g_main_loop_run(main_loop);
+
+	__connman_dnsproxy_cleanup();
+	__connman_util_cleanup();
+	g_source_remove(signal);
+
+	return 0;
+}
-- 
2.34.1

Re: [PATCH 7/7] dnsproxy: add standalone test version of dnsproxy and a test script for it

[Daniel Wagner]

Hi Matthias,

On Thu, Feb 17, 2022 at 12:31:13PM +0100, matthias.gerstner@suse.de wrote:
> +tools_dnsproxy_standalone_CFLAGS = $(src_connmand_CFLAGS) -I$(srcdir)/src
> +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
> +tools/dnsproxy-standalone.c: $(BUILT_SOURCES)
> +tools_dnsproxy_standalone_LDADD = $(src_connmand_LDADD)
> +# pass -zmuldefs to let the linker tolerate the duplicate definition of
> +# main(), the first definition from dnsproxy-standalone should be used
> +tools_dnsproxy_standalone_LDFLAGS = $(src_connmand_LDFLAGS) -Wl,-zmuldefs
> +
> +EXTRA_PROGRAMS += tools/dnsproxy-standalone

I would move this part just under the other tools section. These 'tools'
are not supposed to be installed.

> --- /dev/null
> +++ b/tools/dnsproxy-simple-test
> @@ -0,0 +1,112 @@
> +#!/bin/bash
> +
> +# this script runs the dnsproxy-standalone test program and runs a couple of
> +# standard DNS queries against it, using the currently configured DNS server
> +# in the system as dnsproxy configuration.
> +
> +echoerr() {
> +	echo $@ 1>&2
> +	exit 1
> +}
> +
> +while [ $# -gt 0 ]; do
> +	case "$1" in
> +	"--valgrind")
> +		VALGRIND=`which valgrind`
> +		if [ -z "$VALGRIND" ]; then
> +			echoerr "no valgrind executable found"
> +		fi
> +		;;
> +	"-h")
> +		echo "$0 [--valgrind]"
> +		echo "--valgrind: run dnsproxy-standalone in valgrind"
> +		exit 2
> +		;;
> +	*)
> +		echoerr "Unknown argument $1"
> +		;;
> +	esac
> +	shift
> +done
> +
> +if [ -e "Makefile" ]; then
> +	BUILDROOT="$PWD"
> +else
> +	if [ ! -n "$BUILDROOT" ]; then
> +		echoerr "You need to set the BUILDROOT environment variable or run this script from the connman build tree root"
> +	fi
> +
> +	pushd "$BUILDROOT" >/dev/null || echoerr "couldn't enter $BUILDROOT"
> +fi
> +make tools/dnsproxy-standalone || echoerr "failed to build dnsproxy-standalone"

Just build this tool along with the other tools. No need for this
special casing IMO. It's also not working in my setup:

$ ./tools/dnsproxy-simple-test
gcc -Wall -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -O0 -U_FORTIFY_SOURCE -g    tools/dnsproxy-standalone.c   -o tools/dnsproxy-standalone
tools/dnsproxy-standalone.c:7:10: fatal error: glib.h: No such file or directory
    7 | #include <glib.h>
      |          ^~~~~~~~

Thanks,
Daniel

Re: [PATCH 4/7] .gitignore: also ignore backup configure script

[Daniel Wagner]

On Thu, Feb 17, 2022 at 12:31:10PM +0100, matthias.gerstner@suse.de wrote:
> From: Matthias Gerstner <matthias.gerstner@suse.de>
> 
> ---
>  .gitignore | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/.gitignore b/.gitignore
> index cd7d8810..4e0c0aa7 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -15,6 +15,7 @@ config.log
>  config.status
>  config.sub
>  configure
> +configure~

I have changed this to a more generic *~

Re: [PATCH 3/7] dnsproxy: protocol_offset: remove error return case and return size_t

[Daniel Wagner]

On Thu, Feb 17, 2022 at 12:31:09PM +0100, matthias.gerstner@suse.de wrote:
> From: Matthias Gerstner <matthias.gerstner@suse.de>
> 
> There is never any socket created that is not UDP or TCP, so this error
> should never occur unless some memory corruption or similar occured.
> Therefore `abort()` on unexpected values instead.
> 
> This simplifies the calling code since the error case no longer needs to
> be covered (which can also be dangerous when forgotten, because then
> negative error codes are used in calculations).
> ---
>  src/dnsproxy.c | 41 ++++++++++++++---------------------------
>  1 file changed, 14 insertions(+), 27 deletions(-)
> 
> diff --git a/src/dnsproxy.c b/src/dnsproxy.c
> index b195f447..d04057fa 100644
> --- a/src/dnsproxy.c
> +++ b/src/dnsproxy.c
> @@ -233,7 +233,7 @@ static guint16 get_id(void)
>  	return rand;
>  }
>  
> -static int protocol_offset(int protocol)
> +static size_t protocol_offset(int protocol)
>  {
>  	switch (protocol) {
>  	case IPPROTO_UDP:
> @@ -243,9 +243,9 @@ static int protocol_offset(int protocol)
>  		return 2;
>  
>  	default:
> -		return -EINVAL;
> +		// this should never happen

We always use /* */ comment style. I've updated this accordingly.

Re: dnsproxy: some initial refactoring and integration test

[Daniel Wagner]

Hi Matthias,

On Thu, Feb 17, 2022 at 12:31:06PM +0100, matthias.gerstner@suse.de wrote:
> These are the first steps towards a bigger effort to refactor the dnsproxy
> code. Some smaller fixes of compiler warnings and refactorings are already
> included. Mostly this adds an integration test facility though that allows to
> easily test the basic code paths of the dnsproxy without the need for root
> privileges or extensive configuration. With this future changes to the
> codebase can be more easily checked for regressions.

Nice contribution. I've updated small nitpicks and applied patch
1-6. I think patch 7 should just hook up into the tools build target and
it doesn't need any special Makefile hackery.

Thanks,
Daniel

dnsproxy: some initial refactoring and integration test

[matthias.gerstner]

As discussed off-list, adding this test variant of connmand to noinst_PROGRAMS
means that all major object files will be built twice by default, considerably
increasing the build time with no benefit for most users.

Therefore only add the test program if `--enable-test` is set during configure
time. Pass this switch in bootstrap-configure to allow developers to catch
errors in the test source code.

[PATCH] dnsproxy: add standalone test version of dnsproxy and a test script for it

[matthias.gerstner]

From: Matthias Gerstner <matthias.gerstner@suse.de>

Since dnsproxy is not really modular this requires building complete
connman minus the original main function. It makes testing a lot easier
though, since this standalone version doesn't require root privileges or
configuration aspects like the D-Bus connection etc.

Only build this if `--enable-test` is given, otherwise complete connmand
would be built twice just by typing `make`. Also don't add this to
CHECK_PROGRAMS, because the test could need some specific settings to
work (e.g. a working /etc/resolv.conf with domain entry).

Add `--enable-test` to bootstrap-configure such that developers notice
possible build errors introduced in the regression test build.
---
 Makefile.am                 |  13 +++
 bootstrap-configure         |   3 +-
 tools/dnsproxy-simple-test  | 112 ++++++++++++++++++++++++++
 tools/dnsproxy-standalone.c | 154 ++++++++++++++++++++++++++++++++++++
 4 files changed, 281 insertions(+), 1 deletion(-)
 create mode 100755 tools/dnsproxy-simple-test
 create mode 100644 tools/dnsproxy-standalone.c

diff --git a/Makefile.am b/Makefile.am
index e5718b19..5ca8e6be 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -434,6 +434,19 @@ test_scripts += test/vpn-connect test/vpn-disconnect test/vpn-get \
 if TEST
 testdir = $(pkglibdir)/test
 test_SCRIPTS = $(test_scripts)
+
+tools_dnsproxy_standalone_CFLAGS = $(src_connmand_CFLAGS) -I$(srcdir)/src
+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
+tools/dnsproxy-standalone.c: $(BUILT_SOURCES)
+tools_dnsproxy_standalone_LDADD = $(src_connmand_LDADD)
+# pass -zmuldefs to let the linker tolerate the duplicate definition of
+# main(), the first definition from dnsproxy-standalone should be used
+tools_dnsproxy_standalone_LDFLAGS = $(src_connmand_LDFLAGS) -Wl,-zmuldefs
+
+noinst_PROGRAMS += tools/dnsproxy-standalone
+
 endif
 
 EXTRA_DIST += $(test_scripts)
diff --git a/bootstrap-configure b/bootstrap-configure
index 3f697987..ed5ecd78 100755
--- a/bootstrap-configure
+++ b/bootstrap-configure
@@ -18,4 +18,5 @@ fi
 		--enable-vpnc=builtin \
 		--enable-session-policy-local=builtin \
 		--enable-nmcompat \
-		--enable-polkit $*
+		--enable-polkit $* \
+		--enable-test
diff --git a/tools/dnsproxy-simple-test b/tools/dnsproxy-simple-test
new file mode 100755
index 00000000..b8fd7f77
--- /dev/null
+++ b/tools/dnsproxy-simple-test
@@ -0,0 +1,112 @@
+#!/bin/bash
+
+# this script runs the dnsproxy-standalone test program and runs a couple of
+# standard DNS queries against it, using the currently configured DNS server
+# in the system as dnsproxy configuration.
+
+echoerr() {
+	echo $@ 1>&2
+	exit 1
+}
+
+while [ $# -gt 0 ]; do
+	case "$1" in
+	"--valgrind")
+		VALGRIND=`which valgrind`
+		if [ -z "$VALGRIND" ]; then
+			echoerr "no valgrind executable found"
+		fi
+		;;
+	"-h")
+		echo "$0 [--valgrind]"
+		echo "--valgrind: run dnsproxy-standalone in valgrind"
+		exit 2
+		;;
+	*)
+		echoerr "Unknown argument $1"
+		;;
+	esac
+	shift
+done
+
+if [ -e "Makefile" ]; then
+	BUILDROOT="$PWD"
+else
+	if [ ! -n "$BUILDROOT" ]; then
+		echoerr "You need to set the BUILDROOT environment variable or run this script from the connman build tree root"
+	fi
+
+	pushd "$BUILDROOT" >/dev/null || echoerr "couldn't enter $BUILDROOT"
+fi
+make tools/dnsproxy-standalone || echoerr "failed to build dnsproxy-standalone"
+
+HOST=`which host`
+if [ -z "$HOST" ]; then
+	echoerr "Couldn't find 'host' DNS utility"
+fi
+
+DNSPROXY="$BUILDROOT/tools/dnsproxy-standalone"
+
+if [ ! -f "$DNSPROXY" ]; then
+	echoerr "standalone dnsproxy does not exist at $DNSPROXY"
+fi
+
+NS1=`grep -w nameserver -m 1 /etc/resolv.conf | cut -d ' ' -f 2`
+if [ -z "$NS1" ]; then
+	echoerr "Failed to determine system's nameserver from /etc/resolv.conf"
+fi
+
+DOMAIN1=`grep -w search -m 1 /etc/resolv.conf | cut -d ' ' -f 2`
+if [ -z "$DOMAIN1" ]; then
+	echoerr "Failed to determine default DNS domain from /etc/resolv.conf"
+fi
+
+# use an unprivileged port for the proxy so we don't need special permissions
+# to run this test
+PORT=8053
+
+# run the proxy in the background
+$VALGRIND $DNSPROXY $PORT "$DOMAIN1" "$NS1" &
+proxy_pid=$!
+
+cleanup() {
+	if [ $proxy_pid -eq -1 ]; then
+		return 0
+	fi
+	kill $proxy_pid
+	wait $proxy_pid
+	ret=$?
+	proxy_pid=-1
+	return $ret
+}
+
+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
+
+		echo -e "\n\n"
+	done
+done
+
+echo -e "\n\nDNS resolution succeeded for all test combinations"
+cleanup
+if [ $? -eq 0 ]; then
+	exit 0
+else
+	echoerr "dnsproxy returned non-zero exit status $?"
+fi
+
diff --git a/tools/dnsproxy-standalone.c b/tools/dnsproxy-standalone.c
new file mode 100644
index 00000000..690b4313
--- /dev/null
+++ b/tools/dnsproxy-standalone.c
@@ -0,0 +1,154 @@
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/signalfd.h>
+#include <unistd.h>
+
+#include <glib.h>
+
+#include "connman.h"
+
+/*
+ * This is a minimal connman setup  that only runs the internal dnsproxy
+ * component for testing. The advantage is that we can do a full integration
+ * test of the dnsproxy logic without requiring root privileges or setting up
+ * other complexities like D-Bus access etc.
+ */
+
+static GMainLoop *main_loop = NULL;
+
+static void usage(const char *prog)
+{
+	fprintf(stderr, "%s: <listen-port> <dns-domain> <dns-server>\n", prog);
+	exit(1);
+}
+
+static unsigned int to_uint(const char *s)
+{
+	char *end = NULL;
+	unsigned int ret;
+
+	ret = strtoul(s, &end, 10);
+
+	if (*end != '\0') {
+		fprintf(stderr, "invalid argument: %s", s);
+		exit(1);
+	}
+
+	return ret;
+}
+
+static gboolean signal_handler(GIOChannel *channel, GIOCondition cond,
+							gpointer user_data)
+{
+	struct signalfd_siginfo si;
+	ssize_t result;
+	int fd;
+
+	if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP))
+		return FALSE;
+
+	fd = g_io_channel_unix_get_fd(channel);
+
+	result = read(fd, &si, sizeof(si));
+	if (result != sizeof(si))
+		return FALSE;
+
+	switch (si.ssi_signo) {
+	case SIGINT:
+	case SIGTERM:
+		printf("Terminating due to signal\n");
+		g_main_loop_quit(main_loop);
+		break;
+	}
+
+	return TRUE;
+}
+
+static guint setup_signalfd(void)
+{
+	GIOChannel *channel;
+	guint source;
+	sigset_t mask;
+	int fd;
+
+	sigemptyset(&mask);
+	sigaddset(&mask, SIGINT);
+	sigaddset(&mask, SIGTERM);
+
+	if (sigprocmask(SIG_BLOCK, &mask, NULL) < 0) {
+		perror("Failed to set signal mask");
+		return 0;
+	}
+
+	fd = signalfd(-1, &mask, 0);
+	if (fd < 0) {
+		perror("Failed to create signal descriptor");
+		return 0;
+	}
+
+	channel = g_io_channel_unix_new(fd);
+
+	g_io_channel_set_close_on_unref(channel, TRUE);
+	g_io_channel_set_encoding(channel, NULL, NULL);
+	g_io_channel_set_buffered(channel, FALSE);
+
+	source = g_io_add_watch(channel,
+				G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+				signal_handler, NULL);
+
+	g_io_channel_unref(channel);
+
+	return source;
+}
+
+int main(int argc, const char **argv)
+{
+	unsigned int port = 0;
+	const char *domain = argv[2];
+	const char *server = argv[3];
+	guint signal = 0;
+
+	if (argc != 4)
+	{
+		usage(argv[0]);
+	}
+
+	port = to_uint(argv[1]);
+
+	__connman_util_init();
+	printf("Listening on local port %u\n", port);
+	__connman_dnsproxy_set_listen_port(port);
+
+	if (__connman_dnsproxy_init() < 0) {
+		fprintf(stderr, "failed to initialize dnsproxy\n");
+		return 1;
+	}
+
+	printf("Using DNS server %s on domain %s\n", server, domain);
+
+	if (__connman_dnsproxy_append(-1, domain, server) < 0) {
+		fprintf(stderr, "failed to add DNS server\n");
+		return 1;
+	}
+
+	// we need to trick a bit to make the server entry enter "enabled"
+	// state in dnsproxy. Appending and removing an arbitrary entry causes
+	// "enable_fallback()" to be called which does what we want. Doesn't
+	// make much sense but it is good enough for the standalone server at
+	// the moment.
+	__connman_dnsproxy_append(15, domain, server);
+	__connman_dnsproxy_remove(15, domain, server);
+
+	signal = setup_signalfd();
+
+	main_loop = g_main_loop_new(NULL, FALSE);
+
+	g_main_loop_run(main_loop);
+
+	__connman_dnsproxy_cleanup();
+	__connman_util_cleanup();
+	g_source_remove(signal);
+
+	return 0;
+}
-- 
2.34.1

Re: [PATCH] dnsproxy: add standalone test version of dnsproxy and a test script for it

[Daniel Wagner]

Hi Matthias,

On Mon, Feb 28, 2022 at 02:19:21PM +0100, matthias.gerstner@suse.de wrote:
>  if TEST
>  testdir = $(pkglibdir)/test
>  test_SCRIPTS = $(test_scripts)
> +
> +tools_dnsproxy_standalone_CFLAGS = $(src_connmand_CFLAGS) -I$(srcdir)/src
> +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
> +tools/dnsproxy-standalone.c: $(BUILT_SOURCES)
> +tools_dnsproxy_standalone_LDADD = $(src_connmand_LDADD)
> +# pass -zmuldefs to let the linker tolerate the duplicate definition of
> +# main(), the first definition from dnsproxy-standalone should be used
> +tools_dnsproxy_standalone_LDFLAGS = $(src_connmand_LDFLAGS) -Wl,-zmuldefs
> +
> +noinst_PROGRAMS += tools/dnsproxy-standalone
> +

I wrapped this section into an addition 'if INTERNAL_DNS_BACKEND'
because it would fail to build for SYSTEMD_RESOLVED_DNS_BACKEND

> +	// we need to trick a bit to make the server entry enter "enabled"
> +	// state in dnsproxy. Appending and removing an arbitrary entry causes
> +	// "enable_fallback()" to be called which does what we want. Doesn't
> +	// make much sense but it is good enough for the standalone server at
> +	// the moment.

Changed this to /* ... */

Patch applied.

Thanks
Daniel