move down pf_tag_unref() calls in pf_rm_rule() to after the check wetehr there

are still states for the given rule existant.
based on a very nice analysis from cedric@, that is so completely right that
I have nothing to add:

in pf_rm_rule(), the pf_tag_unref() calls are done *before*
the if (rule->states > 0 || rule->entries.tqe_prev != NULL) test.

That mean that the two pf_tag_unref() calls could occur *twice*
for a given rule: first when the rule is removed from the ruleset
and (if the rule was kept around because of a state) a second
time when the state refcount drops to zero and the rule gets
really deleted. Unless I'm mistaken, that breaks the refcounting.

...and cedric was not mistaken.
and, as daniel pointed out:
The breakage this causes is so subtle, I doubt anyone noticed it before, if it
did occur.

consensus on this between cedric, dhartmei and myself

1 files changed, 3 insertions, 3 deletions

diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index 6198ece0529..4355a7e9a46 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pf_ioctl.c,v 1.70 2003/06/23 02:33:39 cedric Exp $ */
+/*	$OpenBSD: pf_ioctl.c,v 1.71 2003/06/27 11:38:23 henning Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -407,10 +407,10 @@ pf_rm_rule(struct pf_rulequeue *rulequeue, struct pf_rule *rule)
 		rule->entries.tqe_prev = NULL;
 		rule->nr = -1;
 	}
-	pf_tag_unref(rule->tag);
-	pf_tag_unref(rule->match_tag);
 	if (rule->states > 0 || rule->entries.tqe_prev != NULL)
 		return;
+	pf_tag_unref(rule->tag);
+	pf_tag_unref(rule->match_tag);
 	pf_dynaddr_remove(&rule->src.addr);
 	pf_dynaddr_remove(&rule->dst.addr);
 	if (rulequeue == NULL) {