rules: matchrule stops returning things whose lifetime it does not own

Three findings, one interface.  matchrule handed back a Rule* and a
pointer into a static char[128], both read by the caller after it had
released rulelock:

	e = matchrule(verb, ..., &rule);
	if(rule != nil && rule->log)		/* freed? */
		syslog(0, "fw", "... %s", e);	/* whose? */

A rule set installed between the return and those two lines frees the
Rule under them, which is a narrow window but this is a firewall, and
two procs deciding at once overwrite each other's reason -- in a program
whose entire output is the reason.  netfs.c ran multi-proc from the
first blocking open and had its own static err with the same problem.
Neither is a race you can test for; both stop existing if the answer
lives in the caller's frame, so it does.  Seven positional arguments
become named fields while the signature is being rewritten anyway.

The third is that revalidate could not ask without being counted.  A
rule edit rebuilds the set, so every hit count starts at zero, and then
revalidate re-checks each live flow against the new rules and charged
every one of them to the rule that matched.  So a rule that had decided
nothing since the edit reported one decision per live connection, and
stats answered a different question after every edit.  count says
whether this is traffic.

Also: the log said "deny tcp connect 127.0.0.2" for a connection to a
port it never named.  getfields writes over the separators it splits
on, so f[1] afterwards is only what precedes the first "!".  The
address is copied before it is taken apart.

Four checks.  Two exercise the log path end to end, denied and
permitted, matching the full address and the rule number in
/sys/log/fw -- which they create if it is missing and remove again if
they made it.  One reads the hit count after an edit: with count put
back to 1 in revalidate it reports 1 where 0 is right.  51 pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

4 files changed, 143 insertions, 89 deletions

diff --git a/fw/src/fw.c b/fw/src/fw.c
index 70bcc5c..3c13182 100644
--- a/fw/src/fw.c
+++ b/fw/src/fw.c
@@ -190,23 +190,27 @@ int
 revalidate(void)
 {
 	Flow *f, **pp;
-	uchar *peer;
-	int i, port, lport, n;
+	Match m;
+	int i, n;
 
 	n = 0;
+	memset(&m, 0, sizeof m);
+	m.count = 0;		/* re-checking a flow is not traffic */
 	lock(&flowlock);
 	for(i = 0; i < Nflow; i++)
 		for(pp = &flowtab[i]; (f = *pp) != nil; ){
+			m.verb = f->verb;
+			m.proto = protonum2name(f->proto);
 			if(f->verb == Vout){
-				peer = f->dst;
-				port = f->dport;
-				lport = f->sport;
+				m.ip = f->dst;
+				m.port = f->dport;
+				m.lport = f->sport;
 			}else{
-				peer = f->src;
-				port = f->sport;
-				lport = f->dport;
+				m.ip = f->src;
+				m.port = f->sport;
+				m.lport = f->dport;
 			}
-			if(matchrule(f->verb, protonum2name(f->proto), peer, 0, port, lport, nil) != nil){
+			if(!matchrule(&m)){
 				*pp = f->next;
 				free(f);
 				n++;
@@ -397,11 +401,9 @@ static void
 relay(Wire *from, Wire *to, int verb)
 {
 	uchar *buf;
-	char *e;
-	uchar *peer;
-	Rule *rule;
+	Match m;
 	Pkt p;
-	int n, port, lport;
+	int n;
 
 	/*
 	 * On the heap, not the stack: these run as libthread procs with a
@@ -424,14 +426,18 @@ relay(Wire *from, Wire *to, int verb)
 		}
 
 		p.verb = verb;
+		memset(&m, 0, sizeof m);
+		m.count = 1;
+		m.verb = verb;
+		m.proto = protonum2name(p.proto);
 		if(verb == Vout){
-			peer = p.dst;
-			port = p.dport;
-			lport = p.sport;
+			m.ip = p.dst;
+			m.port = p.dport;
+			m.lport = p.sport;
 		}else{
-			peer = p.src;
-			port = p.sport;
-			lport = p.dport;
+			m.ip = p.src;
+			m.port = p.sport;
+			m.lport = p.dport;
 		}
 
 		if(flowseen(&p)){
@@ -446,24 +452,23 @@ relay(Wire *from, Wire *to, int verb)
 			continue;
 		}
 
-		e = matchrule(verb, protonum2name(p.proto), peer, 0, port, lport, &rule);
-		if(e != nil){
-			if(rule != nil && rule->log)
+		if(!matchrule(&m)){
+			if(m.log)
 				syslog(0, "fw", "drop %s %s %I!%d -> %I!%d: %s",
 					verb == Vout ? "out" : "in",
 					protonum2name(p.proto),
-					p.src, p.sport, p.dst, p.dport, e);
+					p.src, p.sport, p.dst, p.dport, m.err);
 			ndeny++;
 			if(debug)
 				fprint(2, "drop %s %s %I!%d -> %I!%d: %s\n",
 					verb == Vout ? "out" : "in",
 					protonum2name(p.proto),
-					p.src, p.sport, p.dst, p.dport, e);
+					p.src, p.sport, p.dst, p.dport, m.err);
 			continue;
 		}
 
 		nallow++;
-		if(rule != nil && rule->log)
+		if(m.log)
 			syslog(0, "fw", "pass %s %s %I!%d -> %I!%d",
 				verb == Vout ? "out" : "in",
 				protonum2name(p.proto),
@@ -925,10 +930,7 @@ relayproc(void *a)
 static int
 permitted(uchar *buf, int n, int verb, Pkt *p)
 {
-	Rule *rule;
-	uchar *peer;
-	char *e;
-	int port, lport;
+	Match m;
 
 	parsepkt(buf, n, p);
 	if(!p->ok){
@@ -939,34 +941,37 @@ permitted(uchar *buf, int n, int verb, Pkt *p)
 		return 0;
 	}
 	p->verb = verb;
+	memset(&m, 0, sizeof m);
+	m.count = 1;
+	m.verb = verb;
+	m.proto = protonum2name(p->proto);
 	if(verb == Vout){
-		peer = p->dst;
-		port = p->dport;
-		lport = p->sport;
+		m.ip = p->dst;
+		m.port = p->dport;
+		m.lport = p->sport;
 	}else{
-		peer = p->src;
-		port = p->sport;
-		lport = p->dport;
+		m.ip = p->src;
+		m.port = p->sport;
+		m.lport = p->dport;
 	}
 	if(flowseen(p)){
 		nallow++;
 		return 1;
 	}
-	e = matchrule(verb, protonum2name(p->proto), peer, 0, port, lport, &rule);
-	if(e != nil){
+	if(!matchrule(&m)){
 		ndeny++;
-		if(rule != nil && rule->log)
+		if(m.log)
 			syslog(0, "fw", "drop %s %s %I!%d -> %I!%d: %s",
 				verb == Vout ? "out" : "in", protonum2name(p->proto),
-				p->src, p->sport, p->dst, p->dport, e);
+				p->src, p->sport, p->dst, p->dport, m.err);
 		if(debug)
 			fprint(2, "drop %s %s %I!%d -> %I!%d: %s\n",
 				verb == Vout ? "out" : "in", protonum2name(p->proto),
-				p->src, p->sport, p->dst, p->dport, e);
+				p->src, p->sport, p->dst, p->dport, m.err);
 		return 0;
 	}
 	nallow++;
-	if(rule != nil && rule->log)
+	if(m.log)
 		syslog(0, "fw", "pass %s %s %I!%d -> %I!%d",
 			verb == Vout ? "out" : "in", protonum2name(p->proto),
 			p->src, p->sport, p->dst, p->dport);
diff --git a/fw/src/netfs.c b/fw/src/netfs.c
index f44b089..1300717 100644
--- a/fw/src/netfs.c
+++ b/fw/src/netfs.c
@@ -345,14 +345,16 @@ static char *okverbs[] = {
  * a bare port, or addr!port with addr often "*".
  */
 static char*
-checkctl(char *proto, char *msg, long n)
+checkctl(Match *m, char *proto, char *msg, long n)
 {
-	char buf[512], *f[8], *a[4], *addr, *e;
-	static char err[128];
+	char buf[512], dest[128], *f[8], *a[4], *addr;
 	uchar ip[IPaddrlen], mask[IPaddrlen];
-	Rule *rule;
-	int nf, na, verb, anyip, port, lport, i;
+	int nf, na, i;
 
+	memset(m, 0, sizeof *m);
+	m->count = 1;
+	m->proto = proto;
+	m->ip = ip;
 	if(n <= 0)
 		return nil;
 	if(n >= sizeof buf)
@@ -363,9 +365,9 @@ checkctl(char *proto, char *msg, long n)
 	if((nf = tokenize(buf, f, nelem(f))) < 1)
 		return nil;
 	if(strcmp(f[0], "connect") == 0)
-		verb = Vout;
+		m->verb = Vout;
 	else if(strcmp(f[0], "announce") == 0)
-		verb = Vin;
+		m->verb = Vin;
 	else{
 		/*
 		 * Anything else is refused unless it is known to be
@@ -387,22 +389,29 @@ checkctl(char *proto, char *msg, long n)
 				return nil;
 		syslog(0, "fw", "deny %s %s: control message not permitted",
 			proto, f[0]);
-		snprint(err, sizeof err,
+		snprint(m->err, sizeof m->err,
 			"fw: %s: not a permitted control message", f[0]);
-		return err;
+		return m->err;
 	}
 	if(nf < 2)
 		return nil;			/* malformed; let the kernel say so */
 
+	/*
+	 * Keep the address before splitting it: getfields writes over the
+	 * separators, so f[1] afterwards is only what precedes the first
+	 * one, and the log said "connect 127.0.0.2" for a connection to a
+	 * port it never named.
+	 */
+	snprint(dest, sizeof dest, "%s", f[1]);
 	na = getfields(f[1], a, nelem(a), 0, "!");
 	if(na < 1)
 		return nil;
 	if(na == 1){
 		addr = "*";			/* announce 17019 */
-		port = atoi(a[0]);
+		m->port = atoi(a[0]);
 	}else{
 		addr = a[0];
-		port = strcmp(a[1], "*") == 0 ? -1 : atoi(a[1]);
+		m->port = strcmp(a[1], "*") == 0 ? -1 : atoi(a[1]);
 	}
 
 	/*
@@ -418,27 +427,30 @@ checkctl(char *proto, char *msg, long n)
 	 * peer therefore cannot apply to an announce, which is right:
 	 * at this point there is no peer to name.
 	 */
-	if(verb == Vin){
-		lport = port;
-		port = -1;
-		anyip = 1;
+	if(m->verb == Vin){
+		m->lport = m->port;
+		m->port = -1;
+		m->anyip = 1;
 	}else{
-		lport = -1;
-		anyip = strcmp(addr, "*") == 0;
-		if(!anyip && parseipandmask(ip, mask, addr, nil) == -1){
+		m->lport = -1;
+		m->anyip = strcmp(addr, "*") == 0;
+		if(!m->anyip && parseipandmask(ip, mask, addr, nil) == -1){
 			syslog(0, "fw", "deny %s %s %s: unparseable address",
-				proto, f[0], f[1]);
-			return "fw: unparseable address";
+				proto, f[0], dest);
+			snprint(m->err, sizeof m->err, "fw: unparseable address");
+			return m->err;
 		}
 	}
-	if((e = matchrule(verb, proto, ip, anyip, port, lport, &rule)) != nil){
-		if(rule != nil && rule->log)
-			syslog(0, "fw", "deny %s %s %s: %s", proto, f[0], f[1], e);
-		snprint(err, sizeof err, "fw: %s", e);
-		return err;
+	if(!matchrule(m)){
+		if(m->log)
+			syslog(0, "fw", "deny %s %s %s: %s",
+				proto, f[0], dest, m->err);
+		snprint(buf, sizeof buf, "fw: %s", m->err);
+		snprint(m->err, sizeof m->err, "%s", buf);
+		return m->err;
 	}
-	if(rule != nil && rule->log)
-		syslog(0, "fw", "allow %s %s %s", proto, f[0], f[1]);
+	if(m->log)
+		syslog(0, "fw", "allow %s %s %s", proto, f[0], dest);
 	return nil;
 }
 
@@ -646,10 +658,12 @@ fswrite(Req *r)
 		return;
 	}
 	if((proto = ctlproto(f->path)) != nil){
-		e = checkctl(proto, r->ifcall.data, r->ifcall.count);
+		Match m;
+
+		e = checkctl(&m, proto, r->ifcall.data, r->ifcall.count);
 		free(proto);
 		if(e != nil){
-			respond(r, e);
+			respond(r, e);	/* m outlives the respond, which packs it */
 			return;
 		}
 	}
diff --git a/fw/src/rules.c b/fw/src/rules.c
index 55de9cb..b5992b0 100644
--- a/fw/src/rules.c
+++ b/fw/src/rules.c
@@ -397,44 +397,47 @@ fmthits(char *buf, long nbuf)
 	return p - buf;
 }
 
-char*
-matchrule(int verb, char *proto, uchar *ip, int anyip, int port, int lport, Rule **rp)
+int
+matchrule(Match *m)
 {
 	uchar net[IPaddrlen], rnet[IPaddrlen];
-	static char err[128];
 	Rule *r;
 
-	if(rp != nil)
-		*rp = nil;
+	m->nr = 0;
+	m->log = 0;
 	lock(&rulelock);
 	for(r = rules; r != nil; r = r->next){
-		if(r->verb != Vany && r->verb != verb)
+		if(r->verb != Vany && r->verb != m->verb)
 			continue;
-		if(r->proto != nil && (proto == nil || strcmp(r->proto, proto) != 0))
+		if(r->proto != nil
+		&& (m->proto == nil || strcmp(r->proto, m->proto) != 0))
 			continue;
-		if(r->port >= 0 && r->port != port)
+		if(r->port >= 0 && r->port != m->port)
 			continue;
-		if(r->lport >= 0 && r->lport != lport)
+		if(r->lport >= 0 && r->lport != m->lport)
 			continue;
 		if(!r->anyip){
-			if(anyip)	/* a wildcard request cannot match a specific rule */
+			if(m->anyip)	/* a wildcard request cannot match a specific rule */
 				continue;
-			maskip(ip, r->mask, net);
+			maskip(m->ip, r->mask, net);
 			maskip(r->ip, r->mask, rnet);
 			if(ipcmp(net, rnet) != 0)
 				continue;
 		}
-		if(rp != nil)
-			*rp = r;
-		r->hits++;
+		m->nr = r->nr;
+		m->log = r->log;
+		if(m->count)
+			r->hits++;
 		if(r->allow){
 			unlock(&rulelock);
-			return nil;
+			m->err[0] = '\0';
+			return 1;
 		}
-		snprint(err, sizeof err, "denied by rule %d", r->nr);
 		unlock(&rulelock);
-		return err;
+		snprint(m->err, sizeof m->err, "denied by rule %d", r->nr);
+		return 0;
 	}
 	unlock(&rulelock);
-	return "denied, no rule matched";
+	snprint(m->err, sizeof m->err, "denied, no rule matched");
+	return 0;
 }
diff --git a/fw/src/rules.h b/fw/src/rules.h
index 6aad0ae..f2c38d4 100644
--- a/fw/src/rules.h
+++ b/fw/src/rules.h
@@ -49,7 +49,39 @@ long	fmtrules(char*, long);		/* current set, back in ndb form */
 long	fmthits(char*, long);		/* the same, with hit counts */
 void	dumprules(void);
 void	checklogging(void);
-char*	matchrule(int verb, char *proto, uchar *ip, int anyip, int port, int lport, Rule**);
+/*
+ * One question for the rule list, and its answer, in the caller's
+ * frame.
+ *
+ * matchrule used to hand back a Rule* and a pointer into a static
+ * buffer, and callers read both after it had let go of the lock.  A
+ * rule set installed in between freed the Rule under them, and two
+ * procs deciding at once overwrote each other's reason - in a program
+ * whose entire output is the reason.  Nothing here outlives the Match,
+ * and the Match belongs to whoever asked.
+ *
+ * count says to charge the decision to the rule's tally.  Re-checking
+ * live flows after a rule change is not traffic and must not be
+ * counted, or "how often has this rule decided something" answers a
+ * different question every time the rules are edited.
+ */
+typedef struct Match Match;
+struct Match
+{
+	int	verb;			/* in */
+	char	*proto;			/* nil: any */
+	uchar	*ip;			/* the peer; unread if anyip */
+	int	anyip;
+	int	port;			/* peer port, -1: any */
+	int	lport;			/* local port, -1: any */
+	int	count;			/* charge this to the rule */
+
+	int	nr;			/* out: the rule that decided, 0 if none */
+	int	log;			/* it asked to be logged */
+	char	err[128];		/* why not, if it said no */
+};
+
+int	matchrule(Match*);		/* 1 to permit, 0 to refuse */
 int	protoname2num(char*);
 char*	protonum2name(int);