[PATCH] Core: fixed environment on exit better
Sergey Kandaurov
pluknet at nginx.com
Thu Jul 13 16:58:44 UTC 2023
> On 24 Jun 2023, at 06:06, Maxim Dounin <mdounin at mdounin.ru> 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.
>
> This is probably out of the scope of this patch though.
It could be solved by always checking for pool-allocated variables,
irrespective of whether environ was changed after setenv(),
using that as an additional condition to free environment.
>
>>
>> - 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.
>
Thanks for catching that.
So, we have to compare against 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.
I used to avoid using cycle in cleanup, thus direct malloc() calls.
Anyway, it will be needed in order to access configuration.
Below is an updated patch (commit log unchanged).
# HG changeset patch
# User Sergey Kandaurov <pluknet at nginx.com>
# Date 1689267366 -14400
# Thu Jul 13 20:56:06 2023 +0400
# Node ID 80a60d9707ed4b80402335d3d31a978f418f8056
# Parent f91dc350be9f4a6bf1379a32a210afece7b0d75e
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
@@ -592,7 +592,7 @@ tz_found:
}
cln->handler = ngx_cleanup_environment;
- cln->data = env;
+ cln->data = cycle;
}
n = 0;
@@ -629,19 +629,57 @@ tz_found:
static void
ngx_cleanup_environment(void *data)
{
- char **env = data;
-
- if (environ == env) {
+ ngx_cycle_t *cycle = data;
- /*
- * if the environment is still used, as it happens on exit,
- * the only option is to leak it
- */
+ char *p, **env;
+ size_t size;
+ ngx_str_t *var;
+ ngx_uint_t i, n;
+ ngx_core_conf_t *ccf;
+ ccf = (ngx_core_conf_t *) ngx_get_conf(cycle->conf_ctx, ngx_core_module);
+ env = ccf->environment;
+
+ if (environ != env) {
+ ngx_free(env);
return;
}
- 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++) { /* void */ }
+
+ while (*env) {
+ var = ccf->env.elts;
+
+ for (i = 0; i < ccf->env.nelts; i++) {
+ if (var[i].data == (u_char *) *env) {
+ break;
+ }
+ }
+
+ if (i == ccf->env.nelts) {
+ env++;
+ continue;
+ }
+
+ /* substitute pool allocated variables */
+
+ size = ngx_strlen(*env) + 1;
+ p = ngx_alloc(size, cycle->log);
+
+ if (p) {
+ ngx_memcpy(p, *env, size);
+ *env++ = p;
+
+ } else {
+ ngx_memmove(env, env + 1, sizeof(char *) * (n - (env - environ)));
+ n--;
+ }
+ }
}
>
>> +
>> + 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.
I tried to avoid allocations in ngx_set_environment(),
as it can be called from the master process and harm
hypothetically by accumulating leaked allocations.
--
Sergey Kandaurov
More information about the nginx-devel
mailing list