Hello!
On Wed, Apr 22, 2015 at 09:45:45AM +0200, Sergey Brester wrote:
> enclosed you will find an attached changeset, that fixes a ASLR/DEP
> problem on windows platforms (example Win 7/2008 x64).
>
> To find shared addr offset with ASLR, we have successful used the same
> resp. similar solution on various open source projects (ex.: postgresql
> etc.). Also nginx with suggested patch works fine over 5 months in
> production on several machines (Win-2k8-R2).
Interesting, thanks, though the patch certainly needs some
cleanup. See below for some comments.
> BTW(1): I have interest to fix a problem with multiple workers under
> windows (ex.: http://forum.nginx.org/read.php?2,188714,188714#msg-188714
> [1]).
> @Maxim Dounin: can you possibly give me more information, what you mean
> here, resp. what it currently depends on (see
> http://forum.nginx.org/read.php?2,188714,212568#msg-212568 [2]).
The most obvious problem with multiple workers on Windows is
listening sockets. As of now, if you start multiple workers on
windows, nginx will open multiple listening sockets - one in each
worker process. These are independant sockets, each opened with
SO_REUSEADDR, and only one of these sockets will be able to accept
connections.
Possible solution to the problem would be to pass listening sockets
from a master process to worker processes via handle inheritance
as available in Windows:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms683463(v=vs.85).aspx
[...]
> # HG changeset patch
> # User Serg G. Brester (sebres) <serg.brester@sebres.de>
> # Date 1429643760 -7200
> # Tue Apr 21 21:16:00 2015 +0200
> # Branch sb-mod
> # Node ID f759f7084204bb1f3c2b0a8023a8f03a81da9a0b
> # Parent 1bdfceda86a99a4dc99934181d2f9e2632003ca8
> Windows shmem fix: makes shared memory fully ASLR and DEP compliant (ea. cache zone, limit zone, etc.)
>
> diff -r 1bdfceda86a9 -r f759f7084204 src/os/win32/ngx_shmem.c
> --- a/src/os/win32/ngx_shmem.c Mon Apr 20 17:36:51 2015 +0300
> +++ b/src/os/win32/ngx_shmem.c Tue Apr 21 21:16:00 2015 +0200
> @@ -9,11 +9,30 @@
> #include <ngx_core.h>
>
>
> +static volatile size_t g_addrbaseoffs = 0;
> +
> +static volatile int g_SystemInfo_init = 0;
> +SYSTEM_INFO g_SystemInfo;
> +
> +int ngx_shm_init_once() {
> + if (g_SystemInfo_init)
> + return 1;
> + g_SystemInfo_init = 1;
> + GetSystemInfo(&g_SystemInfo);
> + return 1;
> +}
Such things belong to ngx_win32_init.c. It may be also good
enough to use ngx_pagesize, which is already set there.
> +
> +/* ------------------- */
> +
There are lots of style problems which need cleanup.
> ngx_int_t
> ngx_shm_alloc(ngx_shm_t *shm)
> {
> u_char *name;
> uint64_t size;
> + u_char *addr;
> + u_char *addrbase;
> +
> + ngx_shm_init_once();
>
> name = ngx_alloc(shm->name.len + 2 + NGX_INT32_LEN, shm->log);
> if (name == NULL) {
> @@ -26,6 +45,9 @@
>
> size = shm->size;
>
> + // increase for base address, will be saved inside shared mem :
> + size += sizeof(addr);
> +
As of now, uses of ngx_shm_alloc() already contain address in the
shared memory region where addresses are important. It may worth
to try to use it, if possible (may be with some additional call to
redo the mapping with the address specified).
> shm->handle = CreateFileMapping(INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE,
> (u_long) (size >> 32),
> (u_long) (size & 0xffffffff),
> @@ -34,7 +56,7 @@
> if (shm->handle == NULL) {
> ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno,
> "CreateFileMapping(%uz, %s) failed",
> - shm->size, name);
> + size, name);
> ngx_free(name);
>
> return NGX_ERROR;
> @@ -42,14 +64,79 @@
>
> ngx_free(name);
>
> + shm->exists = 0;
This looks like an unneeded change. The exists flag is set to 0
in ngx_shared_memory_add(), when ngx_shm_t is allocated and
initialized.
> if (ngx_errno == ERROR_ALREADY_EXISTS) {
> shm->exists = 1;
> }
>
> - shm->addr = MapViewOfFile(shm->handle, FILE_MAP_WRITE, 0, 0, 0);
> + /*
> + // Because of Win x64 since Vista/Win7 (always ASLR in kernel32), and nginx uses pointer
> + // inside this shared areas, we should use the same address for shared memory (Windows IPC)
> +
> + // Now we set preferred base to a hardcoded address that newborn processes never seem to be using (in available versions of Windows).
> + // The addresses was selected somewhat randomly in order to minimize the probability that some other library doing something similar
> + // conflicts with us. That is, using conspicuous addresses like 0x20000000 might not be good if someone else does it.
> + */
> + #ifdef _WIN64
> + /*
> + * There is typically a giant hole (almost 8TB):
> + * 00000000 7fff0000
> + * ...
> + * 000007f6 8e8b0000
> + */
> + addrbase = (u_char*)0x0000047047e00000ULL;
> + #else
> + /*
> + * This is more dicey. However, even with ASLR there still
> + * seems to be a big hole:
> + * 10000000
> + * ...
> + * 70000000
> + */
> + addrbase = (u_char*)0x2efe0000;
> + #endif
This probably should be somewhere at ngx_win32_config.h.
>
> - if (shm->addr != NULL) {
> + // add offset (corresponding all used shared) to preferred base:
> + addrbase += g_addrbaseoffs;
> +
> + addr = MapViewOfFileEx(shm->handle, FILE_MAP_WRITE, 0, 0, 0, addrbase);
Have you tried using an arbitrary addresses (with subsequent
remap using proper base address for already existing mappings)?
Does is work noticeably worse?
> +
> + ngx_log_debug4(NGX_LOG_DEBUG_CORE, shm->log, 0, "map shared \"%V\" -> %p (base %p), size: %uz", &shm->name, addr, addrbase, size);
> +
> + if (addr != NULL) {
> +
> + // get allocated address if already exists:
> + if (shm->exists) {
> + // get base and realoc using it if different:
> + addrbase = *(u_char **)addr;
> + ngx_log_debug3(NGX_LOG_DEBUG_CORE, shm->log, 0, "shared \"%V\" -> %p -> %p", &shm->name, addr, addrbase);
> + if (addrbase != addr) {
> + // free:
> + if (UnmapViewOfFile(addr) == 0) {
> + ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno,
> + "UnmapViewOfFile(%p) of file mapping \"%V\" failed",
> + addr, &shm->name);
> + }
> + // allocate again using base address:
> + addr = MapViewOfFileEx(shm->handle, FILE_MAP_WRITE, 0, 0, 0,
> + addrbase // lpBaseAddress
> + );
> + }
> + } else {
> + ngx_log_debug3(NGX_LOG_DEBUG_CORE, shm->log, 0, "shared \"%V\" -> %p (base %p)", &shm->name, addr, addrbase);
> + // save first allocated address as base for next caller :
> + *(u_char **)addr = addr;
It might be better idea to save the address at the end of the
memory block allocated, this will at least allow to avoid address
manipulation here and in ngx_shm_free(). Or use the address
stored in the memory block itself, see above.
> + }
> +
> + if (addr != NULL) {
> + // increase base offset (use proper alignment, obtain it from dwAllocationGranularity):
> + g_addrbaseoffs += (size + g_SystemInfo.dwAllocationGranularity) & (0xffffffff & ~(g_SystemInfo.dwAllocationGranularity-1));
> + //ngx_log_debug2(NGX_LOG_DEBUG_CORE, shm->log, 0, "offset %ui, granularity %ui", g_addrbaseoffs, g_SystemInfo.dwAllocationGranularity);
> + shm->addr = addr + sizeof(addr);
> + ngx_log_debug3(NGX_LOG_DEBUG_CORE, shm->log, 0, "shared alloc \"%V\" -> %p = %p", &shm->name, addr, shm->addr);
> return NGX_OK;
> + }
> +
> }
>
> ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno,
> @@ -65,14 +152,17 @@
> return NGX_ERROR;
> }
>
> -
> void
Unrelated change (and a style bug).
> ngx_shm_free(ngx_shm_t *shm)
> {
> - if (UnmapViewOfFile(shm->addr) == 0) {
> + u_char *addr;
> +
> + addr = shm->addr - sizeof(addr);
> + ngx_log_debug3(NGX_LOG_DEBUG_CORE, shm->log, 0, "shared free \"%V\" -> %p = %p", &shm->name, addr, shm->addr);
> + if (UnmapViewOfFile(addr) == 0) {
> ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno,
> "UnmapViewOfFile(%p) of file mapping \"%V\" failed",
> - shm->addr, &shm->name);
> + addr, &shm->name);
> }
>
> if (CloseHandle(shm->handle) == 0) {
See above, this change shouldn't be needed.
--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
