dotdoh: harden the DoQ listener and bound the dnsmasq children it occupies

Follow-up hardening on the inbound DoQ work, from review of the listener and of
how the three encrypted paths share the embedded dnsmasq.

Protocol handling:

- Trailing bytes on a DoQ stream were discarded silently. RFC 9250, Sec. 4.2
  gives each query its own stream, so anything after that one message is a
  protocol violation; the stream is now reset with `DOQ_PROTOCOL_ERROR` rather
  than letting a non-conformant client smuggle a second message past us. Not
  requiring the client's FIN before answering stays deliberate.
- `SO_REUSEADDR` is gone from the UDP listener. UDP has no `TIME_WAIT` to work
  around, and with the flag a second daemon binds 853 successfully while the
  kernel delivers the datagrams to only one of us. Without it the clash surfaces
  as `EADDRINUSE`.
- A pooled loopback socket the peer closed between the checkout probe and our
  write is now retried once on the write side, as the DoT path already did on
  the read side.
- `ossl_err()` reported the oldest queued OpenSSL error rather than the most
  recent, so an unrelated stale entry could be printed in place of the failure
  being described.

Concurrency:

Every in-flight query occupies one of dnsmasq's TCP children, of which it serves
60 by default (`--max-tcp-connections`). Nothing bounded our share: DoT allowed
one per connection, DoQ one per stream and DoH one per webserver thread, so the
three could ask for far more than exist and starve plain TCP queries and DNSSEC
fallback host-wide.

Admission is now counted in `dotdoh_loopback_take()`, which every path already
calls before obtaining a socket. It reserves a slot, or returns -2 once the limit
is reached so the caller can refuse the query rather than queue it - DoQ resets
the stream with `DOQ_EXCESSIVE_LOAD` so the client knows to back off. The new
`dotdoh_loopback_drop()` releases a socket that must not be reused and frees the
slot, so the error paths account as `dotdoh_loopback_give()` already did.

The limit is derived from `daemon->max_procs` rather than configured, leaving
`LOOPBACK_RESERVE` children for plain TCP: raising dnsmasq's own limit raises
ours, with no second setting to keep in step. Refusals are logged at most once a
minute with a count, naming both numbers, as a per-query line would flood the log
under exactly the overload that produces it.

DoH held a loopback connection per webserver thread for the life of that thread,
pinning a child whether or not it was serving anything and sitting outside any
accounting; DoT held one for the life of each keep-alive connection. Both now
borrow one per query, so idle clients occupy no slot, and the pool is kept as
large as the limit so a query at full concurrency still finds a warm socket
instead of forking. A blocking exchange also arms its own send and receive
timeouts now: the reactors create their sockets without any, correctly, as they
never block on them, and a borrowed one would otherwise be bounded only by
dnsmasq's 300 s child lifetime.

Also documents the `quic://` alias of `doq://`, which `parse_upstream_uri()` has
always accepted, in the `dns.upstreamCA` help text and both `validate_upstreams()`
comments.

Signed-off-by: DL6ER <dl6er@dl6er.de>

Author: DL6ER

src/config/config.c

@@ -604,7 +604,7 @@ void initConfig(struct config *conf)
 	conf->dns.revServers.f = FLAG_RESTART_FTL;
 
 	conf->dns.upstreamCA.k = "dns.upstreamCA";
-	conf->dns.upstreamCA.h = "Path to a CA certificate bundle used to verify encrypted upstream servers (DoT/DoH). If left empty, the system default trust store is used. Only relevant when at least one dns.upstreams entry uses an encrypted scheme (tls://, https://, h3:// or doq://).";
+	conf->dns.upstreamCA.h = "Path to a CA certificate bundle used to verify encrypted upstream servers (DoT/DoH). If left empty, the system default trust store is used. Only relevant when at least one dns.upstreams entry uses an encrypted scheme (tls://, https://, h3://, doq:// or quic://).";
 	conf->dns.upstreamCA.a = cJSON_CreateStringReference("A path to a PEM CA bundle, or empty for the system default trust store");
 	conf->dns.upstreamCA.t = CONF_STRING;
 	conf->dns.upstreamCA.d.s = (char*)"";

src/config/validator.c

@@ -810,8 +810,9 @@ bool validate_str_no_newline(union conf_value *val, const char *key, char err[VA
 
 // Validator for dns.upstreams. Enforces the same array/string/newline rules as
 // validate_array_no_newline() and, in addition, requires every encrypted entry
-// (tls://, https://, h3:// or doq://) to parse as a valid encrypted-upstream
-// URI. Plaintext entries are left untouched - dnsmasq validates those itself.
+// (tls://, https://, h3://, doq:// or quic://) to parse as a valid encrypted-
+// upstream URI. Plaintext entries are left untouched - dnsmasq validates those
+// itself.
 bool validate_upstreams(union conf_value *val, const char *key, char err[VALIDATOR_ERRBUF_LEN])
 {
 	if(!validate_array_no_newline(val, key, err))
@@ -825,11 +826,12 @@ bool validate_upstreams(union conf_value *val, const char *key, char err[VALIDAT
 			continue;
 
 		// Anything carrying a URI scheme ("://") must be a supported encrypted
-		// upstream (tls://, https://, h3:// or doq://) that parses cleanly. A
-		// plaintext server specification handled downstream by dnsmasq never
-		// contains "://", so an entry that does but is not a valid encrypted URI
-		// (e.g. http://, ftp:// or a malformed tls://) is rejected here rather
-		// than being written into dnsmasq.conf and breaking DNS startup.
+		// upstream (tls://, https://, h3://, doq:// or quic://) that parses
+		// cleanly. A plaintext server specification handled downstream by
+		// dnsmasq never contains "://", so an entry that does but is not a valid
+		// encrypted URI (e.g. http://, ftp:// or a malformed tls://) is rejected
+		// here rather than being written into dnsmasq.conf and breaking DNS
+		// startup.
 		if(strstr(s, "://") != NULL)
 		{
 			struct upstream_uri u;

src/dnsmasq_interface.c

@@ -4414,6 +4414,16 @@ void FTL_connection_error(const char *reason, const union mysockaddr *addr, cons
  *
  * @return true if the debug option is enabled, false otherwise.
  */
+unsigned int __attribute__ ((pure)) dnsmasq_max_tcp_children(void)
+{
+	// Read when the listeners start, i.e. after the config is parsed. Fall back to
+	// dnsmasq's own default if that ordering ever changes, so the derived cap can
+	// never come out as zero and refuse everything.
+	if(daemon != NULL && daemon->max_procs > 0)
+		return (unsigned int)daemon->max_procs;
+	return MAX_PROCS;
+}
+
 bool __attribute__ ((pure)) get_dnsmasq_debug(void)
 {
 	return option_bool(OPT_DEBUG);

src/dnsmasq_interface.h

@@ -54,6 +54,11 @@ bool FTL_is_forward_available(const union mysockaddr *addr);
 
 bool get_dnsmasq_debug(void) __attribute__ ((pure));
 
+// Concurrent TCP children dnsmasq will serve (--max-tcp-connections). The
+// encrypted listeners hand every in-flight query to one of these, so they derive
+// their own concurrency limit from it.
+unsigned int dnsmasq_max_tcp_children(void) __attribute__ ((pure));
+
 // defined in src/dnsmasq/cache.c
 extern char *querystr(char *desc, unsigned short type);
 

src/dotdoh/doq_server.c

@@ -80,7 +80,7 @@
 // Cap in-flight queries across all connections. Each holds ~192 KiB of pooled
 // I/O buffers plus one loopback socket to dnsmasq, so this - not the connection
 // count - is what bounds the listener's footprint and its load on dnsmasq.
-#define DOQ_MAX_STREAMS 64
+#define DOQ_MAX_STREAMS 32
 // Most concurrent streams a single connection may hold. Together with
 // DOQ_MAX_CONNS_PER_IP this bounds what one connection costs; the global
 // DOQ_MAX_STREAMS above is what actually caps total in-flight queries.
@@ -173,12 +173,12 @@ static int g_nstreams = 0;
 // Human-readable text for the most recent OpenSSL error. The certificate reload
 // path calls this from the DoQ thread while other threads use OpenSSL too, so it
 // renders into thread-local storage rather than ERR_error_string()'s process-wide
-// buffer, and drains the rest of the queue so a later message cannot report a
-// stale error.
+// buffer. Reads the most recent entry, not the oldest still queued, and drains
+// the queue afterwards so a later message cannot report a stale error.
 static const char *ossl_err(void)
 {
 	static _Thread_local char buf[256];
-	const unsigned long e = ERR_get_error();
+	const unsigned long e = ERR_peek_last_error();
 	if(e == 0)
 		return "no error";
 	ERR_error_string_n(e, buf, sizeof(buf));
@@ -287,8 +287,10 @@ static int doq_bind_socket(int family)
 		          family == AF_INET ? "IPv4" : "IPv6", strerror(errno));
 		return -1;
 	}
+	// No SO_REUSEADDR here: UDP has no TIME_WAIT to work around, and with the flag
+	// a second daemon binds the same port successfully while the kernel delivers
+	// the datagrams to only one of us. Without it the clash surfaces as EADDRINUSE.
 	const int one = 1;
-	setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &one, sizeof(one));
 	if(family == AF_INET6)
 		setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &one, sizeof(one));
 
@@ -441,7 +443,7 @@ static void stream_free(struct doq_stream *s)
 	if(s->ssl != NULL)
 		SSL_free(s->ssl);
 	if(s->upfd >= 0)
-		close(s->upfd);
+		dotdoh_loopback_drop(s->upfd);
 	if(s->conn != NULL)
 		s->conn->nstreams--;
 	// Keep the pooled I/O buffers attached to the slot for the next stream (they
@@ -691,10 +693,25 @@ static int stream_start_resolve(struct doq_stream *s)
 		s->up_ev = POLLOUT;
 		return 0;
 	}
+	if(s->upfd == -2)
+	{
+		// At the concurrency limit; server.c has already logged the summary. Tell
+		// the client why, as the stream-cap path does - a bare teardown reads as
+		// DOQ_NO_ERROR and gives it no reason to back off.
+		log_debug(DEBUG_TLS, "dotdoh: DoQ query from %s refused, concurrency limit reached",
+		          s->conn->client);
+		doq_reset_stream(s->ssl, DOQ_EXCESSIVE_LOAD);
+		return -1;
+	}
 	s->up_pooled = false;
 	s->upfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0);
 	if(s->upfd < 0)
+	{
+		// take() reserved an in-flight slot even though the pool was empty; give
+		// it back or the count never recovers.
+		dotdoh_loopback_drop(-1);
 		return -1;
+	}
 	struct sockaddr_in sa;
 	memset(&sa, 0, sizeof(sa));
 	sa.sin_family = AF_INET;
@@ -721,7 +738,7 @@ static int stream_start_resolve(struct doq_stream *s)
 // the drive contract: 1 keep driving, 0 yield, -1 give up.
 static int stream_retry_upstream(struct doq_stream *s)
 {
-	close(s->upfd);
+	dotdoh_loopback_drop(s->upfd);
 	s->upfd = -1;
 	s->up_pooled = false;
 	s->up_retried = true;
@@ -791,7 +808,17 @@ static int drive_read(struct doq_stream *s)
 	s->wlen = (size_t)flen;
 	s->woff = 0;
 
-	// One query per stream (RFC 9250 Sec. 4.2): anything after it is ignored.
+	// One query per stream (RFC 9250 Sec. 4.2). Bytes beyond that one message are
+	// a protocol violation, so fail the stream instead of silently dropping them:
+	// quietly ignoring them would let a non-conformant client smuggle a second
+	// message past us.
+	if(s->have > off + (size_t)qlen)
+	{
+		log_debug(DEBUG_TLS, "dotdoh: DoQ stream from %s carried %zu trailing bytes, "
+		          "resetting it", s->conn->client, s->have - off - (size_t)qlen);
+		doq_reset_stream(s->ssl, DOQ_PROTOCOL_ERROR);
+		return -1;
+	}
 	s->have = 0;
 
 	if(stream_start_resolve(s) != 0)
@@ -810,6 +837,10 @@ static int drive_up_write(struct doq_stream *s)
 		if(w > 0) { s->woff += (size_t)w; continue; }
 		if(w < 0 && errno == EINTR) continue;
 		if(w < 0 && errno == EAGAIN) { s->up_ev = POLLOUT; return 0; }
+		// A pooled socket the peer closed between the checkout probe and this
+		// write fails here; resend once on a fresh one, as the DoT path does.
+		if(s->up_pooled && !s->up_retried)
+			return stream_retry_upstream(s);
 		return -1;
 	}
 	s->alen = 0; s->agot = 0; s->up_lengot = 0;

src/dotdoh/dot_server.c

@@ -60,7 +60,7 @@
 // then pooled across connections (freed only at thread shutdown), so the cap
 // bounds the worst-case footprint while a connection flood costs no per-
 // connection allocation. The event loop itself scales far higher.
-#define DOT_MAX_CONNS 64
+#define DOT_MAX_CONNS 32
 // Cap concurrent connections from a single source IP so one client cannot hold
 // every slot (a drip of one query per connection re-arms the per-query deadline,
 // so the slow-loris sweep never fires; this is what bounds a single source).
@@ -259,7 +259,7 @@ static void conn_free(struct dot_conn *c)
 		if(c->up_idle)
 			dotdoh_loopback_give(c->upfd);
 		else
-			close(c->upfd);
+			dotdoh_loopback_drop(c->upfd);
 	}
 	// Keep the I/O buffers attached to the slot for the next connection to reuse
 	// (they are freed once, at thread shutdown); reset only the bookkeeping.
@@ -344,9 +344,18 @@ static int conn_start_resolve(struct dot_conn *c)
 		c->st = DS_UP_WRITE;
 		return 0;
 	}
+	if(c->upfd == -2)
+	{
+		log_debug(DEBUG_TLS, "dotdoh: DoT query from %s refused, concurrency limit reached",
+		          c->client);
+		return -1;
+	}
 	c->upfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0);
 	if(c->upfd < 0)
+	{
+		dotdoh_loopback_drop(-1);
 		return -1;
+	}
 	struct sockaddr_in sa;
 	memset(&sa, 0, sizeof(sa));
 	sa.sin_family = AF_INET;
@@ -375,7 +384,7 @@ static int conn_start_resolve(struct dot_conn *c)
 // (advanced, keep driving), 0 (reconnect in flight, yield), -1 (give up).
 static int conn_retry_upstream(struct dot_conn *c)
 {
-	close(c->upfd);
+	dotdoh_loopback_drop(c->upfd);
 	c->upfd = -1;
 	c->up_reused = false;
 	c->up_retried = true;
@@ -523,6 +532,24 @@ static int drive_up_read(struct dot_conn *c)
 	// a message boundary, so conn_free() may return it to the shared pool.
 	c->up_idle = true;
 
+	// Hand the loopback socket back now that the exchange is complete, rather
+	// than holding it until the connection closes. It stays warm in the pool for
+	// whoever needs it next - including this connection's next query - but an
+	// idle keep-alive client no longer occupies one of the shared admission
+	// slots, which would otherwise let a handful of idle DoT connections starve
+	// DoQ and DoH.
+	if(c->upfd >= 0)
+	{
+		dotdoh_loopback_give(c->upfd);
+		c->upfd = -1;
+		// Clear the boundary flag with the fd it described: the next query takes a
+		// fresh socket, and leaving it set would let conn_free() pool that one
+		// while it is still connecting.
+		c->up_idle = false;
+		c->up_reused = false;
+		c->up_retried = false;
+	}
+
 	// RFC 8467 Sec. 4: pad the answer only if the query asked for it.
 	if(c->client_padded)
 		c->alen = edns_pad_response(c->abuf, c->alen, ABUF_SZ);