nginx-devel Digest, Vol 96, Issue 10

[Cao Zhihua]

Hi Maxim,
Your patch is fine to me.

Thanks
Zhihua

Message: 1
Date: Wed, 11 Oct 2017 20:38:45 +0300
From: Maxim Dounin <mdounin at mdounin.ru>
To: nginx-devel at nginx.org
Subject: Re: [PATCH] Free the shared memory only when reconfiguration
	is successful
Message-ID: <20171011173844.GX75166 at mdounin.ru>
Content-Type: text/plain; charset=us-ascii

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,
https://urldefense.proofpoint.com/v2/url?u=http-3A__mailman.nginx.org_pipermail_nginx-2Ddevel_2017-2DOctober_010536.html&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=5hIR0vaOPz44U23VdaJSuH4pwdxinKp20laDxq322wI&m=s1p5j--5wkDOKiGinpToFlM7fKF9_zlVlcGwZe5_RD8&s=CPgxx9B73_SnhhF7jlX6DhY2qtIAmLtUA-2lu4OLeCw&e=.

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
https://urldefense.proofpoint.com/v2/url?u=http-3A__nginx.org_&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=5hIR0vaOPz44U23VdaJSuH4pwdxinKp20laDxq322wI&m=s1p5j--5wkDOKiGinpToFlM7fKF9_zlVlcGwZe5_RD8&s=Lhq1TAuEHcsGBdPKHCvpqkS_CYbBVJKBm00BqKcSQVk&e=