[PATCH v2 11/23] fwd: Improve error handling in fwd_rule_add()

[David Gibson <david@gibson.dropbear.id.au>]

fwd_rule_add() sanity checks the given rule, however all errors are fatal:
either they're assert()s in the case of things that callers should have
already verified, or die()s if we run out of space for the new rule.

This won't suffice any more when we allow rule updates from a
configuration client.  We don't want to trust the input we get from
the client any more than we have to.

Replace the assert()s and die()s with a return value.  Also include warn()s
so that the user gets a more specific idea of the problem in the logs or
stderr.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c     | 15 ++++++++++++---
 fwd.c      | 41 +++++++++++++++++++++++++++++++----------
 fwd.h      |  2 +-
 fwd_rule.c |  3 +--
 fwd_rule.h |  1 +
 5 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/conf.c b/conf.c
index b871646f..5c913820 100644
--- a/conf.c
+++ b/conf.c
@@ -157,6 +157,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
 		.proto = proto,
 		.flags = flags,
 	};
+	char rulestr[FWD_RULE_STRLEN];
 	unsigned delta = to - first;
 	unsigned base, i;
 
@@ -207,20 +208,28 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
 				rulev.addr = inany_loopback4;
 				fwd_rule_conflict_check(&rulev,
 							fwd->rules, fwd->count);
-				fwd_rule_add(fwd, &rulev);
+				if (fwd_rule_add(fwd, &rulev) < 0)
+					goto fail;
 			}
 			if (c->ifi6) {
 				rulev.addr = inany_loopback6;
 				fwd_rule_conflict_check(&rulev,
 							fwd->rules, fwd->count);
-				fwd_rule_add(fwd, &rulev);
+				if (fwd_rule_add(fwd, &rulev) < 0)
+					goto fail;
 			}
 		} else {
 			fwd_rule_conflict_check(&rule, fwd->rules, fwd->count);
-			fwd_rule_add(fwd, &rule);
+			if (fwd_rule_add(fwd, &rule) < 0)
+				goto fail;
 		}
 		base = i - 1;
 	}
+	return;
+
+fail:
+	die("Unable to add rule %s",
+	    fwd_rule_fmt(&rule, rulestr, sizeof(rulestr)));
 }
 
 /**
diff --git a/fwd.c b/fwd.c
index c9637525..3e87169b 100644
--- a/fwd.c
+++ b/fwd.c
@@ -335,24 +335,44 @@ void fwd_rule_init(struct ctx *c)
  * fwd_rule_add() - Validate and add a rule to a forwarding table
  * @fwd:	Table to add to
  * @new:	Rule to add
+ *
+ * Return: 0 on success, negative error code on failure
  */
-void fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
+int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
 {
 	/* Flags which can be set from the caller */
 	const uint8_t allowed_flags = FWD_WEAK | FWD_SCAN | FWD_DUAL_STACK_ANY;
 	unsigned num = (unsigned)new->last - new->first + 1;
 	unsigned port;
 
-	assert(!(new->flags & ~allowed_flags));
-	/* Passing a non-wildcard address with DUAL_STACK_ANY is a bug */
-	assert(!(new->flags & FWD_DUAL_STACK_ANY) ||
-	       inany_equals(&new->addr, &inany_any6));
-	assert(new->first <= new->last);
+	if (new->first > new->last) {
+		warn("Rule has invalid port range %u-%u",
+		     new->first, new->last);
+		return -EINVAL;
+	}
+	if (new->flags & ~allowed_flags) {
+		warn("Rule has invalid flags 0x%hhx",
+		     new->flags & ~allowed_flags);
+		return -EINVAL;
+	}
+	if (new->flags & FWD_DUAL_STACK_ANY &&
+	    !inany_equals(&new->addr, &inany_any6)) {
+		char astr[INANY_ADDRSTRLEN];
 
-	if (fwd->count >= ARRAY_SIZE(fwd->rules))
-		die("Too many port forwarding ranges");
-	if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks))
-		die("Too many listening sockets");
+		warn("Dual stack rule has non-wildcard address %s",
+		     inany_ntop(&new->addr, astr, sizeof(astr)));
+		return -EINVAL;
+	}
+
+	if (fwd->count >= ARRAY_SIZE(fwd->rules)) {
+		warn("Too many rules (maximum %u)", ARRAY_SIZE(fwd->rules));
+		return -ENOSPC;
+	}
+	if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) {
+		warn("Rules require too many listening sockets (maximum %u)",
+		     ARRAY_SIZE(fwd->socks));
+		return -ENOSPC;
+	}
 
 	fwd->rulesocks[fwd->count] = &fwd->socks[fwd->sock_count];
 	for (port = new->first; port <= new->last; port++)
@@ -360,6 +380,7 @@ void fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
 
 	fwd->rules[fwd->count++] = *new;
 	fwd->sock_count += num;
+	return 0;
 }
 
 /**
diff --git a/fwd.h b/fwd.h
index 96b8c608..43bfeadb 100644
--- a/fwd.h
+++ b/fwd.h
@@ -85,7 +85,7 @@ struct fwd_scan {
 #define FWD_PORT_SCAN_INTERVAL		1000	/* ms */
 
 void fwd_rule_init(struct ctx *c);
-void fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new);
+int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new);
 const struct fwd_rule *fwd_rule_search(const struct fwd_table *fwd,
 				       const struct flowside *ini,
 				       uint8_t proto, int hint);
diff --git a/fwd_rule.c b/fwd_rule.c
index 5bc94efe..47d8df1c 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -45,8 +45,7 @@ const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule)
  */
 __attribute__((noinline))
 #endif
-static const char *fwd_rule_fmt(const struct fwd_rule *rule,
-				char *dst, size_t size)
+const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size)
 {
 	const char *percent = *rule->ifname ? "%" : "";
 	const char *weak = "", *scan = "";
diff --git a/fwd_rule.h b/fwd_rule.h
index f852be39..8506a0c4 100644
--- a/fwd_rule.h
+++ b/fwd_rule.h
@@ -51,6 +51,7 @@ struct fwd_rule {
 	 + sizeof(" []%:-  =>  - (best effort) (auto-scan)"))
 
 const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule);
+const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size);
 void fwd_rules_info(const struct fwd_rule *rules, size_t count);
 void fwd_rule_conflict_check(const struct fwd_rule *new,
 			     const struct fwd_rule *rules, size_t count);
-- 
2.53.0