Index | Thread | Search

From:
Florian Obser <florian@openbsd.org>
Subject:
Re: dhcp6leased frontend memory leak
To:
Jonathan Matthew <jonathan@d14n.org>
Cc:
tech@openbsd.org
Date:
Thu, 19 Jun 2025 08:34:37 +0200

Download raw body.

Thread
Nice catch, OK florian

On 2025-06-19 08:08 +10, Jonathan Matthew <jonathan@d14n.org> wrote:
> While looking at memory usage on my home router, I noticed the dhcp6leased 
> frontend process had grown to around 170MB in the couple of months it had
> been running.
>
> After restarting with malloc leak checking and letting it run for a while,
> the malloc dump reported that a number of allocations from getifaddrs()
> hadn't been freed.
>
> update_iface() calls getifaddrs() and then returns early without calling
> freeifaddrs() in some conditions, one of which I'm hitting - it gets
> called for lo1, which isn't in the frontend's interface list.
> Not sure what that means.  My config is just
> 'request prefix delegation on dwqe0 for { lo1 vport0 vport1 }'.
>
> Anyway, update_iface() doesn't look at the interface list until after
> those checks are done, so we can just move the getifaddrs call down.
> I've been running with this diff for a bit and the steady growth of
> the frontend process (8-12 kB every 5 minutes) is no longer happening.
>
> ok?
>
> Index: frontend.c
> ===================================================================
> RCS file: /cvs/src/sbin/dhcp6leased/frontend.c,v
> diff -u -p -u -p -r1.21 frontend.c
> --- frontend.c	27 Apr 2025 16:22:33 -0000	1.21
> +++ frontend.c	18 Jun 2025 10:39:52 -0000
> @@ -559,9 +559,6 @@ update_iface(uint32_t if_index)
>  	int			 flags;
>  	char			 ifnamebuf[IF_NAMESIZE], *if_name;
>  
> -	if (getifaddrs(&ifap) != 0)
> -		fatal("getifaddrs");
> -
>  	if ((if_name = if_indextoname(if_index, ifnamebuf)) == NULL)
>  		return;
>  
> @@ -570,6 +567,9 @@ update_iface(uint32_t if_index)
>  
>  	if (find_iface_conf(&frontend_conf->iface_list, if_name) == NULL)
>  		return;
> +
> +	if (getifaddrs(&ifap) != 0)
> +		fatal("getifaddrs");
>  
>  	memset(&ifinfo, 0, sizeof(ifinfo));
>  	ifinfo.if_index = if_index;
>

-- 
In my defence, I have been left unsupervised.