[PATCH] Core: fixed environment on exit better

[Maxim Dounin]

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.

> > This is probably out of the scope of this patch though.
> > 
> > >  
> > > -    ngx_free(env);
> > > +    /*
> > > +     * if the environment is still used, as it happens on exit,
> > > +     * the only option is to leak it
> > > +     */
> > > +
> > > +    for (n = 0; env[n]; n++) { }
> > > +
> > > +    for (i = 0; env[i]; i++) {
> > > +
> > > +        for (penv = ngx_os_environ; *penv; penv++) {
> > > +            if (ngx_strcmp(*penv, env[i]) == 0) {
> > 
> > I don't think it's going to work.  If I'm reading this correctly, 
> > with the initial environment "FOO=bar" and "env FOO=bar;" in the 
> > configuration this will assume the string is from ngx_os_environ, 
> > but it will be from ccf->env instead.
> > 
> > > +                break;
> > > +            }
> > > +        }
> > > +
> > > +        if (*penv) {
> > > +            continue;
> > > +        }
> > > +
> > > +        /* substitute pool allocated variables */
> > > +
> > > +        size = ngx_strlen(env[i]) + 1;
> > > +        p = malloc(size);
> > 
> > There should be no direct malloc() calls except in low-level code 
> > where logging is not available.  I don't think this particular 
> > place counts, so ngx_alloc() should be used instead.
> > 
> > > +
> > > +        if (p) {
> > > +            ngx_memcpy(p, env[i], size);
> > > +            env[i] = p;
> > > +
> > > +        } else {
> > > +            ngx_memmove(&env[i], &env[i + 1], sizeof(char *) * (n - i));
> > > +            i--; n--;
> > > +        }
> > > +    }
> > >  }
> > 
> > Overall, it looks like a better idea might be to simply add all 
> > the variables added to the environment to the environ allocation 
> > in ngx_set_environment().
> 
> Again, if setenv() is called, realloc() will free everything, including the
> variables.

See above, there should be no realloc().

> On the other hand, since setenv() is not supported anyway, and no
> one complained, we can try this simple approach.

I believe currently setenv() cannot cause issues additional to the 
one being fixed by the patch.  In the worst case, setenv() will 
allocate new environ and copy pointers to the variables there.  As 
long as variables are not pool-allocated, everything will work 
fine.  With pool-allocated variables, there will be a segmentation 
fault if variables are being looked up in an _atexit() handler.

(Another question is if setenv() will actually work as expected if 
called by some code.  Since nginx will clear/replace environment 
on each worker process startup, setenv() results are likely to be 
cleared unless set in the worker process itself.)

To properly support setenv(), a fix would be to check in 
ngx_cleanup_environment() not only environ itself, but also if any 
of the variables in current environ refer to the memory allocated 
in ngx_set_environment().

> 
> > In ngx_set_environment() we explicitly know if the particular 
> > variable is from OS environment or from ccf->environ, so there 
> > will be no need to guess.  Also, the appropriate log is readily 
> > available (and already used in the existing ngx_alloc() call), and 
> > the code to cleanup things is already there and will work without 
> > additional modifications.

-- 
Maxim Dounin
http://mdounin.ru/