Skip to content

Fix GH-18417: Windows SHM reattachment fails when increasing memory_consumption or jit_buffer_size #18443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

When a first PHP process launches, Opcache creates a shared file mapping to use as a shm region. The size of this mapping is set by opcache.memory_consumption.
When a new PHP process launches while the old one is still running, Opcache tries to reattach to the shm.
When reattaching it tries to map the requested size (i.e. set by opcache.memory_consumption). However, if the new requested size is larger than the size used in the original file mapping, then the call to VirtualProtect() will fail and the new PHP process will fail to launch.
It's not possible to resize the virtual region on Windows, unless relying on undocumented APIs like NtExtendSection but then we would sitll need to communicate that to the first process.

This issue is the root cause of Psalm end-to-end tests failing in GH-18417: Psalm estimates the required memory sizes and relaunches itself with more memory requested, if its estimate is below the currently allocated shared memory. This causes a crash on startup and the tests fail.

To solve this, we need to make the mappings unique per requested size. There are two ideas:

  1. Include in zend_system_id. However, this also affects other things and may be too overkill.
  2. Include it in the filename, this is an easy local change. I went with this option.

…y_consumption or jit_buffer_size

When a first PHP process launches, Opcache creates a shared file mapping
to use as a shm region. The size of this mapping is set by
opcache.memory_consumption.
When a new PHP process launches while the old one is still running,
Opcache tries to reattach to the shm.
When reattaching it tries to map the requested size (i.e. set by
opcache.memory_consumption). However, if the new requested size is
larger than the size used in the original file mapping, then the call
to VirtualProtect() will fail and the new PHP process will fail to
launch.
It's not possible to resize the virtual region on Windows, unless
relying on undocumented APIs like `NtExtendSection` but then we would
sitll need to communicate that to the first process.

This issue is the root cause of Psalm end-to-end tests failing in
phpGH-18417: Psalm estimates the required memory sizes and relaunches itself
with more memory requested, if its estimate is below the currently allocated
shared memory. This causes a crash on startup and the tests fail.

To solve this, we need to make the mappings unique per requested size.
There are two ideas:
1. Include in zend_system_id. However, this also affects other things
   and may be too overkill.
2. Include it in the filename, this is an easy local change.
   I went with this option.
@nielsdos nielsdos marked this pull request as ready for review April 27, 2025 14:39
@nielsdos nielsdos requested a review from dstogov as a code owner April 27, 2025 14:39
@nielsdos nielsdos requested a review from arnaud-lb April 27, 2025 14:39
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm this fixes the issue, and the fix looks good.

Some thoughts:

  • The opcache.file_cache_fallback setting should have handled this. It looks like it's checking for exactly this issue, but not at the right place. IMO opcache.file_cache_fallback is not a good solution anyway as it randomly results in a slower process.
  • SHM reattachment does not really make sense for the CLI SAPI. We should disable it at least for this SAPI.
  • Eventually we should disable SHM reattachment for all SAPIs (IMO), as it creates problems and complexity (it is the only reason we have some ASLR/WIN32 special cases in inheritance cache, opcache persistence, optimizer, jit, etc)
  • Related discussions: Don't involve PHP in Apache mpm_winnt control process #7865, Bring back isapiSAPI? #17838

@dstogov
Copy link
Member

dstogov commented Apr 28, 2025

* The `opcache.file_cache_fallback` setting should have handled this. It looks like it's checking for exactly this issue, but not at the right place. IMO `opcache.file_cache_fallback` is not a good solution anyway as it randomly results in a slower process.

please point to that check.

* SHM reattachment does not really make sense for the CLI SAPI. We should disable it at least for this SAPI.

agree.

* Eventually we should disable SHM reattachment for all SAPIs (IMO), as it creates problems and complexity (it is the only reason we have some ASLR/WIN32 special cases in inheritance cache, opcache persistence, optimizer, jit, etc)

This is too pessimistic...

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see problems.

@arnaud-lb
Copy link
Member

@dstogov

* The `opcache.file_cache_fallback` setting should have handled this. It looks like it's checking for exactly this issue, but not at the right place. IMO `opcache.file_cache_fallback` is not a good solution anyway as it randomly results in a slower process.

please point to that check.

I may be wrong (didn't verify), but it seems to me that this check:

info.RegionSize < requested_size) {
should be done on the second MapViewOfFileEx as well, and that we should have fallbacked to the file cache after that second MapViewOfFileEx too.

* Eventually we should disable SHM reattachment for all SAPIs (IMO), as it creates problems and complexity (it is the only reason we have some ASLR/WIN32 special cases in inheritance cache, opcache persistence, optimizer, jit, etc)

This is too pessimistic...

To expand a bit on this: I think it disables preloading (I think that's the reason preloading is not supported on windows?) and inheritance cache for sub-classes of internal classes (including enums), it causes JIT to consider that internal classes/functions can be modified, etc.

Of course removing SHM reattachment now would be catastrophic for process-based SAPIs (e.g. FastCGI), but we could change our SAPIs so they don't need it anymore.

The Apache2 SAPI doesn't really benefit from SHM reattachment when configured with a single process and many threads.

The isapi SAPI (#17838) wouldn't need SHM reattachment either.

@nielsdos
Copy link
Member Author

please point to that check.

I may be wrong (didn't verify), but it seems to me that this check:

info.RegionSize < requested_size) {

should be done on the second MapViewOfFileEx as well, and that we should have fallbacked to the file cache after that second MapViewOfFileEx too.

You misinterpret what this check does. This is before the actual mapping happens, where it checks that there's enough space in the virtual address space at the chosen address for the chosen size.

SHM reattachment does not really make sense for the CLI SAPI. We should disable it at least for this SAPI.

The problem is independent of this SAPI though.
Also: are you sure about this? Psalm, phpstan, and some other applications too probably spawn multiple processes in CLI to parallelize work. In that case, they would attach to the same shm. Is this not beneficial?

@arnaud-lb
Copy link
Member

please point to that check.

I may be wrong (didn't verify), but it seems to me that this check:

info.RegionSize < requested_size) {

should be done on the second MapViewOfFileEx as well, and that we should have fallbacked to the file cache after that second MapViewOfFileEx too.

You misinterpret what this check does

Oh, indeed. I was wrong. Thank you!

SHM reattachment does not really make sense for the CLI SAPI. We should disable it at least for this SAPI.

The problem is independent of this SAPI though.

Yes I know. I wasn't sharing this as an alternate way to fix this issue, but just as a general though about SHM reattachment.

Also: are you sure about this? Psalm, phpstan, and some other applications too probably spawn multiple processes in CLI to parallelize work. In that case, they would attach to the same shm. Is this not beneficial?

Good point. I was considering reattachment purely as workaround for the lack of fork() on Windows, and not as a potential benefit. It certainly reduces memory usage of these applications. It may also improve startup time of the processes. On the other hand it will slow down execution a bit (which may be more important than startup time for a long running process). I guess we should measure it. Also, ASLR disables reattachment (we don't re-attach when the address of execute_ex changed, although I admit I don't fully understand ASLR on Windows, as I would have expected execute_ex to always change, but it's not the case here apparently).

@nielsdos nielsdos closed this in 7869af6 Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows SHM reattachment fails when increasing memory_consumption or jit_buffer_size
3 participants