[PATCH 8 of 9] Upstream: disable re-resolve functionality on Windows

[Aleksei Bavshin]

On 7/10/2024 6:16 AM, Roman Arutyunyan wrote:
> Hi,
> 
> On Thu, Jun 13, 2024 at 03:29:03PM -0700, Aleksei Bavshin wrote:
>> # HG changeset patch
>> # User Aleksei Bavshin <a.bavshin at nginx.com>
>> # Date 1712181327 25200
>> #      Wed Apr 03 14:55:27 2024 -0700
>> # Node ID 375fa42f1a6010692a8782c4f03c6ad465d3f7f7
>> # Parent  8c8d8118c7ac0a0426f48dbfed94e279dddff992
>> Upstream: disable re-resolve functionality on Windows.
>>
>> Following features are currently not implemented on Windows, making re-resolve
>> functionality unsafe to use:
>>
>>   * 'noreuse' shared zones that are re-created on each configuration reload.
>>     The work scheduling logic is not prepared to handle simultaneous access to
>>     the shared zone from multiple generations of the worker processes.
> 
> I don't see a problem here.  Could you please elaborate.

Now I recall that I could've been running a modified version with 
different handling of `noreuse` when I wrote that.

The problem is still valid; assuming that `noreuse` is simply ignored on 
Windows, we could've get the following behavior:
A new cycle is created on configuration reload, and an existing shared 
zone is referenced by the new cycle. Depending on how we handle that, 
`ngx_http_upstream_init_zone` in the new cycle either will find an 
existing set of peers (still actively used by a previous cycle) and 
overwrite host->event fields with worker-local pointers, or will 
allocate a new `ngx_http_upstream_rr_peers_t` and eventually exhausts 
the zone memory.

Thankfully, neither option is possible and `noreuse` does something on 
Windows platform: an upstream zone in the configuration will completely 
break configuration reload with

     2024/07/10 13:13:12 [alert] 41332#45116: MapViewOfFileEx(65536, 
2EFE0000) of file mapping "upstream_zone" failed (487: Attempt to access 
invalid address)

What's happening there is that we see the 'noreuse' flag and attempt to 
allocate a new shared zone. Due to a platform-specific implementation 
detail, we get `ERROR_ALREADY_EXISTS` and proceed with the existing file 
mapping object mapped at a different address.

Next, the code added in af7eba90645d will notice that the newly mapped 
address does not match the expected address of an existing zone and will 
attempt to remap it at the original address via ngx_shm_remap.

As we're in the master process and the original mapping still exists at 
that address, `MapViewOfFileEx` would fail and abort the configuration 
reload.

Note that you don't even need the patches in this series to reproduce that.

***

Overall, I'm convinced that the problem is not in the resolver but in 
the `noreuse` zone type we use in upstreams. And we already allow these 
on Windows, so the ship has sailed.

I saw a previous idea to resolve that by allocating `noreuse` zones with 
a name unique to the config generation and tried implementing that, but 
it wasn't beautiful.

Thus, I'm retracting this patch. Let's just update the Windows platform 
limitations in the documentation instead.

> 
>>   * 'ngx_worker' identification.
>>     It is possible to configure multiple worker processes on Windows, even if
>>     only one would actually handle the traffic.  All of the worker processes are
>>     currently identified as process 0, breaking scheduling and locking of the
>>     resolver tasks.
> 
> This can be fixed.  Patch attached.

Note that the patch is insufficient, as it does not handle respawning.

We should store the ngx_worker value in the ngx_processes and restore 
the variable when necessary, or find an alternative way of passing it to 
the worker process.
My WIP patch with `cycle->generation` support had an attempt to solve 
that with making environment snapshots (GetEnvironmentStrings) and 
passing these to the CreateProcess, but that quickly escalated into a 
ton of unrelated fixes and I decided to stop.

> 
>> diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
>> --- a/src/http/ngx_http_upstream.c
>> +++ b/src/http/ngx_http_upstream.c
>> @@ -6327,7 +6327,7 @@ ngx_http_upstream_server(ngx_conf_t *cf,
>>               continue;
>>           }
>>   
>> -#if (NGX_HTTP_UPSTREAM_ZONE)
>> +#if (NGX_HTTP_UPSTREAM_ZONE && !(NGX_WIN32))
>>           if (ngx_strcmp(value[i].data, "resolve") == 0) {
>>               resolve = 1;
>>               continue;
>> diff --git a/src/stream/ngx_stream_upstream.c b/src/stream/ngx_stream_upstream.c
>> --- a/src/stream/ngx_stream_upstream.c
>> +++ b/src/stream/ngx_stream_upstream.c
>> @@ -545,7 +545,7 @@ ngx_stream_upstream_server(ngx_conf_t *c
>>               continue;
>>           }
>>   
>> -#if (NGX_STREAM_UPSTREAM_ZONE)
>> +#if (NGX_STREAM_UPSTREAM_ZONE && !(NGX_WIN32))
>>           if (ngx_strcmp(value[i].data, "resolve") == 0) {
>>               resolve = 1;
>>               continue;
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel at nginx.org
>> https://mailman.nginx.org/mailman/listinfo/nginx-devel
> 
> --
> Roman Arutyunyan
> 
> 
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel