Hello!
On Thu, Jul 13, 2023 at 08:58:44PM +0400, Sergey Kandaurov wrote:
> > On 24 Jun 2023, at 06:06, Maxim Dounin <mdou...@mdounin.ru> wrote:
> >
> > 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.
>
> 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 <pluk...@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.
The ngx_set_environment() function installs a cleanup handler,
which is to be called on cycle pool destruction, and therefore
there can't be a leak unless the cleanup handler will decide it
needs to leak the allocation.
And, if the cleanup handler will decide to leak it, there should
be no difference with re-allocating things in the cleanup handler
(except the fact that allocations in cleanup handler are risky,
see below).
Rather, I would prefer to avoid allocations in
ngx_cleanup_environment(), since on failure this will corrupt the
environment with unpredictable results, and there are no options
to avoid this. Allocating everything in ngx_set_environment()
seems to be much safer approach.
I've tried a few variants, and the most simple one seems to
allocate variables one-by-one with appropriate cleanup handlers in
ngx_set_environment(). (Doing the same in ngx_set_env() is
slightly easier, but doesn't play well with QUIC.) This approach
addresses all possible environment modifications and also looks
perfectly in line with the existing code.
Here is the patch:
# HG changeset patch
# User Maxim Dounin <mdou...@mdounin.ru>
# Date 1689637083 -10800
# Tue Jul 18 02:38:03 2023 +0300
# Node ID dedb235210f816afc028f60a525009fc5dabd8bf
# Parent 77c1418916f7817a0d020f28d8a08f278a12fe43
Core: fixed environment variables on exit.
Similarly to 6822:c045b4926b2c, environment variables introduced with
the "env" directive (and "NGINX_BPF_MAPS" added by QUIC) are now allocated
via ngx_alloc(), and explicitly freed by a cleanup handler if no longer used.
In collaboration with Sergey Kandaurov.
diff --git a/src/core/nginx.c b/src/core/nginx.c
--- a/src/core/nginx.c
+++ b/src/core/nginx.c
@@ -13,6 +13,7 @@
static void ngx_show_version_info(void);
static ngx_int_t ngx_add_inherited_sockets(ngx_cycle_t *cycle);
static void ngx_cleanup_environment(void *data);
+static void ngx_cleanup_environment_variable(void *data);
static ngx_int_t ngx_get_options(int argc, char *const *argv);
static ngx_int_t ngx_process_options(ngx_cycle_t *cycle);
static ngx_int_t ngx_save_argv(ngx_cycle_t *cycle, int argc, char *const
*argv);
@@ -518,7 +519,8 @@ ngx_add_inherited_sockets(ngx_cycle_t *c
char **
ngx_set_environment(ngx_cycle_t *cycle, ngx_uint_t *last)
{
- char **p, **env;
+ char **p, **env, *str;
+ size_t len;
ngx_str_t *var;
ngx_uint_t i, n;
ngx_core_conf_t *ccf;
@@ -600,7 +602,31 @@ tz_found:
for (i = 0; i < ccf->env.nelts; i++) {
if (var[i].data[var[i].len] == '=') {
- env[n++] = (char *) var[i].data;
+
+ if (last) {
+ env[n++] = (char *) var[i].data;
+ continue;
+ }
+
+ cln = ngx_pool_cleanup_add(cycle->pool, 0);
+ if (cln == NULL) {
+ return NULL;
+ }
+
+ len = ngx_strlen(var[i].data) + 1;
+
+ str = ngx_alloc(len, cycle->log);
+ if (str == NULL) {
+ return NULL;
+ }
+
+ ngx_memcpy(str, var[i].data, len);
+
+ cln->handler = ngx_cleanup_environment_variable;
+ cln->data = str;
+
+ env[n++] = str;
+
continue;
}
@@ -645,6 +671,29 @@ ngx_cleanup_environment(void *data)
}
+static void
+ngx_cleanup_environment_variable(void *data)
+{
+ char *var = data;
+
+ char **p;
+
+ for (p = environ; *p; p++) {
+
+ /*
+ * if an environment variable is still used, as it happens on exit,
+ * the only option is to leak it
+ */
+
+ if (*p == var) {
+ return;
+ }
+ }
+
+ ngx_free(var);
+}
+
+
ngx_pid_t
ngx_exec_new_binary(ngx_cycle_t *cycle, char *const *argv)
{
--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
