php / php-src

The PHP Interpreter
https://www.php.net
Other
38.16k stars 7.75k forks source link

PHP 8.3 not working on Windows 7 #12762

Closed nono303 closed 11 months ago

nono303 commented 11 months ago

Description

Hi, Just trying to run PHP 8.3.0 on Windows 7 (after successfully compiling it on Win11 vs17 x64) and it's failed with message

image

According to https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getcurrentthreadstacklimits it's normal as requirements are:

It had also been clearly mentioned on https://github.com/php/php-src/pull/9104#issuecomment-1627481475
Looking to the source code, I found GetCurrentThreadStackLimits used in Zend/zend_call_stack.c

Based on https://stackoverflow.com/a/52404641 I dirtily applied the patch and now PHP 8.3.0 run fine on windows 7

diff --git "a/Zend/zend_call_stack.c" "b/Zend/zend_call_stack.c"
index 06ee521911..9fc2b570bc 100644
--- "a/Zend/zend_call_stack.c"
+++ "b/Zend/zend_call_stack.c"
@@ -376,7 +376,22 @@ static bool zend_call_stack_get_win32(zend_call_stack *stack)
     *                v  Lower addresses   v
     */

-   GetCurrentThreadStackLimits(&low_limit, &high_limit);
+   static void (WINAPI* GetCurrentThreadStackLimits)(PULONG_PTR , PULONG_PTR);
+   if (!GetCurrentThreadStackLimits) {
+       *(void**)&GetCurrentThreadStackLimits = GetProcAddress(GetModuleHandle(L"kernel32"), "GetCurrentThreadStackLimits");
+       if (!GetCurrentThreadStackLimits) {
+           NT_TIB* tib = (NT_TIB*)NtCurrentTeb();
+           high_limit = (ULONG_PTR)tib->StackBase;
+           MEMORY_BASIC_INFORMATION mbi;
+           if (VirtualQuery(tib->StackLimit, &mbi, sizeof(mbi))) {
+               low_limit = (ULONG_PTR)mbi.AllocationBase;
+           }
+       } else {
+           GetCurrentThreadStackLimits(&low_limit, &high_limit);
+       }
+   } else {
+       GetCurrentThreadStackLimits(&low_limit, &high_limit);
+   }

    result_size = VirtualQuery((void*)low_limit,
            &uncommitted_region, sizeof(uncommitted_region));

image

So... Does this patch (or a corrected one as I don't really master all the elements) would make sense to ensure PHP a long life on old Windows OS ?

PHP Version

PHP 8.3.0

Operating System

Windows 7 x64

nielsdos commented 11 months ago

Windows 7 has been out of support for almost 4 years. Windows server 2008 r2 is out of support except on some azure services where support will end in 2 months. I don't personally believe we should support outdated/unsupported operating systems.

kallesommernielsen commented 11 months ago

This is an intentional change and also listed in the upgrade guide for 8.3

nono303 commented 11 months ago

This is an intentional change and also listed in the upgrade guide for 8.3

Thx! I didn't not see https://www.php.net/manual/en/migration83.windows-support.php I just referred to https://www.php.net/ChangeLog-8.php#PHP_8_3 which seems not have this intentional change in the list...

However, if GetCurrentThreadStackLimits is the only blocker to use 8.3 on an well-known EOL OS, could this patch had a chance to be considered & qualified ?

nielsdos commented 11 months ago

However, if GetCurrentThreadStackLimits is the only blocker to use 8.3 on an well-known EOL OS, could this patch had a chance to be considered & qualified ?

What I don't like about the current patch is how it will keep retrying GetProcAddress even if the first attempt failed.

Not sure how others feel about supporting unsupported OS.

nielsdos commented 11 months ago

I am going to close this as wontfix. The reason is that I don't think we should support obsolete operating systems that are not even supported anymore by its developers. And also because the procedure fetching can cause slowdowns.

bukka commented 11 months ago

I agree, we should no longer support Windows 7 in new versions. We shoud keep compatibility on 8.2 but it's fine if 8.3 will no longer support it.

nono303 commented 11 months ago

Hi, Thx for yours feedback, fully agree on your statement. As I'll use this patch for my own, I've dug a little bit on it and here is a qualified & tested version I just write it here for the posterity if others would like to use it with some background

  1. I reviewed the approach of the nested if and fix it according to what is actually done in https://github.com/php/php-src/blob/master/win32/time.c#L33
    • Normally, all properties are retrieved by reference and nothing need to be freed
  2. I put some debug info to check
    • Non regression for supported OS
    • values accuracy between the two methods on supported OS
    • eventual time overhead on LoadLibrary & GetProcAddress
  3. Benching difference between 8.2.13 and 8.3.0
    • Run https://github.com/php/php-src/blob/master/Zend/micro_bench.php

      Ref. 2023-11-29 1.136 [8.3.0 - jit 1254] Ref. 2022-12-07 1.123 [8.2.0 - jit 1254]

    • Launch my own benchmark on a php WMTS server (16000 hits parallelized by 32, on an old i5 sandybridge with some compute, gd, regexp…)
      • almost the same
        • Memory footprint (privates bytes and working set)
        • cpu (sys & usr)
        • total time

Final

diff --git "a/Zend/zend_call_stack.c" "b/Zend/zend_call_stack.c"
index 06ee521911..0ac230a0bd 100644
--- "a/Zend/zend_call_stack.c"
+++ "b/Zend/zend_call_stack.c"
@@ -376,7 +376,19 @@ static bool zend_call_stack_get_win32(zend_call_stack *stack)
     *                v  Lower addresses   v
     */

-   GetCurrentThreadStackLimits(&low_limit, &high_limit);
+   typedef void (WINAPI* FuncT)(PULONG_PTR , PULONG_PTR);
+   HINSTANCE hDLL = LoadLibrary("Kernel32.dll");
+   FuncT GetCurrentThreadStackLimits = (FuncT) GetProcAddress((HMODULE)hDLL, "GetCurrentThreadStackLimits");
+   if (GetCurrentThreadStackLimits) {
+       GetCurrentThreadStackLimits(&low_limit, &high_limit);
+   } else {
+       NT_TIB* tib = (NT_TIB*)NtCurrentTeb();
+       high_limit = (ULONG_PTR)tib->StackBase;
+       MEMORY_BASIC_INFORMATION mbi;
+       if (VirtualQuery(tib->StackLimit, &mbi, sizeof(mbi))) {
+           low_limit = (ULONG_PTR)mbi.AllocationBase;
+       }
+   }

    result_size = VirtualQuery((void*)low_limit,
            &uncommitted_region, sizeof(uncommitted_region));

Debug

diff --git "a/Zend/zend_call_stack.c" "b/Zend/zend_call_stack.c"
index 06ee521911..eebf45feb5 100644
--- "a/Zend/zend_call_stack.c"
+++ "b/Zend/zend_call_stack.c"
@@ -23,6 +23,7 @@
 #include "zend_portability.h"
 #include "zend_call_stack.h"
 #include <stdint.h>
+#include <time.h>
 #ifdef ZEND_WIN32
 # include <processthreadsapi.h>
 # include <memoryapi.h>
@@ -376,7 +377,37 @@ static bool zend_call_stack_get_win32(zend_call_stack *stack)
     *                v  Lower addresses   v
     */

-   GetCurrentThreadStackLimits(&low_limit, &high_limit);
+   clock_t begin = clock();
+   typedef void (WINAPI* FuncT)(PULONG_PTR , PULONG_PTR);
+   HINSTANCE hDLL = LoadLibrary("Kernel32.dll");
+   FuncT GetCurrentThreadStackLimits = (FuncT) GetProcAddress((HMODULE)hDLL, "GetCurrentThreadStackLimits");
+   if (GetCurrentThreadStackLimits) {
+       GetCurrentThreadStackLimits(&low_limit, &high_limit);
+       ULONG_PTR tmp_low_limit, tmp_high_limit;
+       NT_TIB* tib = (NT_TIB*)NtCurrentTeb();
+       tmp_high_limit = (ULONG_PTR)tib->StackBase;
+       MEMORY_BASIC_INFORMATION mbi;
+       if (VirtualQuery(tib->StackLimit, &mbi, sizeof(mbi))) {
+           tmp_low_limit = (ULONG_PTR)mbi.AllocationBase;
+       }
+       fprintf(stderr, "GetCurrentThreadStackLimits (%lu =? %lu) (%lu =? %lu)\n",(uintptr_t)low_limit, (uintptr_t)tmp_low_limit,(uintptr_t)high_limit,(uintptr_t)tmp_high_limit);
+       fprintf(stderr, "GetCurrentThreadStackLimits %lu %lu\n",(uintptr_t)low_limit,(uintptr_t)high_limit);
+   } else {
+       NT_TIB* tib = (NT_TIB*)NtCurrentTeb();
+       high_limit = (ULONG_PTR)tib->StackBase;
+       MEMORY_BASIC_INFORMATION mbi;
+       if (VirtualQuery(tib->StackLimit, &mbi, sizeof(mbi))) {
+           low_limit = (ULONG_PTR)mbi.AllocationBase;
+       }
+       fprintf(stderr, "StackLimitStackBase %lu %lu\n",(uintptr_t)low_limit, (uintptr_t)high_limit);
+   }
+   clock_t end = clock();
+   unsigned long millis = (end -  begin) * 1000 / CLOCKS_PER_SEC;
+   fprintf(stderr, "time_spent %lu\n",millis);

    result_size = VirtualQuery((void*)low_limit,
            &uncommitted_region, sizeof(uncommitted_region));
kangarko commented 10 months ago

Less than 50 lines of code needed to support an entire operating system? I think that's an insult to the entire Win 7 community.

staabm commented 10 months ago

making it compile is one thing. maintaining it and hunting upcoming bugs is the time consuming story - at the cost of having less time for the people doing a great job in using modern systems

kangarko commented 10 months ago

I completely understand. I meant Windows 7 is still quite in use and as long as those were minor changes like these I think that those extra few minutes are justified.

Of course later as more and more things will break it will come naturally to drop its compatibility, but as far I saw so far it's quite simple to maintain.

nokotto commented 5 months ago

Less than 50 lines of code needed to support an entire operating system... even if it were 200 lines, it sounds like a bad joke. Six months... , two weeks since this push.

May we keep using PHP in Windows 7, please.

nielsdos commented 5 months ago

May we keep using PHP in Windows 7, please.

It's also about keeping those lines up to date and tested. Windows 7 should not be used anyway due to it not receiving security fixes anymore. Feel free to apply the patch yourself and perform your own builds.

nokotto commented 5 months ago

If any of you encounters an advertising pop-up after Windows 11 has scanned your personal data and activities, what is shared with no less than another thousand companies, I trust you will have no objection, because it is a modern system and modernity.

On Windows 10, advanced prototype of such spyware era, very soon a campaign will be activated in an attempt to pressure all of you to migrate with the same seeds used against Windows 7 under the "it's an out of support OS!" umbrella. In many cases, this will end with all of you buying unneeded hardware, even a complete computer, and once again, I trust that none of you will have any objections.

In fact, I trust that you will all have no objections with the long updates blocking the system when you need it, or the general performance with unjustified services that will be re-enabled until you notice your setting was overrided.

But some of us don't want the above mentioned problems, or don't want any other related ones (I guess each person uses Windows 7 under their own reasons, from hardware performance or so on).

Windows 7 should not be used anyway due to it not receiving security fixes anymore.

I think the people using Windows 7 -version where all the ports can be closed with a firewall of our election and the system still works- already knows that it is a insecure system, as insecure as the last updated of the promoted Windows. If an infected file gets into the system, there is nothing you can do. If you have opened the ports of the Windows services, there is nothing you can do, zero-day will come to you.

It's also about keeping those lines up to date and tested.

If a careless modification of those lines fails in Windows 7, it is my guess that someone will notice, come, and try to suggest a solution.

nokotto commented 5 months ago

Feel free to apply the patch yourself and perform your own builds.

Forcing everyone to build is anything but freedom.

nielsdos commented 5 months ago

First, I agree that Microsoft is building spyware and bloat into their operating system. I've used Windows XP for years because Windows 7 had such a hardware requirement bump, until XP went out of support and I was forced to switch away. So I do get the argument of bloat + spyware. AFAIK there are 3rd party tools that can strip off unnecessary and spyware components of Windows.

Note that just blocking off ports on Windows 7 is not a solution because vulnerabilities such as buffer overflows and other nasty stuff don't necessarily need to be in the applications you use, they can very well be in the TCP/IP layer of the kernel for example. In that case a firewall isn't going to protect you necessarily.

Forcing everyone to build is anything but freedom.

No, it is freedom, and a lot of freedom in fact. The source code is available and you can do anything with it and change it to your likings (within the conditions of the license). How is this not freedom?

This thread isn't going anywhere and I'm pretty sure the decision of the people who replied here are final. It'll just be a back and forth between users and maintainers that leads to nowhere. So I'm locking this to reduce the notifications sent to everyone. If you want change, you'll have to go through the RFC process.