Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 12 Mar 2017 12:04:16 -0600
From:      Ian Lepore <ian@freebsd.org>
To:        Pedro Giffuni <pfg@FreeBSD.org>, Lawrence Stewart <lstewart@freebsd.org>, src-committers@freebsd.org, Dag-Erling =?ISO-8859-1?Q?Sm=F8rgrav?= <des@des.no>, svn-src-head@freebsd.org
Subject:   Re: svn commit: r314780 - head/lib/libpam/modules/pam_exec
Message-ID:  <1489341856.40576.88.camel@freebsd.org>
In-Reply-To: <1839903b-fb05-bf3f-17bb-697afca9ecb7@FreeBSD.org>
References:  <201703061545.v26FjkNI027057@repo.freebsd.org> <739617a4-3eed-28d1-73e4-86d25d6d5fed@freebsd.org> <1839903b-fb05-bf3f-17bb-697afca9ecb7@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1489341856.40576.88.camel>