Dear tech@,
the scan-build(1) tool from the llvm port on sbin/pfctl reported a
memory leak in the error path of load_feedback_profile.
There are more places that could be investigated.
Would it make sense to look into these?
Independent of that I was wondering if it makes sense to warn if
pfctl_optimize_ruleset fails. There cases should exist where
the function with 1 without printing a warning.
Any feedback?
mbuhl
Index: sbin/pfctl/pfctl.c
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.388
diff -u -p -r1.388 pfctl.c
--- sbin/pfctl/pfctl.c 27 Jul 2022 12:28:27 -0000 1.388
+++ sbin/pfctl/pfctl.c 27 Jul 2022 12:46:05 -0000
@@ -1452,7 +1452,8 @@ pfctl_load_ruleset(struct pfctl *pf, cha
}
if (pf->optimize)
- pfctl_optimize_ruleset(pf, rs);
+ if (pfctl_optimize_ruleset(pf, rs))
+ warnx("ruleset optimizations failed");
while ((r = TAILQ_FIRST(rs->rules.active.ptr)) != NULL) {
TAILQ_REMOVE(rs->rules.active.ptr, r, entries);
Index: sbin/pfctl/pfctl_optimize.c
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl_optimize.c,v
retrieving revision 1.49
diff -u -p -r1.49 pfctl_optimize.c
--- sbin/pfctl/pfctl_optimize.c 28 Jan 2022 05:24:15 -0000 1.49
+++ sbin/pfctl/pfctl_optimize.c 27 Jul 2022 12:46:05 -0000
@@ -869,13 +869,13 @@ load_feedback_profile(struct pfctl *pf,
struct pf_ruleset *rs;
if ((por = calloc(1, sizeof(*por))) == NULL) {
warn("calloc");
- return (1);
+ goto free_queue;
}
pr.nr = nr;
if (ioctl(pf->dev, DIOCGETRULE, &pr) == -1) {
warnx("%s", pf_strerror(errno));
free(por);
- return (1);
+ goto free_queue;
}
memcpy(&por->por_rule, &pr.rule, sizeof(por->por_rule));
rs = pf_find_or_create_ruleset(pr.anchor_call);
@@ -884,7 +884,7 @@ load_feedback_profile(struct pfctl *pf,
}
if (construct_superblocks(pf, &queue, &prof_superblocks))
- return (1);
+ goto free_queue;
/*
@@ -920,6 +920,14 @@ load_feedback_profile(struct pfctl *pf,
blockcur = block;
}
return (0);
+
+free_queue:
+ while ((por = TAILQ_FIRST(&queue))) {
+ TAILQ_REMOVE(&queue, por, por_entry);
+ free(por);
+ }
+
+ return (1);
}
