ok mvs@
> On 22 Feb 2021, at 18:51, Alexander Bluhm <[email protected]> wrote:
>
> Hi,
>
> ip_insertoptions() may prepend a mbuf. In this case "goto bad" has
> to free the new chain. Currently we leak the new mbuf in front of
> the old chain. NetBSD has fixed this bug here:
>
> ----------------------------
> revision 1.33
> date: 1996-10-11 18:19:08 +0000; author: is; state: Exp; lines: +2 -2;
> Fix a mbuf leak in ip_output().
>
> Scenario: If ip_insertoptions() prepends a new mbuf to the chain, the
> bad: label's m_freem(m0) still would free only the original mbuf chain
> if the transmission failed for, e.g., no route to host; resulting in
> one lost mbuf per failed packet. (The original posting included a
> demonstration program).
>
> Original report of this bug was by [email protected]
> (JINMEI Tatuya) on comp.bugs.4bsd.
> ----------------------------
>
> Free m instead of m0 in the bad case. This allows to simplify a
> bunch of goto done.
>
> ok?
>
> bluhm
>
> Index: netinet/ip_output.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_output.c,v
> retrieving revision 1.365
> diff -u -p -r1.365 ip_output.c
> --- netinet/ip_output.c 10 Feb 2021 18:28:06 -0000 1.365
> +++ netinet/ip_output.c 22 Feb 2021 15:48:38 -0000
> @@ -111,9 +111,6 @@ ip_output(struct mbuf *m0, struct mbuf *
> #if NPF > 0
> u_int orig_rtableid;
> #endif
> -#ifdef MROUTING
> - int rv;
> -#endif
>
> NET_ASSERT_LOCKED();
>
> @@ -250,8 +247,7 @@ reroute:
> /* Should silently drop packet */
> if (error == -EINVAL)
> error = 0;
> - m_freem(m);
> - goto done;
> + goto bad;
> }
> if (tdb != NULL) {
> /*
> @@ -348,13 +344,13 @@ reroute:
> */
> if (ipmforwarding && ip_mrouter[ifp->if_rdomain] &&
> (flags & IP_FORWARDING) == 0) {
> + int rv;
> +
> KERNEL_LOCK();
> rv = ip_mforward(m, ifp);
> KERNEL_UNLOCK();
> - if (rv != 0) {
> - m_freem(m);
> - goto done;
> - }
> + if (rv != 0)
> + goto bad;
> }
> }
> #endif
> @@ -366,10 +362,8 @@ reroute:
> * loop back a copy if this host actually belongs to the
> * destination group on the loopback interface.
> */
> - if (ip->ip_ttl == 0 || (ifp->if_flags & IFF_LOOPBACK) != 0) {
> - m_freem(m);
> - goto done;
> - }
> + if (ip->ip_ttl == 0 || (ifp->if_flags & IFF_LOOPBACK) != 0)
> + goto bad;
>
> goto sendit;
> }
> @@ -427,8 +421,7 @@ sendit:
> if (pf_test(AF_INET, (flags & IP_FORWARDING) ? PF_FWD : PF_OUT,
> ifp, &m) != PF_PASS) {
> error = EACCES;
> - m_freem(m);
> - goto done;
> + goto bad;
> }
> if (m == NULL)
> goto done;
> @@ -453,8 +446,7 @@ sendit:
> if (ipsec_in_use && (flags & IP_FORWARDING) && (ipforwarding == 2) &&
> (m_tag_find(m, PACKET_TAG_IPSEC_IN_DONE, NULL) == NULL)) {
> error = EHOSTUNREACH;
> - m_freem(m);
> - goto done;
> + goto bad;
> }
> #endif
>
> @@ -534,7 +526,7 @@ done:
> if_put(ifp);
> return (error);
> bad:
> - m_freem(m0);
> + m_freem(m);
> goto done;
> }
>
>
