simplify and document lookup_next_rule()

1 files changed, 22 insertions, 43 deletions

diff --git a/sys/netinet/ipfw/ip_fw2.c b/sys/netinet/ipfw/ip_fw2.c
index 8562330..df0af0e 100644
--- a/sys/netinet/ipfw/ip_fw2.c
+++ b/sys/netinet/ipfw/ip_fw2.c
@@ -630,50 +630,27 @@ send_reject(struct ip_fw_args *args, int code, int ip_len, struct ip *ip)
 }
 
 /**
- * Given an ip_fw *, lookup_next_rule will return a pointer
- * to the next rule, which can be either the jump
- * target (for skipto instructions) or the next one in the list (in
- * all other cases including a missing jump target).
- * The result is also written in the "next_rule" field of the rule.
- * Backward jumps are not allowed, so we start the search from the
- * rule following the current one.
+ * Return the pointer to the skipto target.
+ *
+ * IMPORTANT: this should only be called on SKIPTO rules, and the
+ * jump target is taken from the 'rulenum' argument, which may come
+ * from the rule itself (direct skipto) or not (tablearg)
  *
  * The function never returns NULL: if the requested rule is not
  * present, it returns the next rule in the chain.
- * As a side effect, the rule pointer is also set so next time
- * the jump will not require a scan of the list.
+ * This also happens in case of a bogus argument > 65535
  */
-
 static struct ip_fw *
-lookup_next_rule(struct ip_fw *me, u_int32_t tablearg)
+lookup_next_rule(struct ip_fw *me, uint32_t rulenum)
 {
-	struct ip_fw *rule = NULL;
-	ipfw_insn *cmd;
-	u_int16_t	rulenum;
-
-	/* look for action, in case it is a skipto */
-	cmd = ACTION_PTR(me);
-	if (cmd->opcode == O_LOG)
-		cmd += F_LEN(cmd);
-	if (cmd->opcode == O_ALTQ)
-		cmd += F_LEN(cmd);
-	if (cmd->opcode == O_TAG)
-		cmd += F_LEN(cmd);
-	if (cmd->opcode == O_SKIPTO ) {
-		if (tablearg != 0) {
-			rulenum = (u_int16_t)tablearg;
-		} else {
-			rulenum = cmd->arg1;
-		}
-		for (rule = me->next; rule ; rule = rule->next) {
-			if (rule->rulenum >= rulenum) {
-				break;
-			}
-		}
+	struct ip_fw *rule;
+
+	for (rule = me->next; rule ; rule = rule->next) {
+		if (rule->rulenum >= rulenum)
+			break;
 	}
 	if (rule == NULL)	/* failure or not a skipto */
-		rule = me->next;
-	me->next_rule = rule;
+		rule = me->next ? me->next : me;
 	return rule;
 }
 
@@ -2013,13 +1990,15 @@ do {								\
 					l = 0;		/* exit inner loop */
 					break;
 				}
-				/* handle skipto */
+				/* skipto: */
 				if (cmd->arg1 == IP_FW_TABLEARG) {
-					f = lookup_next_rule(f, tablearg);
-				} else {
-					if (f->next_rule == NULL)
-						lookup_next_rule(f, 0);
-					f = f->next_rule;
+				    f = lookup_next_rule(f, tablearg);
+				} else { /* direct skipto */
+				    /* update f->next_rule if not set */
+				    if (f->next_rule == NULL)
+					f->next_rule =
+					    lookup_next_rule(f, cmd->arg1);
+				    f = f->next_rule;
 				}
 				/*
 				 * Skip disabled rules, and
@@ -2032,7 +2011,7 @@ do {								\
 				if (f) { /* found a valid rule */
 					l = f->cmd_len;
 					cmd = f->cmd;
-				} else {
+				} else { /* should not happen */
 					l = 0;  /* exit inner loop */
 				}
 				match = 1;