Alexander Bluhm wrote:
> On Thu, Feb 19, 2015 at 06:31:01PM -0500, Ted Unangst wrote:
> > Maxime Villard wrote:
> > > Hi,
> > > I put here a bug among others:
> >
> > Thanks. I see two cases here where we need to pool_put the qs. Also need to
> > change return to break so that we release the rwlock.
> >
> >
> > Index: pf_ioctl.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/pf_ioctl.c,v
> > retrieving revision 1.282
> > diff -u -p -r1.282 pf_ioctl.c
> > --- pf_ioctl.c 10 Feb 2015 06:45:55 -0000 1.282
> > +++ pf_ioctl.c 19 Feb 2015 23:28:33 -0000
> > @@ -1032,10 +1032,14 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
> > bcopy(&q->queue, qs, sizeof(*qs));
> > qs->qid = pf_qname2qid(qs->qname, 1);
> > if (qs->parent[0] && (qs->parent_qid =
> > - pf_qname2qid(qs->parent, 0)) == 0)
> > - return (ESRCH);
> > + pf_qname2qid(qs->parent, 0)) == 0) {
> > + pool_put(&pf_queue_pl, qs);
> > + error = ESRCH;
> > + break;
> > + }
> > qs->kif = pfi_kif_get(qs->ifname);
> > if (!qs->kif->pfik_ifp) {
>
> pfi_kif_get() may return NULL. And if it mallocs the kif, we would
> leak it here. But it does not set pfik_ifp. I think this check
> should be "if (qs->kif == NULL)".
Yes. That is consistent with other callers.
Index: pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.282
diff -u -p -r1.282 pf_ioctl.c
--- pf_ioctl.c 10 Feb 2015 06:45:55 -0000 1.282
+++ pf_ioctl.c 20 Feb 2015 01:00:29 -0000
@@ -1032,10 +1032,14 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
bcopy(&q->queue, qs, sizeof(*qs));
qs->qid = pf_qname2qid(qs->qname, 1);
if (qs->parent[0] && (qs->parent_qid =
- pf_qname2qid(qs->parent, 0)) == 0)
- return (ESRCH);
+ pf_qname2qid(qs->parent, 0)) == 0) {
+ pool_put(&pf_queue_pl, qs);
+ error = ESRCH;
+ break;
+ }
qs->kif = pfi_kif_get(qs->ifname);
- if (!qs->kif->pfik_ifp) {
+ if (qs->kif == NULL) {
+ pool_put(&pf_queue_pl, qs);
error = ESRCH;
break;
}
