Re: [PATCH 1/4] Add framework for tunables

[Florian Weimer <fweimer at redhat dot com>]

On 12/27/2016 09:59 PM, Siddhesh Poyarekar wrote:

> > > +static int
> > > +tunables_unsetenv (char **ep, const char *name)
> >
> > This is copied from elf/dl-environ.c, it seems.  Is it possible to avoid
> > creating this copy?
>
> I'm going to eventually drop the dl-environ.c copy by moving the LD_*
> variables to tunables.

This is fine.  Perhaps add a comment.

> > Calling either form of unsetenv is problematic because it may make the
> > auxiliary vector unreachable on !LIBC_START_MAIN_AUXVEC_ARG platforms
> > (which seems to include the majority, so perhaps this is a non-issue).
>
> I'm not sure what the problem is here.

If auxv isn't passed explicitly, the vector starts after the last NULL 
pointer in the envp array.  If you put in additional NULL pointers there 
to remove element, that's not going to work anymore.

> Either way, it doesn't seem like
> something the patch introduces, is it?

Probably not, but I don't have a complete picture in my head of the 
startup code.

> > > diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
> >
> > > +/* A tunable.  */
> > > +struct _tunable
> > > +{
> >
> > > +  const char *env_alias;        /* The compatibility environment
> > > +                       variable name.  */
> >
> > Maybe store the variable name as well, as to
>
> I don't understand this, can you please elaborate?

I meant to write “variable name length”, so that it's possible to 
optimize the comparison somewhat.  But it's likely better to keep the 
complexity down for now, so please disregard this suggestion.

> > I believe this introduces a security vulnerability.  You must keep the
> > setting of dl_debug_mask.
>
> I'll add it back in #ifdef SHARED in disable_tunable.

Are you sure this is sufficient?  You need to make sure that the value 
is zeroed out again after it has been populated from the environment and 
before it is used in any way by the dynamic linker.

Thanks,
Florian