Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 12 Mar 2017 14:31:17 -0500
From:      Pedro Giffuni <pfg@FreeBSD.org>
To:        Ian Lepore <ian@freebsd.org>, Lawrence Stewart <lstewart@freebsd.org>, src-committers@freebsd.org, =?UTF-8?Q?Dag-Erling_Sm=c3=b8rgrav?= <des@des.no>, svn-src-head@freebsd.org
Subject:   Re: svn commit: r314780 - head/lib/libpam/modules/pam_exec
Message-ID:  <374fa6f7-9e02-7c50-ccde-7f2c5386123e@FreeBSD.org>
In-Reply-To: <1489341856.40576.88.camel@freebsd.org>
References:  <201703061545.v26FjkNI027057@repo.freebsd.org> <739617a4-3eed-28d1-73e4-86d25d6d5fed@freebsd.org> <1839903b-fb05-bf3f-17bb-697afca9ecb7@FreeBSD.org> <1489341856.40576.88.camel@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help


On 3/12/2017 1:04 PM, Ian Lepore wrote:
> On Sun, 2017-03-12 at 12:30 -0500, Pedro Giffuni wrote:
>> On 3/12/2017 12:14 PM, Lawrence Stewart wrote:
>>> Hi Pedro,
>>>
>>> On 07/03/2017 02:45, Pedro F. Giffuni wrote:
>>>> Author: pfg
>>>> Date: Mon Mar  6 15:45:46 2017
>>>> New Revision: 314780
>>>> URL: https://svnweb.freebsd.org/changeset/base/314780
>>>>
>>>> Log:
>>>>     libpam: extra bounds checking through reallocarray(3).
>>>>     
>>>>     Reviewed by:	des
>>>>     MFC after:	1 week
>>>>
>>>> Modified:
>>>>     head/lib/libpam/modules/pam_exec/pam_exec.c
>>>>
>>>> Modified: head/lib/libpam/modules/pam_exec/pam_exec.c
>>>> =================================================================
>>>> =============
>>>> --- head/lib/libpam/modules/pam_exec/pam_exec.c	Mon Mar  6
>>>> 15:42:03 2017	(r314779)
>>>> +++ head/lib/libpam/modules/pam_exec/pam_exec.c	Mon Mar  6
>>>> 15:45:46 2017	(r314780)
>>>> @@ -138,7 +138,7 @@ _pam_exec(pam_handle_t *pamh __unused,
>>>>    	nitems = sizeof(env_items) / sizeof(*env_items);
>>>>    	/* Count PAM return values put in the environment. */
>>>>    	nitems_rv = options->return_prog_exit_status ?
>>>> PAM_RV_COUNT : 0;
>>>> -	tmp = realloc(envlist, (envlen + nitems + 1 + nitems_rv
>>>> + 1) *
>>>> +	tmp = reallocarray(envlist, envlen + nitems + 1 +
>>>> nitems_rv + 1,
>>>>    	    sizeof(*envlist));
>>>>    	if (tmp == NULL) {
>>>>    		openpam_free_envlist(envlist);
>>>>
>>> This commit breaks pam_exec for me... without this change I see the
>>> expected PAM_* environment variables from my execed script, but
>>> with
>>> this change I no longer see any of them.
>> Thanks for the report.
>>
>> It seems strange this can cause any failure. Perhaps there is a
>> latent
>> overflow here and we have been living with it? I will revert while it
>> is
>> investigated.
>>
>> BTW, the "nitems" variable may conflict with nitems() in sys/param.h.
>>
> A quirk of C that's often forgotten is that a function-like macro is
> only expanded as a macro if the token following the macro name is an
> open paren.  So nitems() is a macro invokation and nitems = 0; is just
> a variable.
>
> I'm not arguing against the replacement of variables named nitems, I
> actually think that should have been done as part of importing the
> function-like definition of nitems from netbsd.

I am not worried about 'nitems', which is actually FreeBSD-native 
(NetBSD has another name for it).

I do think reallocarray() found an underlying issue here though.

Pedro.




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?374fa6f7-9e02-7c50-ccde-7f2c5386123e>