[PATCH 11/18] 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 configuraiton
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      | 40 ++++++++++++++++++++++++++++++----------
 fwd.h      |  2 +-
 fwd_rule.c |  3 +--
 fwd_rule.h |  1 +
 5 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/conf.c b/conf.c
index a93837cc..cd30f2f8 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..cee2c7f6 100644
--- a/fwd.c
+++ b/fwd.c
@@ -335,24 +335,43 @@ 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("Forwarding rule has invalid port range %u-%u",
+		     new->first, new->last);
+		return -EINVAL;
+	}
+	if (new->flags & ~allowed_flags) {
+		warn("Forwarding rule has invalid flags 0x%x",
+		     new->flags & ~allowed_flags);
+		return -EINVAL;
+	}
+	if (new->flags & FWD_DUAL_STACK_ANY &&
+	    !inany_equals(&new->addr, &inany_any6)) {
+		warn("Dual stack forward must have unspecified address");
+		return -EINVAL;
+	}
 
-	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");
+	if (fwd->count >= ARRAY_SIZE(fwd->rules)) {
+		warn("Too many forwarding rules (maximum %u)",
+		     ARRAY_SIZE(fwd->rules));
+		return -ENOSPC;
+	}
+	if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) {
+		warn(
+"Forwarding 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 +379,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 1d74cbd2..c56749e1 100644
--- a/fwd.h
+++ b/fwd.h
@@ -83,7 +83,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 4d5048f9..ef9308c2 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -39,8 +39,7 @@ const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule)
  * @dst:	Buffer to store output (should have FWD_RULE_STRLEN bytes)
  * @size:	Size of @dst
  */
-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