On 25/05/16(Wed) 01:47, Alexander Bluhm wrote:
> 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?
> [...]
> > 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?
I did, I'm not convinced (yet) but not opposed.
>
> > @@ -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.
Good catch, updated diff below.
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 27 May 2016 11:34:54 -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);
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);
@@ -1163,10 +1164,8 @@ 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);
+ rt->rt_gwroute = NULL;
return (0);
}
