Re: [PATCH libnetfilter_queue] Stop a memory leak in nfq_close

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Jun 11, 2024 at 12:46:39PM +1000, Duncan Roe wrote:
> Hi Pablo,
> 
> On Wed, Jun 05, 2024 at 05:19:23PM +0200, Pablo Neira Ayuso wrote:
> > Hi Duncan,
> >
> > On Tue, May 07, 2024 at 09:17:19AM +1000, Duncan Roe wrote:
> > > 0c5e5fb introduced struct nfqnl_q_handle *qh_list which can point to
> > > dynamically acquired memory. Without this patch, that memory is not freed.
> >
> > Indeed.
> >
> > Looking at the example available at utils, I can see this assumes
> > that:
> >
> >         nfq_destroy_queue(qh);
> >
> > needs to be called.
> >
> > qh->data can be also set to heap structure, in that case this would leak too.
> >
> > It seems nfq_destroy_queue() needs to be called before nfq_close() by design.
> 
> Oh sorry, I missed that. Anyone starting with the example available at utils as
> a template will be OK then.
> But someone carefully checking each line of code might do a
> `man nfq_destroy_queue` and see:
>        Removes the binding for the specified queue handle. This call also
>        unbind from the nfqueue handler, so you don't have to call
>        nfq_unbind_pf.
> And on then doing `man nfq_unbind_pf` that person would see:
>        Unbinds the given queue connection handle from processing packets
>        belonging to the given protocol family.
> 
>        This call is obsolete, Linux kernels from 3.8 onwards ignore it.
> And might draw the conclusion that the call to nfq_destroy_queue is unnecessary,
> especially if planning to call exit after calling nfq_close.

Then, update documentation.

> > Probably add:
> >
> >         assert(h->qh_list == NULL);
> 
> I don't like that. It would be the first assert() in libnetfilter_queue.
> libnfnetlink is peppered with asserts: I removed them in the replacement
> libmnl-using code because libmnl doesn't have them. Have you looked at the v2
> patches BTW? I'd really appreciate some feedback.
>
> >
> > at the top of nfq_close() instead to give a chance to users of this to
> > fix their code in case they are leaking qh?
> 
> It's not as important to call nfq_destroy_queue as it used to be. Why not just
> free the memory?

It is not possible to know if qh->data is stored in the bss, onstack
or the heap, it is up to the user to decide this.

> I could send a v2 with the Fixes: tag removed and a commit
> message that mentions the change is a backstop in case nfq_destroy_queue was not
> called.
> 
> Either way, `man nfq_destroy_queue` could be improved e.g.:
>        Removes the binding for the specified queue handle. This call also
>        releases associated internal memory.
> While being about it, how about removing the obsolete code snippet at the
> head of Library initialisation (that details calls to nfq_[un]bind_pf)?
> Perhaps a separate doc: patch?

I'd suggest to address this by updating documentation.

Thanks.