[PATCH] Core: fixed environment on exit better

[Roman Arutyunyan]

Hi,

On Wed, Jun 28, 2023 at 06:08:35PM +0300, Maxim Dounin wrote:
> Hello!
> 
> On Wed, Jun 28, 2023 at 04:45:16PM +0400, Roman Arutyunyan wrote:
> 
> > On Sat, Jun 24, 2023 at 05:06:46AM +0300, Maxim Dounin wrote:
> > > Hello!
> > > 
> > > On Fri, Jun 23, 2023 at 07:55:47PM +0400, Sergey Kandaurov wrote:
> > > 
> > > > # HG changeset patch
> > > > # User Sergey Kandaurov <pluknet at nginx.com>
> > > > # Date 1687535739 -14400
> > > > #      Fri Jun 23 19:55:39 2023 +0400
> > > > # Node ID 34a8e1a4161542896c11c4a5c60d6a6fe1931e3d
> > > > # Parent  c681f4906d838ac45b517fada4c4274ade03fd3c
> > > > Core: fixed environment on exit better.
> > > > 
> > > > The fix in 6822:c045b4926b2c to replace environment allocations from a pool
> > > > was incomplete, such that it left pool owned allocations for variables set
> > > > with values using the "env" directive.  Another consumer of this interface
> > > > is the QUIC BPF helper that uses the environment variable NGINX_BPF_MAPS.
> > > > The fix is to re-allocate variables with malloc, to avoid use after free.
> > > > If memory allocation failed, the only option is to expel from environment.
> > > > 
> > > > The observed symptoms are similar to described in 6822:c045b4926b2c, that
> > > > is a segmentation fault on worker process exit from atexit() handler seen
> > > > with 3rd party modules or if nginx was built with lcov profiling.
> > > > 
> > > > diff --git a/src/core/nginx.c b/src/core/nginx.c
> > > > --- a/src/core/nginx.c
> > > > +++ b/src/core/nginx.c
> > > > @@ -631,17 +631,48 @@ ngx_cleanup_environment(void *data)
> > > >  {
> > > >      char  **env = data;
> > > >  
> > > > -    if (environ == env) {
> > > > +    char        *p, **penv;
> > > > +    size_t       size;
> > > > +    ngx_uint_t   i, n;
> > > >  
> > > > -        /*
> > > > -         * if the environment is still used, as it happens on exit,
> > > > -         * the only option is to leak it
> > > > -         */
> > > > -
> > > > +    if (environ != env) {
> > > > +        ngx_free(env);
> > > >          return;
> > > >      }
> > > 
> > > Note: if we assume that the environment can be arbitrary modified, 
> > > I don't think it's enough.  For example, a setenv() call can 
> > > replace environ with something different, so this code will free 
> > > env and exit, but pool-allocated variables will be still 
> > > referenced by the new environ.
> > 
> > Another problem is if setenv() was called, environ may have been
> > reallocated with realloc() which may result in double-free in this function,
> > even with current code.
> 
> I don't think that realloc() is allowed here.  At least BSD libc 
> does not try to do this and only use realloc() on it's internal 
> storage, and glibc does the same.

You're right.  I see, glibc only realloc()'ates what it allocated itself.


stdlib/setenv.c:

/* If this variable is not a null pointer we allocated the current               
   environment.  */                                                              
static char **last_environ; 

[..]

--
Roman Arutyunyan