rules: a rule set is not 64 kilobytes long

fmtrules formatted into a 64K buffer with seprint, which clamps, and
returned how much it had written.  Nothing looked at whether that was
everything.  Since prepend, append and delete all work by formatting
the whole set out, editing the text and parsing it back -- deliberately,
so that a rule typed at ctl and a rule in a file go through one parser
-- editing a set past the limit did not truncate the display, it
truncated the rules.

Measured with 2000 rules, about 104K formatted:

	and all of it comes back    want: 2000  got: 1214
	and survives an edit        want: ok    got: refused
	with nothing lost off the end            got: 1214

786 rules gone from the running firewall, and the only sign is that the
edit after it failed.  save wrote the same short file, so reload would
then have made the loss permanent.

Now sized and allocated to fit.  The bound is per rule -- the fixed
attributes at their longest, plus the protocol, which is the only part
whose length is ndb's choice rather than ours -- summed under the same
lock that formats, so an install cannot get between the two passes.
flows had the identical cap and gets the identical fix; on a busy
firewall it is the file most likely to reach it.  Rulebuf is gone.

Six checks: a 2000-rule set loads, comes back whole, survives an edit,
and saves whole.

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

4 files changed, 103 insertions, 37 deletions

diff --git a/fw/src/fw.c b/fw/src/fw.c
index 3c13182..ef4aff0 100644
--- a/fw/src/fw.c
+++ b/fw/src/fw.c
@@ -508,8 +508,6 @@ static char *ctltext =
 	"changes take effect at once, and live connections that the\n"
 	"new rules forbid are dropped rather than left running.\n";
 
-enum { Rulebuf = 64*1024 };
-
 /*
  * ctl edits are done by writing the rule set back out as ndb, editing
  * the text, and parsing the whole thing again.  It is not the quickest
@@ -562,12 +560,11 @@ editrules(char *add, int atfront, int delete)
 	char *cur, *all, *err, *p, *nl;
 	int i;
 
-	if((cur = mallocz(Rulebuf, 1)) == nil)
+	if((cur = rulestext()) == nil)
 		return "out of memory";
-	cur[fmtrules(cur, Rulebuf-1)] = '\0';
 
 	if(delete > 0){
-		/* fmtrules writes one line per rule, so line n is rule n */
+		/* rulestext writes one line per rule, so line n is rule n */
 		p = cur;
 		for(i = 1; i < delete && p != nil; i++)
 			if((p = strchr(p, '\n')) != nil)
@@ -618,9 +615,9 @@ saverules(char *file)
 
 	if(file == nil)
 		return "no rule file to save to";
-	if((buf = mallocz(Rulebuf, 1)) == nil)
+	if((buf = rulestext()) == nil)
 		return "out of memory";
-	n = fmtrules(buf, Rulebuf-1);
+	n = strlen(buf);
 	if((fd = create(file, OWRITE, 0644)) < 0){
 		free(buf);
 		return "cannot create the rule file";
@@ -653,20 +650,27 @@ flushflows(void)
 	return nil;
 }
 
+/* allocated to fit, for the reason rulestext is */
 static char*
 flowtext(void)
 {
 	char *buf, *p, *e;
 	Flow *f;
-	long now;
+	long now, sz;
 	int i;
 
-	if((buf = mallocz(Rulebuf, 1)) == nil)
-		return nil;
-	p = buf;
-	e = buf + Rulebuf;
 	now = time(0);
 	lock(&flowlock);
+	sz = 1;
+	for(i = 0; i < Nflow; i++)
+		for(f = flowtab[i]; f != nil; f = f->next)
+			sz += 160;
+	if((buf = malloc(sz)) == nil){
+		unlock(&flowlock);
+		return nil;
+	}
+	p = buf;
+	e = buf + sz;
 	for(i = 0; i < Nflow; i++)
 		for(f = flowtab[i]; f != nil; f = f->next)
 			p = seprint(p, e, "%s %s %I!%d -> %I!%d idle %ld\n",
@@ -738,20 +742,17 @@ fsdestroyfid(Fid *fid)
 static void
 fsread(Req *r)
 {
-	char *s;
-	long n;
+	char *s, *t;
 
 	switch((int)(uintptr)r->fid->file->aux){
 	case Qctl:
 		readstr(r, ctltext);
 		break;
 	case Qrules:
-		if((s = mallocz(Rulebuf, 1)) == nil){
+		if((s = rulestext()) == nil){
 			respond(r, "out of memory");
 			return;
 		}
-		n = fmtrules(s, Rulebuf-1);
-		s[n] = '\0';
 		readstr(r, s);
 		free(s);
 		break;
@@ -764,14 +765,18 @@ fsread(Req *r)
 		free(s);
 		break;
 	case Qstats:
-		if((s = mallocz(Rulebuf, 1)) == nil){
+		if((s = hitstext()) == nil){
+			respond(r, "out of memory");
+			return;
+		}
+		if((t = smprint("passed %d\ndropped %d\n\n%s", nallow, ndeny, s)) == nil){
+			free(s);
 			respond(r, "out of memory");
 			return;
 		}
-		n = snprint(s, Rulebuf-1, "passed %d\ndropped %d\n\n", nallow, ndeny);
-		fmthits(s+n, Rulebuf-1-n);
-		readstr(r, s);
 		free(s);
+		readstr(r, t);
+		free(t);
 		break;
 	default:
 		respond(r, "not a readable file");
@@ -1259,7 +1264,7 @@ threadmain(int argc, char **argv)
 	fmtinstall('I', eipfmt);
 	fmtinstall('V', eipfmt);
 	fmtinstall('E', eipfmt);
-	fmtinstall('M', eipfmt);	/* fmtrules prints masks with it */
+	fmtinstall('M', eipfmt);	/* rulestext prints masks with it */
 
 	rulepath = argv[0];
 	readrules(rulepath);
diff --git a/fw/src/rules.c b/fw/src/rules.c
index b5992b0..82a31a5 100644
--- a/fw/src/rules.c
+++ b/fw/src/rules.c
@@ -302,16 +302,47 @@ readrules(char *file)
 /*
  * The current set, written back out as ndb.  What comes out here must
  * parse back in unchanged; it is what gets persisted.
+ *
+ * Allocated to fit, not written into 64K and clipped.  A rule set that
+ * outgrew the buffer used to come back short, and since prepend, append
+ * and delete all work by formatting the set out, editing the text and
+ * parsing it again, editing one rule past the limit deleted every rule
+ * after it.  Silently.
  */
-long
-fmtrules(char *buf, long nbuf)
+static long
+rulesize(Rule *r)
+{
+	long n;
+
+	/*
+	 * "allow=out" and the fixed part of every attribute, an address
+	 * and a mask at their longest, two ports, log=yes, the tabs and
+	 * the newline.  Only the protocol is unbounded, and it is ndb's
+	 * word rather than anything we choose.
+	 */
+	n = 160;
+	if(r->proto != nil)
+		n += strlen(r->proto);
+	return n;
+}
+
+char*
+rulestext(void)
 {
-	char *p, *e;
+	char *buf, *p, *e;
+	long sz;
 	Rule *r;
 
-	p = buf;
-	e = buf + nbuf;
 	lock(&rulelock);
+	sz = 1;
+	for(r = rules; r != nil; r = r->next)
+		sz += rulesize(r);
+	if((buf = malloc(sz)) == nil){
+		unlock(&rulelock);
+		return nil;
+	}
+	p = buf;
+	e = buf + sz;
 	for(r = rules; r != nil; r = r->next){
 		p = seprint(p, e, "%s=%s", r->allow ? "allow" : "deny",
 			r->verb == Vin ? "in" : r->verb == Vout ? "out" : "*");
@@ -328,7 +359,8 @@ fmtrules(char *buf, long nbuf)
 		p = seprint(p, e, "\n");
 	}
 	unlock(&rulelock);
-	return p - buf;
+	USED(p);
+	return buf;
 }
 
 /*
@@ -375,18 +407,26 @@ dumprules(void)
  * The rules with a count of how often each has decided something.  A
  * rule that has never fired is either dead or protecting you from
  * something that has not happened yet, and it is worth being able to
- * tell which.  Kept out of fmtrules so that what "rules" prints stays
+ * tell which.  Kept out of rulestext so that what "rules" prints stays
  * a rule set that can be written straight back.
  */
-long
-fmthits(char *buf, long nbuf)
+char*
+hitstext(void)
 {
-	char *p, *e;
+	char *buf, *p, *e;
+	long sz;
 	Rule *r;
 
-	p = buf;
-	e = buf + nbuf;
 	lock(&rulelock);
+	sz = 1;
+	for(r = rules; r != nil; r = r->next)
+		sz += 64 + (r->proto != nil ? strlen(r->proto) : 0);
+	if((buf = malloc(sz)) == nil){
+		unlock(&rulelock);
+		return nil;
+	}
+	p = buf;
+	e = buf + sz;
 	for(r = rules; r != nil; r = r->next)
 		p = seprint(p, e, "%-8ld %s=%s%s%s\n", r->hits,
 			r->allow ? "allow" : "deny",
@@ -394,7 +434,8 @@ fmthits(char *buf, long nbuf)
 			r->proto != nil ? "\tproto=" : "",
 			r->proto != nil ? r->proto : "");
 	unlock(&rulelock);
-	return p - buf;
+	USED(p);
+	return buf;
 }
 
 int
diff --git a/fw/src/rules.h b/fw/src/rules.h
index f2c38d4..7d7fe1b 100644
--- a/fw/src/rules.h
+++ b/fw/src/rules.h
@@ -45,8 +45,8 @@ void	readrules(char*);		/* parse and install, fatal on error */
 Rule*	parserules(char*, char**);	/* parse only; nil + reason on error */
 void	installrules(Rule*);		/* swap in, then call rulechanged */
 void	freerules(Rule*);
-long	fmtrules(char*, long);		/* current set, back in ndb form */
-long	fmthits(char*, long);		/* the same, with hit counts */
+char*	rulestext(void);		/* current set as ndb; free it */
+char*	hitstext(void);			/* the same, with hit counts */
 void	dumprules(void);
 void	checklogging(void);
 /*
diff --git a/fw/test/fwtest.rc b/fw/test/fwtest.rc
index f687295..15ea531 100755
--- a/fw/test/fwtest.rc
+++ b/fw/test/fwtest.rc
@@ -326,6 +326,26 @@ deny=*		log=yes
 	r=`{wr $mtpt/ctl 'delete 0'}
 	check 'delete 0 is refused' refused $"r
 
+	# A rule set used to be formatted into 64K and clipped, and since
+	# every ctl edit works by formatting the set out, editing the text
+	# and parsing it back, editing one rule past the limit deleted
+	# every rule after it.  This set is about 104K.
+	awk 'BEGIN{for(i = 0; i < 2000; i++)
+		printf "allow=out\tproto=tcp\tip=10.9.0.0/24\tport=%d\n", 1000+i}' \
+		> $tmp/big.ndb
+	r=`{wr $mtpt/ctl 'reload '^$tmp/big.ndb}
+	check 'a rule set larger than 64K loads' ok $"r
+	r=`{grep -c . $mtpt/rules}
+	check 'and all of it comes back' 2000 $"r
+	r=`{wr $mtpt/ctl 'append deny=out proto=udp'}
+	check 'and survives an edit' ok $"r
+	r=`{grep -c . $mtpt/rules}
+	check 'with nothing lost off the end' 2001 $"r
+	r=`{wr $mtpt/ctl 'save '^$tmp/saved.ndb}
+	check 'and saves' ok $"r
+	r=`{grep -c . $tmp/saved.ndb}
+	check 'all of itself' 2001 $"r
+
 	stopfw $nA $nB
 }