[PATCH] Free the shared memory only when reconfiguration is successful

[Maxim Dounin]

Hello!

On Wed, Oct 11, 2017 at 01:45:04AM -0700, Zhihua Cao wrote:

> # HG changeset patch
> # User Zhihua Cao <czhihua at vmware.com>
> # Date 1507710209 25200
> #      Wed Oct 11 01:23:29 2017 -0700
> # Node ID 648b1cca8f50d83eea02a6cc2c105ae95a3f3d72
> # Parent  3012fcb69db4f35dd5851e3156625dc18a823fce
> Free the shared memory only when reconfiguration is successful
> 
> If nginx reconfiguration fails, it maybe crash when killing the master process.
> The reason is that the unreused shared memory is freed during reconfiguration,
> the shared memory address is unmapped from master's address space, but the
> reconfiguration maybe fail(open listening sockets maybe fails due to the
> address is already inuse) and roll back to the old configuration.
> When killing nginx process, master still access the adrress
> which is unmapped from master's address space, segment fault exception occurs.

Thank you for the patch.  See comments below.

> diff -r 3012fcb69db4 -r 648b1cca8f50 src/core/ngx_cycle.c
> --- a/src/core/ngx_cycle.c	Tue Oct 10 18:22:51 2017 +0300
> +++ b/src/core/ngx_cycle.c	Wed Oct 11 01:23:29 2017 -0700
> @@ -16,6 +16,7 @@
>  static ngx_int_t ngx_test_lockfile(u_char *file, ngx_log_t *log);
>  static void ngx_clean_old_cycles(ngx_event_t *ev);
>  static void ngx_shutdown_timer_handler(ngx_event_t *ev);
> +static void ngx_shared_memory_reset_stale(ngx_cycle_t *cycle);
>  
>  
>  volatile ngx_cycle_t  *ngx_cycle;
> @@ -470,7 +471,8 @@
>                  goto shm_zone_found;
>              }
>  
> -            ngx_shm_free(&oshm_zone[n].shm);
> +            /* don't free the old shm zone here, just mark it stale */
> +            oshm_zone[n].stale = 1;
>  
>              break;
>          }

There should be no need to introduce additional the "stale" flag and 
set/reset it.  Simply using identical conditions in the "free the 
unnecessary shared memory" loop should work fine.  See patch below 
(mostly untested though).

Another problem to consider here are platforms where shared memory 
zone name is actually used externally, notably Windows.  On the 
other hand, re-creating shared memory zone with the same name 
seems to be already silently broken at least on Windows: it simply 
uses the old zone instead, potentially leading to a segmentation 
fault as well.  With the patch, it now properly fails.

# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1507739571 -10800
#      Wed Oct 11 19:32:51 2017 +0300
# Node ID 7e1cdcd9e88283c0ba6883a4bbc73fd784a4cf19
# Parent  3012fcb69db4f35dd5851e3156625dc18a823fce
Core: free shared memory zones only after reconfiguration.

This is what usually happens for zones no longer used in the new
configuration, but zones where size or tag were changed were freed
when creating a new memory zones.  If reconfiguration failed (for
example, due a conflicting listening socket), this resulted in a
segmentation fault in the master process.

Reported by Zhihua Cao,
http://mailman.nginx.org/pipermail/nginx-devel/2017-October/010536.html.

diff --git a/src/core/ngx_cycle.c b/src/core/ngx_cycle.c
--- a/src/core/ngx_cycle.c
+++ b/src/core/ngx_cycle.c
@@ -470,8 +470,6 @@ ngx_init_cycle(ngx_cycle_t *old_cycle)
                 goto shm_zone_found;
             }
 
-            ngx_shm_free(&oshm_zone[n].shm);
-
             break;
         }
 
@@ -662,14 +660,26 @@ ngx_init_cycle(ngx_cycle_t *old_cycle)
                 n = 0;
             }
 
-            if (oshm_zone[i].shm.name.len == shm_zone[n].shm.name.len
-                && ngx_strncmp(oshm_zone[i].shm.name.data,
-                               shm_zone[n].shm.name.data,
-                               oshm_zone[i].shm.name.len)
-                == 0)
+            if (oshm_zone[i].shm.name.len != shm_zone[n].shm.name.len) {
+                continue;
+            }
+
+            if (ngx_strncmp(oshm_zone[i].shm.name.data,
+                            shm_zone[n].shm.name.data,
+                            oshm_zone[i].shm.name.len)
+                != 0)
+            {
+                continue;
+            }
+
+            if (oshm_zone[i].tag == shm_zone[n].tag
+                && oshm_zone[i].shm.size == shm_zone[n].shm.size
+                && !oshm_zone[i].noreuse)
             {
                 goto live_shm_zone;
             }
+
+            break;
         }
 
         ngx_shm_free(&oshm_zone[i].shm);

-- 
Maxim Dounin
http://nginx.org/