[PATCH] Core: fixed environment on exit better

[Roman Arutyunyan]

Hi,

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.

> 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.  On the other hand, since setenv() is not supported anyway, and no
one complained, we can try this simple approach.

> 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/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Roman Arutyunyan