On Tue, May 24, 2016 at 05:03:32PM +0200, Martin Pieuchot wrote:
> Hrvoje Popovski founds that as soon as his ARP cache reached 2097152
> entries his machine started leaking route entries.
>
> Turns out that with that many entries he exhausted malloc(9) and
> rt_setgate() started failing, leaking the rt->rt_parent reference.
>
> The diff below fixes the issue, ok?
>
> Index: net/route.c
> ===================================================================
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.300
> diff -u -p -r1.300 route.c
> --- net/route.c 2 May 2016 22:15:49 -0000 1.300
> +++ net/route.c 24 May 2016 15:02:06 -0000
> @@ -1091,6 +1091,10 @@ rtrequest(int req, struct rt_addrinfo *i
> * it to (re)order routes.
> */
> if ((error = rt_setgate(rt, info->rti_info[RTAX_GATEWAY]))) {
> + ifafree(ifa);
> + rtfree(rt->rt_parent);
> + rtfree(rt->rt_gwroute);
> + free(rt->rt_gateway, M_RTABLE, 0);
If rt_setgate() fails, neither rt_gwroute nor rt_gateway have been
allocated. So freeing them should not be necessary. But it does
not hurt either and may be safer.
> free(ndst, M_RTABLE, dlen);
> pool_put(&rtentry_pool, rt);
> return (error);
> @@ -1118,12 +1122,9 @@ rtrequest(int req, struct rt_addrinfo *i
> }
> if (error != 0) {
> ifafree(ifa);
> - if ((rt->rt_flags & RTF_CLONED) != 0 && rt->rt_parent)
> - rtfree(rt->rt_parent);
> - if (rt->rt_gwroute)
> - rtfree(rt->rt_gwroute);
> - if (rt->rt_gateway)
> - free(rt->rt_gateway, M_RTABLE, 0);
> + rtfree(rt->rt_parent);
> + rtfree(rt->rt_gwroute);
> + free(rt->rt_gateway, M_RTABLE, 0);
> free(ndst, M_RTABLE, dlen);
> pool_put(&rtentry_pool, rt);
> return (EEXIST);
Have you considered to move those two error handlings into a goto
error to avoid duplication? Do you know why we return (EEXIST)
instead of (error) here?
> @@ -1163,10 +1164,7 @@ rt_setgate(struct rtentry *rt, struct so
> }
> memmove(rt->rt_gateway, gate, glen);
>
> - if (rt->rt_gwroute != NULL) {
> - rtfree(rt->rt_gwroute);
> - rt->rt_gwroute = NULL;
> - }
> + rtfree(rt->rt_gwroute);
Why have you removed the rt->rt_gwroute = NULL here? It results
in a freed pointer that may be used later.
>
> return (0);
> }
