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

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

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.

Probably add:

        assert(h->qh_list == NULL);

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?

Thanks

> Fixes: 0c5e5fb15205 ("sync with all 'upstream' changes in libnfnetlink_log")
> Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
> ---
>  src/libnetfilter_queue.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/libnetfilter_queue.c b/src/libnetfilter_queue.c
> index bf67a19..f152efb 100644
> --- a/src/libnetfilter_queue.c
> +++ b/src/libnetfilter_queue.c
> @@ -481,7 +481,13 @@ EXPORT_SYMBOL
>  int nfq_close(struct nfq_handle *h)
>  {
>  	int ret;
> +	struct nfq_q_handle *qh;
>  
> +	while (h->qh_list) {
> +		qh = h->qh_list;
> +		h->qh_list = qh->next;
> +		free(qh);
> +	}
>  	ret = nfnl_close(h->nfnlh);
>  	if (ret == 0)
>  		free(h);
> -- 
> 2.35.8
> 
>