[PATCH] Core: fixed environment on exit better
Maxim Dounin
mdounin at mdounin.ru
Wed Jul 19 02:31:18 UTC 2023
Hello!
On Tue, Jul 18, 2023 at 09:51:49PM +0400, Sergey Kandaurov wrote:
> > On 18 Jul 2023, at 04:19, Maxim Dounin <mdounin at mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Thu, Jul 13, 2023 at 08:58:44PM +0400, Sergey Kandaurov wrote:
> >
> >>> 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.
> >
> > 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).
>
> Let's take a look at the master/worker fork model, for clarity,
> where allocations are decided to be leaked in the cleanup handler,
> that is, in ngx_cleanup_environment().
>
> In this model, cleanup handlers are registered on the master process
> startup and on each reconfigure, and called when the master process
> is exited, as part of destroying the pool in ngx_master_process_exit(),
> and on each reconfigure, where old cycle pool is destroyed at the
> end of successful ngx_init_cycle(), correspondingly.
> Additionally, cleanup handlers are called on worker process exit
> in ngx_worker_process_exit(), due to the fork model.
>
> The difference is that if such allocations are postponed to the cleanup
> handler in worker processes and to be leaked, this won't affect master
> process memory, while worker processes are about to exit anyway.
> In that sense, allocations should be guarded additionally with
> ngx_process == NGX_PROCESS_WORKER. Unlike that, and what you propose,
> allocating things in the master process means they can be leaked there
> as well, and such leakages can be accumulated with each reconfigure.
What happens in worker processes is indeed not really interesting,
as environment there is expected to leak, and it will do so.
But in the master process you also have to preserve environment on
exit as long as it was set there (notably, with the embedded perl
module compiled in). And you decide to leak or not based on the
environ state in the cleanup handler, so there is no difference in
whether the memory is allocated in ngx_set_environment() or in the
cleanup handler.
> There is already an allocation of n+1 environment pointers, though,
> in ngx_set_environment().
> In this regard, it seems natural to continue allocations there, as
> you propose, and address potential leakages separately, if at all.
>
> Additionally, to make it clear what happens on reconfigure, the
> master process appears to replace the environment before calling
> cleanup handlers attributed to the old cycle, such that the proposed
> ngx_cleanup_environment_variable() should be able to free memory
> (that's not the case for process exit, but see above).
> So leaking memory for environment variables seems to continue
> to be rather hypothetical.
Well, I tend to think that I actually know how to trigger a leak:
if we compile perl module dynamically, and load it, it will call
ngx_set_environment() during configuration parsing. And if we'll
unload it on the next configuration reload, environ won't be
overwritten, so cleanup handler will leak it. Then it can be
loaded again, and so on. (Nitpicking: on FreeBSD this also
requires TZ to be set, since otherwise ngx_timezone_update() will
update environ.)
This behaviour is, however, irrelevant to the approach taken, and
will manifest itself regardless of the place where actual
allocation happens. (And, of course, this behaviour is already
present with the current code.)
Not sure if fixing this worth the effort though.
> >
> > 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.
>
> It is handled by removing such variables from environment
> if allocation has failed.
Sure, it is handled in your code. Still, this handling will
result in corrupted environment, without the variable in question,
and results are unpredictable.
> This is rather odd behaviour though.
> In general, allocations in cleanup look awkward and best to avoid.
Exactly.
>
> > 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 <mdounin at 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)
> > {
> >
>
> That said, I think your change is good.
Pushed to http://mdounin.ru/hg/nginx, thanks for looking.
--
Maxim Dounin
http://mdounin.ru/
More information about the nginx-devel
mailing list