[PATCH] Core: fixed environment on exit better

[Maxim Dounin]

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.

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().

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/