On Sat, Sep 12, 2015 at 11:17:54AM +0200, Martin Pieuchot wrote:
> I'd like to have careful reviews. I'm running with this for a couple
> of months but I doubt I'm exercising all the code paths.
> @@ -1677,24 +1677,21 @@ icmp6_redirect_output(struct mbuf *m0, s
>
> {
> /* target lladdr option */
> - struct rtentry *rt_nexthop = NULL;
Do you think it is a good idea to reuse the rt parameter in this
goto spaghetti mess? I looks correct, but I would keep a local
variable for this local route usage.
> @@ -1351,6 +1356,7 @@ nd6_cache_lladdr(struct ifnet *ifp, stru
> } else {
> /* do nothing if static ndp is set */
> if (rt->rt_flags & RTF_STATIC)
> + rtfree(rt);
> return;
> is_newentry = 0;
> }
The brackets around { rtfree(rt); return; } are missing.
> @@ -1699,13 +1714,11 @@ nd6_output(struct ifnet *ifp, struct mbu
> return (0);
>
> sendpkt:
> - return ((*ifp->if_output)(ifp, m, sin6tosa(dst), rt));
> -
> - bad:
> - m_freem(m);
> + error = ((*ifp->if_output)(ifp, m, sin6tosa(dst), rt));
> + if (created)
> + rtfree(rt);
> return (error);
> }
You also need a rtfree(rt) before the return (0).
> @@ -888,6 +888,7 @@ nd6_na_input(struct mbuf *m, int off, in
> }
>
> freeit:
> + rtfree(rt);
> m_freem(m);
> if_put(ifp);
> return;
There are goto freeit; before rt is set. As rt is not initialized,
you may free some value on the stack.
> LIST_FOREACH(pfxrtr, &pr->ndpr_advrtrs, pfr_entry) {
> @@ -1545,7 +1550,9 @@ find_pfxlist_reachable_router(struct nd_
> (ln = (struct llinfo_nd6 *)rt->rt_llinfo) &&
> ND6_IS_LLINFO_PROBREACH(ln))
> break; /* found */
> + rtfree(rt);
> }
> + rtfree(rt);
If you have found nothing, do not break and finish the loop, you
do a double free.
bluhm
