all: massive src/db.c rework

First, and most importantly, let me state that this is perhaps the worst
possible example of a patch I can think of, and if anyone tries to submit
a PR/patch like this one I will reject it almost immediately.  I'm only
merging this because 1) this patch escalated quickly, 2) splitting it would
require a disproportionate amount of time, and 3) this effort had blocked
other work for too long ... and, well, I'm the maintainer.  Consider this
a bit of "maintainer privilege" if you will.

This patch started simply enough: the goal was to add/augment some tests to
help increase the libseccomp test coverage.  Unfortunately, this particular
test improvement uncovered a rather tricky bug which escalated quite quickly
and soon involved a major rework of how we build the filter tree in src/db.c.
This rework brought about changes throughout the repository, including the
transaction and ABI specific code.

Signed-off-by: Paul Moore <paul@paul-moore.com>

1 files changed, 38 insertions, 31 deletions

diff --git a/src/arch-x86.c b/src/arch-x86.c
index a1178f5..4126737 100644
--- a/src/arch-x86.c
+++ b/src/arch-x86.c
@@ -210,24 +210,25 @@ int x86_syscall_rewrite(int *syscall)
 
 /**
  * add a new rule to the x86 seccomp filter
- * @param col the filter collection
  * @param db the seccomp filter db
- * @param strict the strict flag
  * @param rule the filter rule
  *
  * This function adds a new syscall filter to the seccomp filter db, making any
  * necessary adjustments for the x86 ABI.  Returns zero on success, negative
  * values on failure.
  *
+ * It is important to note that in the case of failure the db may be corrupted,
+ * the caller must use the transaction mechanism if the db integrity is
+ * important.
+ *
  */
-int x86_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
-		 struct db_api_rule_list *rule)
+int x86_rule_add(struct db_filter *db, struct db_api_rule_list *rule)
 {
-	int rc;
+	int rc = 0;
 	unsigned int iter;
 	int sys = rule->syscall;
 	int sys_a, sys_b;
-	struct db_api_rule_list *rule_a, *rule_b;
+	struct db_api_rule_list *rule_a, *rule_b, *rule_dup = NULL;
 
 	if ((sys <= -100 && sys >= -120) || (sys >= 359 && sys <= 373)) {
 		/* (-100 to -120) : multiplexed socket syscalls
@@ -235,21 +236,27 @@ int x86_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
 
 		/* strict check for the multiplexed socket syscalls */
 		for (iter = 0; iter < ARG_COUNT_MAX; iter++) {
-			if ((rule->args[iter].valid != 0) && (strict))
-				return -EINVAL;
+			if ((rule->args[iter].valid != 0) && (rule->strict)) {
+				rc = -EINVAL;
+				goto add_return;
+			}
 		}
 
 		/* determine both the muxed and direct syscall numbers */
 		if (sys > 0) {
 			sys_a = _x86_sock_mux(sys);
-			if (sys_a == __NR_SCMP_ERROR)
-				return __NR_SCMP_ERROR;
+			if (sys_a == __NR_SCMP_ERROR) {
+				rc = __NR_SCMP_ERROR;
+				goto add_return;
+			}
 			sys_b = sys;
 		} else {
 			sys_a = sys;
 			sys_b = _x86_sock_demux(sys);
-			if (sys_b == __NR_SCMP_ERROR)
-				return __NR_SCMP_ERROR;
+			if (sys_b == __NR_SCMP_ERROR) {
+				rc = __NR_SCMP_ERROR;
+				goto add_return;
+			}
 		}
 
 		/* use rule_a for the multiplexed syscall and use rule_b for
@@ -264,9 +271,10 @@ int x86_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
 		} else {
 			/* need two rules, dup the first and link together */
 			rule_a = rule;
-			rule_b = db_rule_dup(rule_a);
+			rule_dup = db_rule_dup(rule_a);
+			rule_b = rule_dup;
 			if (rule_b == NULL)
-				return -ENOMEM;
+				goto add_return;
 			rule_b->prev = rule_a;
 			rule_b->next = NULL;
 			rule_a->next = rule_b;
@@ -286,26 +294,24 @@ int x86_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
 		if (rule_b != NULL)
 			rule_b->syscall = sys_b;
 
-		/* add the rules as a single transaction */
-		rc = db_col_transaction_start(col);
-		if (rc < 0)
-			return rc;
+		/* we should be protected by a transaction checkpoint */
 		if (rule_a != NULL) {
 			rc = db_rule_add(db, rule_a);
 			if (rc < 0)
-				goto fail_transaction;
+				goto add_return;
 		}
 		if (rule_b != NULL) {
 			rc = db_rule_add(db, rule_b);
 			if (rc < 0)
-				goto fail_transaction;
+				goto add_return;
 		}
-		db_col_transaction_commit(col);
 	} else if (sys <= -200 && sys >= -224) {
 		/* multiplexed ipc syscalls */
 		for (iter = 0; iter < ARG_COUNT_MAX; iter++) {
-			if ((rule->args[iter].valid != 0) && (strict))
-				return -EINVAL;
+			if ((rule->args[iter].valid != 0) && (rule->strict)) {
+				rc = -EINVAL;
+				goto add_return;
+			}
 		}
 		rule->args[0].arg = 0;
 		rule->args[0].op = SCMP_CMP_EQ;
@@ -316,18 +322,19 @@ int x86_rule_add(struct db_filter_col *col, struct db_filter *db, bool strict,
 
 		rc = db_rule_add(db, rule);
 		if (rc < 0)
-			return rc;
+			goto add_return;
 	} else if (sys >= 0) {
 		/* normal syscall processing */
 		rc = db_rule_add(db, rule);
 		if (rc < 0)
-			return rc;
-	} else if (strict)
-		return -EDOM;
-
-	return 0;
+			goto add_return;
+	} else if (rule->strict) {
+		rc = -EDOM;
+		goto add_return;
+	}
 
-fail_transaction:
-	db_col_transaction_abort(col);
+add_return:
+	if (rule_dup != NULL)
+		free(rule_dup);
 	return rc;
 }