Hi,
On Wed, Jun 28, 2023 at 06:08:35PM +0300, Maxim Dounin wrote:
> Hello!
>
> On Wed, Jun 28, 2023 at 04:45:16PM +0400, Roman Arutyunyan wrote:
>
> > On Sat, Jun 24, 2023 at 05:06:46AM +0300, Maxim Dounin 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.
> >
> > Another problem is if setenv() was called, environ may have been
> > reallocated with realloc() which may result in double-free in this function,
> > even with current code.
>
> I don't think that realloc() is allowed here. At least BSD libc
> does not try to do this and only use realloc() on it's internal
> storage, and glibc does the same.
You're right. I see, glibc only realloc()'ates what it allocated itself.
stdlib/setenv.c:
/* If this variable is not a null pointer we allocated the current
environment. */
static char **last_environ;
[..]
--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
