fixes from main

Dockerfile

@@ -36,8 +36,8 @@ RUN dnf -y update && dnf install -y python3.14-devel @development-tools gcc-c++
     vectorscan-devel libtins-devel libpcap-devel boost-devel cargo
 
 COPY ./backend/binsrc /execute/binsrc
-RUN g++ binsrc/nfregex.cpp -o cppregex -std=c++23 -O3 -lnetfilter_queue -pthread -lnfnetlink $(pkg-config --cflags --libs libtins libhs libmnl)
+RUN g++ binsrc/nfregex.cpp -o cppregex -std=c++23 -O3 -Wall -lnetfilter_queue -pthread -lnfnetlink $(pkg-config --cflags --libs libtins libhs libmnl)
-RUN g++ binsrc/pyfilter.cpp -o cpproxy -std=c++23 -O3 -lnetfilter_queue -lpython3.14 -pthread -lnfnetlink $(pkg-config --cflags --libs libtins libmnl python3)
+RUN g++ binsrc/pyfilter.cpp -o cpproxy -std=c++23 -O3 -Wall -lnetfilter_queue -lpython3.14 -pthread -lnfnetlink $(pkg-config --cflags --libs libtins libmnl python3)
 
 # The proxy datapath engine, built here rather than in a rust image on purpose: it
 # links the same libhs the C++ binaries do, and a binary built against one distro's

backend/binsrc/classes/netfilter.cpp

@@ -1,5 +1,6 @@
 #include <vector>
 #include <thread>
+#include <iostream>
 #include <type_traits>
 #include "../utils.cpp"
 #include "nfqueue.cpp"
@@ -80,7 +81,8 @@ public:
     static constexpr int QUEUE_BASE_NUM = 1000;
 
     explicit MultiThreadQueue(size_t n_threads) 
-        : n_threads(n_threads), workers(n_threads) 
+        // Listed in declaration order (that is the order they are really built in).
+        : workers(n_threads), n_threads(n_threads) 
     {
         if(n_threads == 0) throw std::invalid_argument("At least 1 thread required");
         
@@ -105,8 +107,19 @@ public:
         for(auto& worker : workers) {
             worker.run_thread_loop();
         }
+		/*  One unexpected message must not end the interception. Whatever
+		    `handle_next_packet` throws — a netlink read that failed, a packet
+		    libtins refuses — the nftables rules stay installed either way, so
+		    leaving this loop means a service that still claims to be filtering
+		    and is not. */
 		for (;;){
-        	nfq->handle_next_packet(&workers);
+			try {
+				nfq->handle_next_packet(&workers);
+			} catch (const std::exception& e) {
+				std::cerr << "[error] [MultiThreadQueue.start] " << e.what() << std::endl;
+			} catch (...) {
+				std::cerr << "[error] [MultiThreadQueue.start] unknown error while handling a packet" << std::endl;
+			}
 		}
     }
 

backend/binsrc/classes/nfqueue.cpp

@@ -8,12 +8,40 @@
 #include <libmnl/libmnl.h>
 #include <tins/tins.h>
 #include <map>
+#include <vector>
+#include <iostream>
+#include <cerrno>
+#include <cstdlib>
+#include <cstring>
 
 using namespace std;
 
 namespace Firegex{
 namespace NfQueue{
 
+/*  Largest packet we are willing to hand back to the kernel in a verdict.
+    NFQUEUE payloads are carried in a netlink attribute whose length field is
+    16 bit wide, so anything above this can never be re-injected anyway; the
+    cap exists to stop a python filter from making us allocate an arbitrary
+    amount of memory through the payload it returns. */
+const size_t MAX_VERDICT_PACKET_SIZE = 0xffff;
+
+/*  Reads the fail-open policy once, tolerating a missing variable.
+    `strcmp(getenv(...), "1")` was the previous spelling, which dereferences a
+    null pointer the moment the binary is run without the variable set — by
+    hand, or by anything that is not the backend.
+    Note that this only drives NFQA_CFG_F_FAIL_OPEN (what the kernel does when
+    the queue is full) and the fallback verdict used when a packet cannot be
+    parsed; the "what happens when this process is not running at all" half of
+    the policy lives in the nftables rules (the `bypass` queue flag). */
+inline bool nfqueue_fail_open(){
+	static const bool value = [](){
+		const char* env = getenv("FIREGEX_NFQUEUE_FAIL_OPEN");
+		return env != nullptr && strcmp(env, "1") == 0;
+	}();
+	return value;
+}
+
 enum class FilterAction{ DROP, ACCEPT, MANGLE, NOACTION };
 enum class L4Proto { TCP, UDP, RAW };
 typedef Tins::TCPIP::StreamIdentifier stream_id;
@@ -115,12 +143,13 @@ class PktRequest {
 
 	public:
 
+	// Listed in declaration order (that is the order they are really built in).
 	PktRequest(const char* payload, size_t plen, T* ctx, mnl_socket* nl, nfgenmsg *nfg, nfqnl_msg_packet_hdr *ph, bool is_input):
-		ctx(ctx), nl(nl), res_id(nfg->res_id),
+		action(FilterAction::NOACTION), nl(nl), res_id(nfg->res_id),
-		packet_id(ph->packet_id), is_input(is_input),
+		packet_id(ph->packet_id),
+		is_ipv6((payload[0] & 0xf0) == 0x60), is_input(is_input),
 		packet(string(payload, plen)),
-		action(FilterAction::NOACTION),
+		ctx(ctx)
-		is_ipv6((payload[0] & 0xf0) == 0x60)
 	{
 		if (is_ipv6){
 			ipv6 = new Tins::IPv6((uint8_t*)packet.c_str(), plen);
@@ -304,6 +333,15 @@ class PktRequest {
 	void mangle_custom_data(const char* data_ptr, size_t data_len){
 		if (action == FilterAction::NOACTION){
 			try{
+				/*  The payload comes from the operator's python, and what is
+				    rebuilt from it is what goes back to the kernel in a verdict.
+				    A netlink attribute carries its length in 16 bits, so a
+				    bigger packet could never be re-injected anyway — refusing it
+				    here is what stops a filter from making this process
+				    allocate an arbitrary amount of memory for nothing. */
+				if (data_len + _header_size > MAX_VERDICT_PACKET_SIZE){
+					throw invalid_argument("Mangled packet is too big to be re-injected");
+				}
 				set_data(data_ptr, data_len);
 				reserialize();
 				action = FilterAction::MANGLE;
@@ -338,7 +376,16 @@ class PktRequest {
 
 	private:
 	void perform_action(bool do_serialize = true){
-		char buf[MNL_SOCKET_BUFFER_SIZE+packet.size()];
+		/*  This used to be a VLA sized on the (attacker influenced) packet
+		    length, which overflows the worker stack as soon as a filter mangles
+		    a packet into something large. A thread_local buffer keeps the
+		    allocation off the stack while still avoiding a malloc per packet. */
+		static thread_local vector<char> verdict_buffer;
+		const size_t needed = MNL_SOCKET_BUFFER_SIZE + packet.size();
+		if (verdict_buffer.size() < needed){
+			verdict_buffer.resize(needed);
+		}
+		char* buf = verdict_buffer.data();
 		struct nlmsghdr *nlh_verdict = nfq_nlmsg_put(buf, NFQNL_MSG_VERDICT, ntohs(res_id));
 		switch (action)
 		{
@@ -455,9 +502,7 @@ class NfQueue {
 		nlh = nfq_nlmsg_put(queue_msg_buffer, NFQNL_MSG_CONFIG, queue_num);
 		nfq_nlmsg_cfg_put_params(nlh, NFQNL_COPY_PACKET, 0xffff);
 
-		char * enable_fail_open = getenv("FIREGEX_NFQUEUE_FAIL_OPEN");
+		if (nfqueue_fail_open()){
-
-		if (strcmp(enable_fail_open, "1") == 0){
 			mnl_attr_put_u32(nlh, NFQA_CFG_FLAGS, htonl(NFQA_CFG_F_GSO|NFQA_CFG_F_FAIL_OPEN));
 			mnl_attr_put_u32(nlh, NFQA_CFG_MASK, htonl(NFQA_CFG_F_GSO|NFQA_CFG_F_FAIL_OPEN));
 		}else{
@@ -504,6 +549,18 @@ class NfQueue {
     
     private:
 
+	/*  Issues a plain verdict for a packet we could not build a PktRequest for.
+	    Every early return below has to go through this: a packet that is never
+	    given a verdict stays pinned in the kernel queue until it fills up. */
+	static void _send_raw_verdict(mnl_socket* nl, uint16_t res_id, uint32_t packet_id, int verdict) {
+		char buf[MNL_SOCKET_BUFFER_SIZE];
+		struct nlmsghdr *nlh_verdict = nfq_nlmsg_put(buf, NFQNL_MSG_VERDICT, ntohs(res_id));
+		nfq_nlmsg_verdict_put(nlh_verdict, ntohl(packet_id), verdict);
+		if (mnl_socket_sendto(nl, nlh_verdict, nlh_verdict->nlmsg_len) < 0) {
+			cerr << "[error] [NfQueue._send_raw_verdict] failed to send the verdict" << endl;
+		}
+	}
+
     static int _real_queue_cb(const nlmsghdr *nlh, void *data_ptr) {
 		
         internal_nfqueue_execution_data_tmp* info = (internal_nfqueue_execution_data_tmp*) data_ptr;
@@ -511,28 +568,37 @@ class NfQueue {
 		//Extract attributes from the nlmsghdr
 		nlattr *attr[NFQA_MAX+1] = {};
 		
+		/*  None of the paths below may return MNL_CB_ERROR: that aborts
+		    mnl_cb_run(), which the caller turns into an exception. A single
+		    unexpected message must never be able to take the interceptor down,
+		    because the nftables rules outlive the process. */
 		if (nfq_nlmsg_parse(nlh, attr) < 0) {
 			cerr << "[error] [NfQueue._real_queue_cb] problems parsing" << endl;
-			return MNL_CB_ERROR;
+			return MNL_CB_OK;
 		}
 		if (attr[NFQA_PACKET_HDR] == nullptr) {
+			// Without the header there is no packet id, so there is nothing to
+			// answer: just skip the message.
 			cerr << "[error] [NfQueue._real_queue_cb] packet header not set" << endl;
-			return MNL_CB_ERROR;
+			return MNL_CB_OK;
-		}
-		if (attr[NFQA_MARK] == nullptr) {
-			cerr << "[error] [NfQueue._real_queue_cb] mark not set" << endl;
-			return MNL_CB_ERROR;
 		}
 		
 		struct nfqnl_msg_packet_hdr *ph = (nfqnl_msg_packet_hdr*) mnl_attr_get_payload(attr[NFQA_PACKET_HDR]);
 		struct nfgenmsg *nfg = (nfgenmsg *)mnl_nlmsg_get_payload(nlh);
 
+		// A packet we cannot inspect follows the service's fail-open policy
+		// instead of being silently let through.
+		const int fallback_verdict = nfqueue_fail_open() ? NF_ACCEPT : NF_DROP;
+
+		if (attr[NFQA_MARK] == nullptr) {
+			cerr << "[error] [NfQueue._real_queue_cb] mark not set" << endl;
+			_send_raw_verdict(info->nl, nfg->res_id, ph->packet_id, fallback_verdict);
+			return MNL_CB_OK;
+		}
+
 		if (attr[NFQA_PAYLOAD] == nullptr) {
 			cerr << "[error] [NfQueue._real_queue_cb] payload not set" << endl;
-			char buf[MNL_SOCKET_BUFFER_SIZE];
+			_send_raw_verdict(info->nl, nfg->res_id, ph->packet_id, NF_ACCEPT);
-			struct nlmsghdr *nlh_verdict = nfq_nlmsg_put(buf, NFQNL_MSG_VERDICT, ntohs(nfg->res_id));
-			nfq_nlmsg_verdict_put(nlh_verdict, ntohl(ph->packet_id), NF_ACCEPT);
-			mnl_socket_sendto(info->nl, nlh_verdict, nlh_verdict->nlmsg_len);
 			return MNL_CB_OK;
 		}
 
@@ -540,10 +606,58 @@ class NfQueue {
 		uint16_t plen = mnl_attr_get_payload_len(attr[NFQA_PAYLOAD]);
 		char *payload = (char *)mnl_attr_get_payload(attr[NFQA_PAYLOAD]);
 
+		if (plen == 0) {
+			cerr << "[error] [NfQueue._real_queue_cb] empty payload" << endl;
+			_send_raw_verdict(info->nl, nfg->res_id, ph->packet_id, NF_ACCEPT);
+			return MNL_CB_OK;
+		}
+
+		/*  NFQA_CFG_F_GSO is enabled, so the kernel can hand us a super packet
+		    bigger than the copy range; NFQA_CAP_LEN then reports the real
+		    length. Parsing a truncated packet yields a wrong view of the stream
+		    and re-injecting one would corrupt the connection, so treat it like
+		    any other packet we cannot inspect. */
+		if (attr[NFQA_CAP_LEN] != nullptr) {
+			uint32_t cap_len = ntohl(mnl_attr_get_u32(attr[NFQA_CAP_LEN]));
+			if (cap_len > plen) {
+				cerr << "[warning] [NfQueue._real_queue_cb] truncated packet ("
+					 << plen << " of " << cap_len << " bytes), applying the fail-open policy" << endl;
+				_send_raw_verdict(info->nl, nfg->res_id, ph->packet_id, fallback_verdict);
+				return MNL_CB_OK;
+			}
+		}
+
 		bool is_input = ntohl(mnl_attr_get_u32(attr[NFQA_MARK])) & 0x1; // == 0x1337 that is odd
-		handle_func(new PktRequest<D>(
+
-			payload, plen, (D*)info->data, info->nl, nfg, ph, is_input
+		/*  libtins throws (Tins::malformed_packet & friends) on a malformed ip
-		));
+		    header, truncated tcp options or bogus ipv6 extension headers. That
+		    exception would otherwise unwind through libmnl's C frames and kill
+		    the whole process, which is a remotely triggerable way of disabling
+		    the firewall: one crafted packet is enough. */
+		PktRequest<D>* pkt = nullptr;
+		try {
+			pkt = new PktRequest<D>(
+				payload, plen, (D*)info->data, info->nl, nfg, ph, is_input
+			);
+		} catch (const std::exception& e) {
+			cerr << "[error] [NfQueue._real_queue_cb] cannot parse the packet: " << e.what() << endl;
+			_send_raw_verdict(info->nl, nfg->res_id, ph->packet_id, fallback_verdict);
+			return MNL_CB_OK;
+		} catch (...) {
+			cerr << "[error] [NfQueue._real_queue_cb] cannot parse the packet (unknown error)" << endl;
+			_send_raw_verdict(info->nl, nfg->res_id, ph->packet_id, fallback_verdict);
+			return MNL_CB_OK;
+		}
+
+		try {
+			handle_func(pkt); // Takes ownership of pkt
+		} catch (const std::exception& e) {
+			cerr << "[error] [NfQueue._real_queue_cb] cannot enqueue the packet: " << e.what() << endl;
+			if (pkt->get_action() == FilterAction::NOACTION) {
+				_send_raw_verdict(info->nl, nfg->res_id, ph->packet_id, fallback_verdict);
+			}
+			delete pkt;
+		}
 		
 		return MNL_CB_OK;
     }
@@ -554,6 +668,7 @@ class NfQueue {
 			nl = nullptr;
 		}
 		delete[] queue_msg_buffer;
+		queue_msg_buffer = nullptr;
 	}
 
     inline ssize_t _send_config_cmd(nfqnl_msg_config_cmds cmd){
@@ -562,8 +677,31 @@ class NfQueue {
 		return mnl_socket_sendto(nl, nlh, nlh->nlmsg_len);
 	}
 
+	/*  Retries the errors that simply mean "this message is gone, carry on".
+	    ENOBUFS signals kernel side packet loss, ENOSPC is what libmnl reports
+	    for a netlink message too big for our buffer (reachable with GSO super
+	    packets) and EINTR is just a signal. Treating any of them as fatal used
+	    to abort the whole interceptor. */
 	inline ssize_t _recv_packet(){
-		return mnl_socket_recvfrom(nl, queue_msg_buffer, NFQUEUE_BUFFER_SIZE);
+		for(;;){
+			ssize_t ret = mnl_socket_recvfrom(nl, queue_msg_buffer, NFQUEUE_BUFFER_SIZE);
+			if (ret >= 0){
+				return ret;
+			}
+			switch (errno) {
+				case EINTR:
+					continue;
+				case ENOBUFS:
+					cerr << "[warning] [NfQueue._recv_packet] the kernel dropped packets (ENOBUFS)" << endl;
+					continue;
+				case ENOSPC:
+				case EMSGSIZE:
+					cerr << "[warning] [NfQueue._recv_packet] netlink message too big for the buffer, skipped" << endl;
+					continue;
+				default:
+					return -1;
+			}
+		}
 	}	
 
 };

backend/binsrc/nfregex.cpp

@@ -44,6 +44,9 @@ void config_updater (){
 		}catch(const std::exception& e){
 			cerr << "[error] [updater] Failed to build new configuration!" << endl;
 			osyncstream(cout) << "ACK FAIL " << e.what() << endl;
+		}catch(...){
+			cerr << "[error] [updater] Failed to build new configuration!" << endl;
+			osyncstream(cout) << "ACK FAIL unknown error" << endl;
 		}
 	}
 	
@@ -76,7 +79,7 @@ int main(int argc, char *argv[]){
 		stream_mode = false;
 	}
 	
-	bool fail_open = strcmp(getenv("FIREGEX_NFQUEUE_FAIL_OPEN"), "1") == 0;
+	bool fail_open = Firegex::NfQueue::nfqueue_fail_open();
 
 	regex_config.reset(new RegexRules(stream_mode));
 
@@ -85,7 +88,18 @@ int main(int argc, char *argv[]){
 	cerr << "[info] [main] Queue: " << queue_manager.queue_num() << " threads assigned: " << n_of_threads << " stream mode: " << stream_mode << " fail open: " << fail_open << endl;
 
 	thread qthr([&](){
-		queue_manager.start();
+		/*  An exception escaping a std::thread is std::terminate(): exit
+		    explicitly instead, so the backend sees a failed process it can
+		    react to rather than an abort. */
+		try{
+			queue_manager.start();
+		}catch(const std::exception& e){
+			cerr << "[fatal] [main] Queue loop stopped: " << e.what() << endl;
+			exit(EXIT_FAILURE);
+		}catch(...){
+			cerr << "[fatal] [main] Queue loop stopped with an unknown error" << endl;
+			exit(EXIT_FAILURE);
+		}
 	});
 	config_updater();
 	qthr.join();

backend/binsrc/pyfilter.cpp

@@ -95,13 +95,31 @@ Final note: is not raccomanded to use variables that starts with __firegex_ in y
 
 
 
+/*  Upper bound for a filter code blob. Without it a desynchronised or corrupt
+    length prefix makes us try to allocate an arbitrary amount of memory. */
+const uint32_t MAX_PYFILTER_CODE_SIZE = 64 * 1024 * 1024;
+
 void config_updater (){
 	while (true){
 		PyThreadState* state = PyEval_SaveThread(); // Release GIL while doing IO operation
 		uint32_t code_size;
-		memcpy(&code_size, control_socket.recv(4).c_str(), 4);
+		string code;
-		code_size = be32toh(code_size);
+		try{
-		string code = control_socket.recv(code_size);
+			string raw_code_size = control_socket.recv(4);
+			memcpy(&code_size, raw_code_size.c_str(), 4);
+			code_size = be32toh(code_size);
+			if (code_size > MAX_PYFILTER_CODE_SIZE){
+				throw runtime_error("Announced filter code size is out of range");
+			}
+			code = control_socket.recv(code_size);
+		}catch(const std::exception& e){
+			// The control socket is the only link with the backend: if it is
+			// gone there is nothing left to filter for, so exit cleanly instead
+			// of letting the exception terminate the process.
+			PyEval_AcquireThread(state);
+			cerr << "[fatal] [updater] Control socket error: " << e.what() << endl;
+			exit(EXIT_FAILURE);
+		}
 		#ifdef DEBUG
 		cerr << "[DEBUG] [updater] Received code: " << code << endl;
 		#endif
@@ -114,6 +132,9 @@ void config_updater (){
 		}catch(const std::exception& e){
 			cerr << "[error] [updater] Failed to build new configuration!" << endl;
 			control_socket << "ACK FAIL " << e.what() << endl;
+		}catch(...){
+			cerr << "[error] [updater] Failed to build new configuration!" << endl;
+			control_socket << "ACK FAIL unknown error" << endl;
 		}
 	}
 }
@@ -143,7 +164,18 @@ int main(int argc, char *argv[]) {
 	cerr << "[info] [main] Queue: " << queue.queue_num() << " threads assigned: " << n_of_threads << endl;
 
 	thread qthr([&](){
-		queue.start();
+		/*  An exception escaping a std::thread is std::terminate(): exit
+		    explicitly instead, so the backend sees a failed process it can
+		    react to rather than an abort. */
+		try{
+			queue.start();
+		}catch(const std::exception& e){
+			cerr << "[fatal] [main] Queue loop stopped: " << e.what() << endl;
+			exit(EXIT_FAILURE);
+		}catch(...){
+			cerr << "[fatal] [main] Queue loop stopped with an unknown error" << endl;
+			exit(EXIT_FAILURE);
+		}
 	});
 	config_updater();
 	qthr.join();

backend/binsrc/regex/regex_rules.cpp

@@ -80,7 +80,7 @@ class RegexRules{
 
 	private:
 		static inline uint16_t glob_seq = 0;
-		uint16_t version;
+		uint16_t version = 0;
 		vector<pair<string, decoded_regex>> decoded_input_rules;
 		vector<pair<string, decoded_regex>> decoded_output_rules;
 		bool is_stream = true;
@@ -104,7 +104,7 @@ class RegexRules{
 			vector<const char*> regex_match_rules(n_of_regex);
 			vector<unsigned int> regex_array_ids(n_of_regex);
 			vector<unsigned int> regex_flags(n_of_regex);
-			for(int i = 0; i < n_of_regex; i++){
+			for(size_t i = 0; i < n_of_regex; i++){
 				regex_match_rules[i] = decoded[i].second.regex.c_str();
 				regex_array_ids[i] = i;
 				regex_flags[i] = HS_FLAG_SINGLEMATCH | HS_FLAG_ALLOWEMPTY;
@@ -114,7 +114,7 @@ class RegexRules{
 			}
 			#ifdef DEBUG
 			cerr << "[DEBUG] [RegexRules.fill_ruleset] compiling " << n_of_regex << " regexes..." << endl;
-			for (int i = 0; i < n_of_regex; i++){
+			for (size_t i = 0; i < n_of_regex; i++){
 				cerr << "[DEBUG] [RegexRules.fill_ruleset] regex[" << i << "]: " << decoded[i].first << " " << decoded[i].second.regex << endl;
 				cerr << "[DEBUG] [RegexRules.fill_ruleset] regex_match_rules[" << i << "]: " << regex_match_rules[i] << endl;
 				cerr << "[DEBUG] [RegexRules.fill_ruleset] regex_flags[" << i << "]: " << regex_flags[i] << endl;
@@ -139,7 +139,7 @@ class RegexRules{
 			}
 			ruleset.hs_db = rebuilt_db;
 			ruleset.regexes = vector<string>(n_of_regex);
-			for(int i = 0; i < n_of_regex; i++){
+			for(size_t i = 0; i < n_of_regex; i++){
 				ruleset.regexes[i] = decoded[i].first;
 			}
 		}
@@ -147,24 +147,25 @@ class RegexRules{
 	public:
 		RegexRules(vector<string> raw_rules, bool is_stream){
 			this->is_stream = is_stream;
+			/*  `throw current_exception()` used to be used here: that throws a
+			    std::exception_ptr *object*, which does not derive from
+			    std::exception, so the `catch(const std::exception&)` in the
+			    config updater never matched it and an invalid regex terminated
+			    the whole process. A bare `throw;` rethrows the original. */
 			for(string ele : raw_rules){
-				try{
+				decoded_regex rule = decode_regex(ele);
-					decoded_regex rule = decode_regex(ele);
+				if (rule.direction == FilterDirection::CTOS){
-					if (rule.direction == FilterDirection::CTOS){
+					decoded_input_rules.push_back(make_pair(ele, rule));
-						decoded_input_rules.push_back(make_pair(ele, rule));
+				}else{
-					}else{
+					decoded_output_rules.push_back(make_pair(ele, rule));
-						decoded_output_rules.push_back(make_pair(ele, rule));
-					}
-				}catch(...){
-					throw current_exception();
 				}
 			}
-			fill_ruleset(decoded_input_rules, input_ruleset);
 			try{
+				fill_ruleset(decoded_input_rules, input_ruleset);
 				fill_ruleset(decoded_output_rules, output_ruleset);
 			}catch(...){
 				free_dbs();
-				throw current_exception();
+				throw;
 			}
 			this->version = ++glob_seq; // 0 version is the null version
 		}
@@ -173,10 +174,12 @@ class RegexRules{
 			return version;
 		}
 
-		RegexRules(bool is_stream){
+		/*  These two have to *delegate*: writing `RegexRules(...)` as a plain
-			vector<string> no_rules;
+		    statement in the body just builds a temporary and throws it away,
-			RegexRules(no_rules, is_stream);
+		    leaving this instance with an uninitialised version and with the
-		}
+		    is_stream argument silently ignored (which is why MATCH_MODE=block,
+		    used by every udp service, never took effect). */
+		RegexRules(bool is_stream): RegexRules(vector<string>(), is_stream) {}
 
 		bool stream_mode(){
 			return is_stream;
@@ -184,9 +187,7 @@ class RegexRules{
 
 
 
-		RegexRules(){
+		RegexRules(): RegexRules(true) {}
-			RegexRules(true);
-		}
 		
 		~RegexRules(){
 			free_dbs();

backend/binsrc/utils.cpp

@@ -76,19 +76,42 @@ public:
         return *this;
     }
 
+    // Stream sockets are free to accept fewer bytes than asked for, so a single
+    // write() can silently truncate a message and desynchronise the protocol.
     void send(const std::string& data) {
-        if (::write(sockfd, data.c_str(), data.size()) == -1) {
+        size_t sent = 0;
-            throw std::runtime_error(std::string("write error: ") + std::strerror(errno));
+        while (sent < data.size()) {
+            ssize_t written = ::write(sockfd, data.c_str() + sent, data.size() - sent);
+            if (written < 0) {
+                if (errno == EINTR) continue;
+                throw std::runtime_error(std::string("write error: ") + std::strerror(errno));
+            }
+            if (written == 0) {
+                throw std::runtime_error("write error: connection closed by peer");
+            }
+            sent += static_cast<size_t>(written);
         }
     }
 
+    /*  Reads exactly `size` bytes. The previous single read() returned whatever
+        happened to be buffered, which for the nfproxy control protocol meant a
+        short read on the 4 byte length prefix (reading past the end of the
+        returned string) or on the filter code itself, leaving the leftover
+        bytes to be parsed as the next length prefix. */
     std::string recv(size_t size) {
         std::string buffer(size, '\0');
-        ssize_t bytesRead = ::read(sockfd, &buffer[0], size);
+        size_t total = 0;
-        if (bytesRead <= 0) {
+        while (total < size) {
-            throw std::runtime_error(std::string("read error: ") + std::strerror(errno));
+            ssize_t bytesRead = ::read(sockfd, &buffer[total], size - total);
+            if (bytesRead < 0) {
+                if (errno == EINTR) continue;
+                throw std::runtime_error(std::string("read error: ") + std::strerror(errno));
+            }
+            if (bytesRead == 0) {
+                throw std::runtime_error("read error: connection closed by peer");
+            }
+            total += static_cast<size_t>(bytesRead);
         }
-        buffer.resize(bytesRead);  // resize to actual bytes read
         return buffer;
     }
 
@@ -101,22 +124,21 @@ public:
 
     // Overload for manipulators (e.g., std::endl)
     UnixClientConnection& operator<<(std::ostream& (*manip)(std::ostream&)) {
-        // Check if the manipulator is std::endl (or equivalent flush)
+        // The flush test used to be `if (static_cast<...>(std::flush))`, i.e. the
-        if (manip == static_cast<std::ostream& (*)(std::ostream&)>(std::endl)){
+        // address of a function, which is always true: every manipulator flushed
+        // and std::endl flushed twice.
+        if (manip == static_cast<std::ostream& (*)(std::ostream&)>(std::endl)) {
             streamBuffer << '\n';  // Add a newline
-            std::string packet = streamBuffer.str();
+        } else if (manip != static_cast<std::ostream& (*)(std::ostream&)>(std::flush)) {
-            streamBuffer.str("");  // Clear the buffer
-            // Send the accumulated data as one packet
-            send(packet);
-        }
-        if (static_cast<std::ostream& (*)(std::ostream&)>(std::flush)) {
-            std::string packet = streamBuffer.str();
-            streamBuffer.str("");  // Clear the buffer
-            // Send the accumulated data as one packet
-            send(packet);
-        } else {
             // For other manipulators, simply pass them to the buffer
             streamBuffer << manip;
+            return *this;
+        }
+        std::string packet = streamBuffer.str();
+        streamBuffer.str("");  // Clear the buffer
+        // Send the accumulated data as one packet
+        if (!packet.empty()) {
+            send(packet);
         }
         return *this;
     }
@@ -174,7 +196,10 @@ private:
     std::queue<T> private_std_queue;
     std::condition_variable condNotEmpty;
     std::condition_variable condNotFull;
-    size_t count; // Guard with Mutex
+    // Must be initialised: an indeterminate value either disables the backpressure
+    // entirely (the queue then grows until the process is OOM killed) or, if it
+    // happens to equal MAX, blocks the netlink thread on the very first packet.
+    size_t count = 0; // Guard with Mutex
 public:
 
     void put(T new_value)

backend/modules/services/firewall.py

@@ -35,6 +35,17 @@ nft = FiregexTables()
 #: The user's Python, one file per filter.
 CODE_DIR = "db/service_filters"
 
+#: How many times a datapath may be brought back before it is declared broken, and how
+#: long without a crash puts the counter back to zero.
+#:
+#: Bounded, because the alternative to a budget is a restart loop: a binary that dies on
+#: the first packet of every attempt would be restarted forever, and each attempt tears
+#: the rules down and builds them again. Spending the budget stops the service outright,
+#: which is the honest end state — the rules come off and the interface says stopped,
+#: instead of a service that reports itself active and is filtering nothing.
+MAX_RESTART_ATTEMPTS = 5
+CRASH_COUNTER_RESET = 60
+
 #: How long between two log lines about the connection limit. Being at the limit means
 #: connections are arriving faster than they leave, so a line each would be the flood
 #: arriving twice — once at the service and once in the operator's log.
@@ -95,6 +106,10 @@ class ServiceManager:
         #: filter, or the port the proxy is listening on. Kept so an address added later
         #: can be steered at the same datapath instead of restarting it.
         self._steer: dict = {}
+        #: How many times this service's datapath has died on its own recently, and
+        #: when the last one was.
+        self._crash_count = 0
+        self._last_crash = 0.0
         #: Carried between lines so a multi-line traceback keeps its severity.
         self._in_traceback = False
         self._engine_level = LEVEL.INFO
@@ -278,6 +293,76 @@ class ServiceManager:
                 level = LEVEL.OUTPUT
             self.log.add(level, stripped)
 
+    def _on_datapath_died(self, what: str, returncode: int) -> None:
+        """A datapath process is gone and nobody asked it to go.
+
+        Scheduled rather than awaited: this is called from the task that was watching the
+        process, and the recovery below stops and starts the service — which cancels that
+        very task. Doing it inline would have the watchdog cancel itself half way through.
+        """
+        #: A bare `create_task` swallows whatever the coroutine raises, and the first
+        #: version of this recovery did exactly that: a wrong constant name meant the
+        #: restart never ran, while the line reporting the crash still appeared — a
+        #: watchdog that says the right thing and does nothing. Anything that goes wrong
+        #: in here is the operator's business too.
+        task = asyncio.create_task(self._recover_datapath(what, returncode))
+
+        def _complain(done: asyncio.Task):
+            if done.cancelled():
+                return
+            if done.exception():
+                self.log.add(
+                    LEVEL.ERROR,
+                    f"could not recover the datapath: {done.exception()!r}",
+                )
+
+        task.add_done_callback(_complain)
+
+    async def _recover_datapath(self, what: str, returncode: int) -> None:
+        """Bring the datapath back, a bounded number of times.
+
+        **The nftables rules outlive the process**, which is what makes this worth doing
+        at all: when the binary dies the rules stay, so traffic keeps being steered at a
+        queue nobody is reading and the interface goes on saying the service is active.
+        What actually happens to that traffic is then decided by the `bypass` flag alone —
+        accepted where the service is fail-open, dropped where it is not — and either way
+        nothing filters it and nothing says so.
+
+        Restarting rebuilds the processes *and* their rules. It is attempted a bounded
+        number of times so that a binary dying on the first packet of every attempt does
+        not become a restart loop; when the budget is spent the service is stopped for
+        good, which takes the rules off and makes the failure visible.
+        """
+        if not self.active or self.transport is None:
+            return  # already being stopped on purpose
+        now = time.monotonic()
+        if now - self._last_crash > CRASH_COUNTER_RESET:
+            self._crash_count = 0
+        self._last_crash = now
+        self._crash_count += 1
+        self.log.add(
+            LEVEL.ERROR,
+            f"{what} exited on its own with code {returncode} — its rules are still in "
+            f"place, so nothing was filtering this service",
+        )
+        if self._crash_count > MAX_RESTART_ATTEMPTS:
+            self.log.add(
+                LEVEL.ERROR,
+                f"it has done that {self._crash_count} times; stopping the service "
+                f"instead of restarting it again",
+            )
+            await self.disable()
+            return
+        self.log.add(
+            LEVEL.WARN,
+            f"restarting it ({self._crash_count}/{MAX_RESTART_ATTEMPTS})",
+        )
+        try:
+            await self.restart()
+        except Exception as e:
+            self.log.add(LEVEL.ERROR, f"could not restart it: {e}")
+            await self.disable()
+
     def _on_over_limit(self, refused: int) -> None:
         """The connection limit turned something away. Say so, and keep the count.
 
@@ -323,6 +408,7 @@ class ServiceManager:
                 on_exception=self._on_exception,
                 on_engine=self._on_engine,
                 on_over_limit=self._on_over_limit,
+                on_died=self._on_datapath_died,
             )
             try:
                 steer = await self.transport.start(self.chain())
@@ -495,7 +581,7 @@ class ServiceManager:
                                     await self.transport.withdraw((host, addr.port))
                                 except Exception as e:
                                     self.log.add(
-                                        LEVEL.WARNING,
+                                        LEVEL.WARN,
                                         f"could not withdraw {addr.ip_int}: {e}")
             if self.active:
                 self.log.add(LEVEL.INFO, f"no longer protecting {gone[0].ip_int}:{gone[0].port}")

backend/modules/services/nftables.py

@@ -556,7 +556,19 @@ class FiregexTables(NFTableManager):
             )
         cmds = []
         for position, queue_num in enumerate(queue_nums):
-            queue = {"queue": {"num": str(queue_num), "flags": ["bypass"]}}
+            # **`bypass` is the service's own fail-open policy, not a constant.** It tells
+            # the kernel to accept a packet when nothing is bound to the queue, which is
+            # exactly what happens when the interceptor dies — and it was set on every
+            # rule regardless. So a service with fail_open *off*, whose whole point is
+            # that traffic stops rather than passes when filtering stops, went wide open
+            # the moment its binary crashed: the one case the setting exists for. The
+            # other half of the same policy is `FIREGEX_NFQUEUE_FAIL_OPEN`, which decides
+            # what the process does while it is alive; the two have to agree or the
+            # answer depends on how the filtering stopped.
+            queue = {"queue": {
+                "num": str(queue_num),
+                **({"flags": ["bypass"]} if srv.fail_open else {}),
+            }}
             cmds.append(self._rule(
                 self.queue_output_chain(position),
                 self._match(ip, port, family, l4, "saddr", "sport")

backend/modules/services/transports.py

@@ -116,7 +116,7 @@ class Transport:
     """What a network layer has to be able to do."""
 
     def __init__(self, srv, on_block=None, on_output=None, on_exception=None, on_engine=None,
-                 on_over_limit=None):
+                 on_over_limit=None, on_died=None):
         self.srv = srv
         # Called with the id of whatever refused a connection — a regex rule id, or a
         # filter id for a pyfilter. Counters live in the database, which is the
@@ -136,6 +136,12 @@ class Transport:
         #: last report. The engine's own counter resets when it restarts, so somebody
         #: above has to add them up if the trace is to outlive the process.
         self.on_over_limit = on_over_limit
+        #: Called when a datapath process dies without being asked to. **The nftables
+        #: rules outlive the process**, so nobody notices on their own: the traffic keeps
+        #: being steered at a queue nobody is reading, the interface goes on reporting the
+        #: service as active, and what it is doing is decided by the `bypass` flag alone.
+        #: Whoever is above decides whether to bring it back.
+        self.on_died = on_died
         #: Whether whatever this layer listens on can accept IPv6. Declared here rather
         #: than on the one layer that has a listener, so a caller asking a transport
         #: what it can accept gets an answer instead of an AttributeError.
@@ -278,15 +284,36 @@ class _QueueStage:
         self._sock_ready = asyncio.Event()
         #: Which rule id each filter code stands for, for block attribution.
         self._codes: dict[str, str] = {}
+        #: Set by `stop()`, so the watchdog can tell a shutdown we asked for from one we
+        #: did not. Without the distinction every ordinary stop looks like a crash.
+        self._stopped = False
+        self._watchdog: asyncio.Task | None = None
 
     async def start(self) -> int:
         if self.link.kind == KIND.PYFILTER:
             self.queue_num = await self._start_cpproxy()
         else:
             self.queue_num = await self._start_cppregex()
+        self._watchdog = asyncio.create_task(self._watch())
         await self.reload(self.link)
         return self.queue_num
 
+    async def _watch(self):
+        """Notice the binary dying, which nothing else here does.
+
+        The reader task sees EOF on stdout and returns, which is indistinguishable from a
+        clean shutdown, so a crashed interceptor used to leave a service that reports
+        itself as active and filters nothing — its rules still pointing traffic at a queue
+        with no reader behind it.
+        """
+        try:
+            returncode = await self.process.wait()
+        except asyncio.CancelledError:
+            return
+        if self._stopped:
+            return  # we killed it ourselves
+        self.owner._stage_died(self.link.filter.name, returncode)
+
     # --- cppregex: config on stdin, events on stdout --------------------------
 
     async def _start_cppregex(self) -> int:
@@ -496,6 +523,12 @@ class _QueueStage:
             raise Exception(f"the nfqueue binary rejected the filters: {detail}")
 
     async def stop(self) -> None:
+        # Said before anything is torn down, or the watchdog wakes on the kill below and
+        # reports a crash we asked for.
+        self._stopped = True
+        if self._watchdog and self._watchdog is not asyncio.current_task():
+            self._watchdog.cancel()
+        self._watchdog = None
         for task in (self._reader_task, self._output_task):
             if task:
                 task.cancel()
@@ -588,6 +621,10 @@ class NfqueueTransport(Transport):
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
         self.stages: list[_QueueStage] = []
+        #: One report per datapath, not one per process: a chain is several binaries and
+        #: they usually die together, which would otherwise ask for four restarts of one
+        #: service.
+        self._reported_death = False
 
     async def start(self, chain: list[ChainLink]) -> dict:
         self.check(self.srv, chain)
@@ -623,6 +660,18 @@ class NfqueueTransport(Transport):
             await stage.stop()
         self.stages = []
 
+    def _stage_died(self, filter_name: str, returncode: int):
+        """One of the chain's processes is gone and nobody asked it to go.
+
+        Reported once per shutdown: the stages share a service, so a chain of four that
+        dies together would otherwise ask for four restarts of the same thing.
+        """
+        if self._reported_death:
+            return
+        self._reported_death = True
+        if self.on_died:
+            self.on_died(f"the {filter_name} interceptor", returncode)
+
 
 class _EmptyFilter:
     """Stands in for a service with nothing active, so it still has a datapath."""