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

[Ian Lepore]

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.

-- Ian