svn commit: r306337 - head/sys/kern

[Bruce Evans]

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