Date: Mon, 26 Sep 2016 22:18:31 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: Hiren Panchasara <hiren@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r306337 - head/sys/kern Message-ID: <20160926215837.K1725@besplex.bde.org> In-Reply-To: <20160926203343.S1542@besplex.bde.org> References: <201609261013.u8QADwrV002892@repo.freebsd.org> <20160926203343.S1542@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 26 Sep 2016, Bruce Evans wrote: > On Mon, 26 Sep 2016, Hiren Panchasara wrote: > >> Author: hiren >> Date: Mon Sep 26 10:13:58 2016 >> New Revision: 306337 >> URL: https://svnweb.freebsd.org/changeset/base/306337 >> >> Log: >> In sendit(), if mp->msg_control is present, then in sockargs() we are >> allocating I didn't actually write mangling of the quotes like that. >> mbuf to store mp->msg_control. Later in kern_sendit(), call to >> getsock_cap(), >> will check validity of file pointer passed, if this fails EBADF is >> returned but >> mbuf allocated in sockargs() is not freed. Fix this possible leak. Hmm. In my reply I thought it was a cleanup after a simpler function that was missing. The bad API makes it kern_sendit()'s responsibility to clean up in all cases. I don't like the layering, but the mere existence of kern_sendit() means that you can't fix it by changing one of its callers. It exists so that it can have multiple callers. it has 4 other callers, and 2 of these (linux and freebsd32 compat) pass it a non-null 'control' >> @@ -737,6 +737,8 @@ sendit(struct thread *td, int s, struct >> >> bad: >> free(to, M_SONAME); >> + if (control) >> + m_freem(control); >> return (error); >> } > > The "bad" label is an old style bug. This is the general return path, > not an error handling path. This gives many collateral obfuscations > and pessimizations. > > Now it seems to give a large bug with the help of the obfuscations: > when mp->msg_control != NULL, in the usual subcase where there is no > error, kern_sendit() must have already done m_freem(control) else the > usual subcase would have leaked. The code falls through to "bad" > and does m_freem(control) again. It is possible that a double m_freem() does nothing the second time because it sees a null chain, but it doesn't seem to support that. > [... my cleanups don't fix the problem] Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160926215837.K1725>