From owner-svn-src-all@freebsd.org Mon Sep 26 12:18:40 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 5AAA6BEACA1; Mon, 26 Sep 2016 12:18:40 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 22CABB22; Mon, 26 Sep 2016 12:18:39 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id E39653C8C1D; Mon, 26 Sep 2016 22:18:31 +1000 (AEST) Date: Mon, 26 Sep 2016 22:18:31 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Evans cc: Hiren Panchasara , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r306337 - head/sys/kern In-Reply-To: <20160926203343.S1542@besplex.bde.org> Message-ID: <20160926215837.K1725@besplex.bde.org> References: <201609261013.u8QADwrV002892@repo.freebsd.org> <20160926203343.S1542@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=VIkg5I7X c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=rWBMA1eanbLRUJGvs5oA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Sep 2016 12:18:40 -0000 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