svn commit: r314780 - head/lib/libpam/modules/pam_exec

[Pedro Giffuni]

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.