On Mon, Jul 20, 2020 at 01:14:00PM +0200, Alexandr Nedvedicky wrote:
> I took a closer look at your change and related area. Below is an alternate
> way to fix the bug you've found.
Thanks for bringing it up again, I forgot to reply earlier.
> there are few details worth to note:
>
> ptr_array matters to main ruleset only, because it is the only
> ruleset covered by MD5 sum for pfsync.
Correct, as is directly seen by the only caller pf_commit_rules():
819 /* Calculate checksum for the main ruleset */
820 if (rs == &pf_main_ruleset) {
821 error = pf_setup_pfsync_matching(rs);
822 if (error != 0)
823 return (error);
824 }
> it looks like we should also recompute the MD5sum if we remove the rule
> from the main ruleset
Yes, but that is a separate bug, regardless of how we handle checksum
calculation and rule lookups.
My plan was to tackle this next, but reusing existing code instead:
After my initial diff, pf_setup_pfsync_matching() would merely update
the checksum, so we could rename the function to something more
appropiate and call it from pf_purge_rule() instead of duplicating it
there.
> my diff introduces a new member of pr_ruleset structure, which keeps the
> array size so we can pass it to free(9f) later.
I persued this first (as done in other free() size diffs) but eventually
opted for the removal of ptr_array for the sake of simplicity.
> I have no pfsync deployment readily available for testing. Will be glad
> if you can give my diff a try.
Works as expected in both regards: pfsync picks the right rule and the
checksum updates after rules expired.
