[PATCH] Fixed segfault when switching off master process during upgrade

Maxim Dounin mdounin at mdounin.ru
Thu Nov 24 18:26:01 UTC 2022


Hello!

On Thu, Nov 24, 2022 at 01:09:41PM +0400, Roman Arutyunyan wrote:

> Hi,
> 
> On Wed, Nov 23, 2022 at 11:52:00PM +0300, Maxim Dounin wrote:
> > Hello!
> > 
> > On Wed, Nov 23, 2022 at 05:56:25PM +0400, Roman Arutyunyan wrote:
> > 
> > > Hi,
> > > 
> > > On Sun, Oct 30, 2022 at 05:42:33AM +0300, Maxim Dounin wrote:
> > > > # HG changeset patch
> > > > # User Maxim Dounin <mdounin at mdounin.ru>
> > > > # Date 1667097733 -10800
> > > > #      Sun Oct 30 05:42:13 2022 +0300
> > > > # Node ID ef9c94be7fe4685f0eeee41f76b964ea252f519f
> > > > # Parent  b73d95226c84b93e51f23f7b35782d98d3b516b9
> > > > Fixed segfault when switching off master process during upgrade.
> > > > 
> > > > Binary upgrades are not supported without master process, but it is,
> > > > however, possible, that nginx running with master process is asked
> > > > to upgrade binary, and the configuration file as available on disk
> > > > at this time includes "master_process off;".
> > > > 
> > > > If this happens, listening sockets inherited from the previous binary
> > > > will have ls[i].previous set.  But the old cycle on initial process
> > > > startup, including startup after binary upgrade, is destroyed by
> > > > ngx_init_cycle() once configuration parsing is complete.  As a result,
> > > > an attempt to dereference ls[i].previous in ngx_event_process_init()
> > > > accesses already freed memory.
> > > > 
> > > > Fix is to avoid looking into ls[i].previous if the old cycle is already
> > > > freed.
> > > > 
> > > > diff --git a/src/event/ngx_event.c b/src/event/ngx_event.c
> > > > --- a/src/event/ngx_event.c
> > > > +++ b/src/event/ngx_event.c
> > > > @@ -813,7 +813,9 @@ ngx_event_process_init(ngx_cycle_t *cycl
> > > >          rev->deferred_accept = ls[i].deferred_accept;
> > > >  #endif
> > > >  
> > > > -        if (!(ngx_event_flags & NGX_USE_IOCP_EVENT)) {
> > > > +        if (!(ngx_event_flags & NGX_USE_IOCP_EVENT)
> > > > +            && cycle->old_cycle)
> > > > +        {
> > > >              if (ls[i].previous) {
> > > >  
> > > >                  /*
> > > 
> > > Doesn't this also mean that we can throw away zeroing ls->previous?
> > > 
> > > diff --git a/src/os/unix/ngx_process_cycle.c b/src/os/unix/ngx_process_cycle.c
> > > --- a/src/os/unix/ngx_process_cycle.c
> > > +++ b/src/os/unix/ngx_process_cycle.c
> > > @@ -888,15 +888,6 @@ ngx_worker_process_init(ngx_cycle_t *cyc
> > >      tp = ngx_timeofday();
> > >      srandom(((unsigned) ngx_pid << 16) ^ tp->sec ^ tp->msec);
> > >  
> > > -    /*
> > > -     * disable deleting previous events for the listening sockets because
> > > -     * in the worker processes there are no events at all at this point
> > > -     */
> > > -    ls = cycle->listening.elts;
> > > -    for (i = 0; i < cycle->listening.nelts; i++) {
> > > -        ls[i].previous = NULL;
> > > -    }
> > > -
> > >      for (i = 0; cycle->modules[i]; i++) {
> > >          if (cycle->modules[i]->init_process) {
> > >              if (cycle->modules[i]->init_process(cycle) == NGX_ERROR) {
> > > 
> > 
> > Yes, this code is now redundant.  Added to the patch, with the 
> > following paragraph in the commit log:
> > 
> > : With this change it is also no longer needed to clear ls[i].previous in
> > : worker processes, so the relevant code was removed.
> > 
> > Full patch:
> > 
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1669236533 -10800
> > #      Wed Nov 23 23:48:53 2022 +0300
> > # Node ID 8852f39311de9913435fb2f2fc601a3655f42a99
> > # Parent  09463dd9c50463e35856a3bd6b57bc6ca382e6f0
> > Fixed segfault when switching off master process during upgrade.
> > 
> > Binary upgrades are not supported without master process, but it is,
> > however, possible, that nginx running with master process is asked
> > to upgrade binary, and the configuration file as available on disk
> > at this time includes "master_process off;".
> > 
> > If this happens, listening sockets inherited from the previous binary
> > will have ls[i].previous set.  But the old cycle on initial process
> > startup, including startup after binary upgrade, is destroyed by
> > ngx_init_cycle() once configuration parsing is complete.  As a result,
> > an attempt to dereference ls[i].previous in ngx_event_process_init()
> > accesses already freed memory.
> > 
> > Fix is to avoid looking into ls[i].previous if the old cycle is already
> > freed.
> > 
> > With this change it is also no longer needed to clear ls[i].previous in
> > worker processes, so the relevant code was removed.
> > 
> > diff --git a/src/event/ngx_event.c b/src/event/ngx_event.c
> > --- a/src/event/ngx_event.c
> > +++ b/src/event/ngx_event.c
> > @@ -813,7 +813,9 @@ ngx_event_process_init(ngx_cycle_t *cycl
> >          rev->deferred_accept = ls[i].deferred_accept;
> >  #endif
> >  
> > -        if (!(ngx_event_flags & NGX_USE_IOCP_EVENT)) {
> > +        if (!(ngx_event_flags & NGX_USE_IOCP_EVENT)
> > +            && cycle->old_cycle)
> > +        {
> >              if (ls[i].previous) {
> >  
> >                  /*
> > diff --git a/src/os/unix/ngx_process_cycle.c b/src/os/unix/ngx_process_cycle.c
> > --- a/src/os/unix/ngx_process_cycle.c
> > +++ b/src/os/unix/ngx_process_cycle.c
> > @@ -759,7 +759,6 @@ ngx_worker_process_init(ngx_cycle_t *cyc
> >      ngx_cpuset_t     *cpu_affinity;
> >      struct rlimit     rlmt;
> >      ngx_core_conf_t  *ccf;
> > -    ngx_listening_t  *ls;
> >  
> >      if (ngx_set_environment(cycle, NULL) == NULL) {
> >          /* fatal */
> > @@ -889,15 +888,6 @@ ngx_worker_process_init(ngx_cycle_t *cyc
> >      tp = ngx_timeofday();
> >      srandom(((unsigned) ngx_pid << 16) ^ tp->sec ^ tp->msec);
> >  
> > -    /*
> > -     * disable deleting previous events for the listening sockets because
> > -     * in the worker processes there are no events at all at this point
> > -     */
> > -    ls = cycle->listening.elts;
> > -    for (i = 0; i < cycle->listening.nelts; i++) {
> > -        ls[i].previous = NULL;
> > -    }
> > -
> >      for (i = 0; cycle->modules[i]; i++) {
> >          if (cycle->modules[i]->init_process) {
> >              if (cycle->modules[i]->init_process(cycle) == NGX_ERROR) {
> 
> Looks good.

Pushed to http://mdounin.ru/hg/nginx, thanks.

-- 
Maxim Dounin
http://mdounin.ru/



More information about the nginx-devel mailing list