Core: invalidated shared memory addresses after unmapping. #737
Conversation
src/os/unix/ngx_shmem.h
Outdated
| #if (NGX_HAVE_MAP_ANON || NGX_HAVE_MAP_DEVZERO) | ||
| #define NGX_INVALID_SHM MAP_FAILED | ||
| #elif (NGX_HAVE_SYSVSHM) | ||
| #define NGX_INVALID_SHM (void *) -1 | ||
| #endif |
There was a problem hiding this comment.
NGX_INVALID_SHM value here is chosen for consistency with the state after a failed ngx_shm_alloc(). Alloc may leave the address set to MAP_FAILED, thus we have to check for this value anyways.
Coincidentally, the error path in ngx_init_cycle() is not ready to handle MAP_FAILED left by the allocation failure and will attempt to ngx_shm_free() it. Seems like a good reason to consistently reset the addr to NULL.
There was a problem hiding this comment.
The last point is indeed true. Failed allocations end up with followup unmap errors.
limit_req_zone 1 zone=foo:100000000m rate=1r/s;
Log:
2025/07/15 20:26:19 [alert] 3588934#0: mmap(MAP_ANON|MAP_SHARED, 104857600000000) failed (12: Cannot allocate memory)
2025/07/15 20:26:19 [alert] 3588934#0: munmap(FFFFFFFFFFFFFFFF, 104857600000000) failed (22: Invalid argument)
This needs to be fixed for sure by resetting shm->addr to NULL.
|
Hi @bavshin-f5 Rather than introducing a new macro, you could just reuse If you do want to introduce a new macro, I'd alias it to While unlikely, it is possible for mmap(2) to return address Same reason mmap(2) doesn't return |
|
There are two configurations where we consider There's also at least one location where we considered I don't have a strong opinion on the value though. That's why there are two commits; the first one does exactly what you suggest and the second one can be discarded. |
POSIX says this about non-fixed allocations: Also, POSIX does not say anything about Anyway, it looks like using |
|
Regarding the cleanup handlers which may access the freed memory and require an additional reset in |
There are no such handlers in the nginx code at the moment. I attempted to add one as noted in the PR description and encountered the problem while removing the shared zone from the configuration. Ideally, I'd also love to have a
So |
Yes, Regarding the handler, the case looks unusual, hence the question. Typically a module allocates shared memory and then uses slab allocator to allocate data structures within the shared memory. The pointers are typically named |
As we unmap the unused shared zones before destroying the cycle pool, it was possible to access already unmapped address from a cleanup handler. The change gives such handlers a way to determine that the memory is unavailable and any processing should be skipped.
It also requires the Even with
More or less, as it stands from what I can tell we never call Still, nginx does support quite a lot of legacy type stuff... |
As we unmap the unused shared zones before destroying the cycle pool, it was possible to access already unmapped address from a cleanup handler. The change gives such handlers a way to determine that the memory is unavailable and any processing should be skipped.
I encountered the issue while attempting to discard unused data from a shared zone. The most suitable location for such code is a cleanup handler on the cycle pool: it's a bit early with the old cycle workers still running and potentially using the data, but we already know that the new cycle is going live and its shared data is initialized. And we don't have anything reliable beyond that point.
Currently, this pattern is extremely dangerous, as the handler may accidentally touch an unmapped address and crash the master process. There are a few indirect ways of testing if an address is mapped, but most of those are non-portable and worse than the offered patch.
In the end, I had to go back to a proven solution with non-reusable zones. But it would be nice to figure out an alternative to these, even if it takes several small changes like the current one.