<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN">
<html><body style='font-size: 10pt; font-family: Verdana,Geneva,sans-serif'>
<p>Hello,</p>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">
<pre>There are lots of style problems which need cleanup.</pre>
</blockquote>
<p><span style="font-size: 11.0pt; font-family: 'Calibri','sans-serif'; color: #1f497d;">The newer, "nginx-style" compliant version of changeset (shmem fix2.patch) was already posted to nginx-devel (Thx, Filipe DA SILVA).<br />You will find it <span style="font-size: 11.0pt; font-family: 'Calibri','sans-serif'; color: #1f497d;">also </span>on under <a href="https://github.com/sebres/nginx/commit/e7c149f1ad76b9d850fb59ecc479d4a658c13e04">https://github.com/sebres/nginx/commit/e7c149f1ad76b9d850fb59ecc479d4a658c13e04</a>.</span> </p>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">
<pre>Such things belong to ngx_win32_init.c. It may be also good
enough to use ngx_pagesize, which is already set there.</pre>
</blockquote>
<p> <span style="font-size: 11.0pt; font-family: 'Calibri','sans-serif'; color: #1f497d;">Agree, but some (little) things can reside in same module, where these are used (noted as todo).</span> </p>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">
<pre>This probably should be somewhere at ngx_win32_config.h. </pre>
</blockquote>
<p><span style="font-size: 11.0pt; font-family: 'Calibri','sans-serif'; color: #1f497d;">Imho, belong definitelly in shmem module. But don't want to fight about it :)</span> </p>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">
<pre>It might be better idea to save the address at the end of the
memory block allocated... </pre>
</blockquote>
<p><span style="font-size: 11.0pt; font-family: 'Calibri','sans-serif'; color: #1f497d;">And for example stop to work of other new worker (when unfortunately overwriting of it <span style="font-size: 11.0pt; font-family: 'Calibri','sans-serif'; color: #1f497d;"><span style="font-size: 11.0pt; font-family: 'Calibri','sans-serif'; color: #1f497d;">occurs</span></span>). That brings at once many problem and restrictions <span style="font-size: 11.0pt; font-family: 'Calibri','sans-serif'; color: #1f497d;">later</span> (for example <span style="font-size: 11.0pt; font-family: 'Calibri','sans-serif'; color: #1f497d;"></span>more difficult implementing of <span style="font-size: 11.0pt; font-family: 'Calibri','sans-serif'; color: #1f497d;">shared area </span>resizing, etc). And for what? To save 2 line code for decrement addr pointer in ngx_shm_free? </span></p>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">
<pre>Have you tried using an arbitrary addresses (with subsequent
remap using proper base address for already existing mappings)?
Does is work noticeably worse?</pre>
</blockquote>
<p>I've fought against ASLR since 2008 resp. x64 was released and it was essential. This specific solution is extensively tested and runs in production also without any problems (not only in nginx).</p>
<p>Regards,<br />sebres.</p>
<p> </p>
<p>Am 22.04.2015 18:29, schrieb Maxim Dounin:</p>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;"><!-- node type 8 --><!-- node type 8 --><!-- node type 8 -->
<pre>Hello!
On Wed, Apr 22, 2015 at 09:45:45AM +0200, Sergey Brester wrote:</pre>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">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).</blockquote>
<pre>Interesting, thanks, though the patch certainly needs some
cleanup. See below for some comments.</pre>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">BTW(1): I have interest to fix a problem with multiple workers under windows (ex.: <a href="http://forum.nginx.org/read.php?2,188714,188714#msg-188714">http://forum.nginx.org/read.php?2,188714,188714#msg-188714</a> [1]). @Maxim Dounin: can you possibly give me more information, what you mean here, resp. what it currently depends on (see <a href="http://forum.nginx.org/read.php?2,188714,212568#msg-212568">http://forum.nginx.org/read.php?2,188714,212568#msg-212568</a> [2]).</blockquote>
<pre>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:
<a href="http://msdn.microsoft.com/en-us/library/windows/desktop/ms683463(v=vs.85).aspx">http://msdn.microsoft.com/en-us/library/windows/desktop/ms683463(v=vs.85).aspx</a>
[...]</pre>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;"># HG changeset patch # User Serg G. Brester (sebres) <<a href="mailto:serg.brester@sebres.de">serg.brester@sebres.de</a>> # 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; +}</blockquote>
<pre>Such things belong to ngx_win32_init.c. It may be also good
enough to use ngx_pagesize, which is already set there.</pre>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">+ +/* ------------------- */ +</blockquote>
<pre>There are lots of style problems which need cleanup.</pre>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">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); +</blockquote>
<pre>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).</pre>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">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;</blockquote>
<pre>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.</pre>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">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</blockquote>
<pre>This probably should be somewhere at ngx_win32_config.h.</pre>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">- 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);</blockquote>
<pre>Have you tried using an arbitrary addresses (with subsequent
remap using proper base address for already existing mappings)?
Does is work noticeably worse?</pre>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">+ + 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;</blockquote>
<pre>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.</pre>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">+ } + + 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</blockquote>
<pre>Unrelated change (and a style bug).</pre>
<blockquote style="padding-left: 5px; border-left: #1010ff 2px solid; margin-left: 5px;">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) {</blockquote>
<pre>See above, this change shouldn't be needed.
</pre>
</blockquote>
</body></html>