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.
