Hello!
On Fri, Jun 23, 2023 at 07:55:47PM +0400, Sergey Kandaurov wrote:
> # HG changeset patch
> # User Sergey Kandaurov <pluk...@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
