Hello,
On Fri, Dec 29, 2017 at 11:04:07PM +0100, Alexander Bluhm wrote:
> On Thu, Dec 28, 2017 at 12:46:45AM +0100, Alexandr Nedvedicky wrote:
> > I like your change. You have my OK with small change here:
>
> I have commitet my diff with a small addition. I fixed a memory
> leak that I had introduced before.
>
> > > -pf_pkt_state_key_ref(struct mbuf *m)
> > > +pf_mbuf_link_state_key(struct mbuf *m, struct pf_state_key *sk)
> > > {
> > > - pf_state_key_ref(m->m_pkthdr.pf.statekey);
> > KASSERT(m->m_pkthdr.pf.statekey == NULL);
> > > + m->m_pkthdr.pf.statekey = pf_state_key_ref(sk);
> > > }
> >
> > I think we should have KASSERT() above as kind of check if
> > more safety belts are needed to mitigate potential memory leak.
>
> It does not work that easily. The only caller m_dup_pkthdr() does
> set the statekey before.
>
> I tried to put an assert "to->m_pkthdr.pf.statekey == NULL" before
> the "to->m_pkthdr = from->m_pkthdr" in m_dup_pkthdr(). But the
> mbuf "to" is not initialized, it is m_dup_pkthdr()'s job to fill
> the memory. So we cannot assume that to->m_pkthdr.pf.statekey
> contains anything useful.
>
> All we could do is this:
>
> ok?
>
I have no better plan.
Thanks and OK
sashan
