Hello!

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

> # HG changeset patch
> # User Zhihua Cao <czhihua@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@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel