Skip to content
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

PHP 8.3 not working on Windows 7 #12762

Closed
nono303 opened this issue Nov 24, 2023 · 15 comments
Closed

PHP 8.3 not working on Windows 7 #12762

nono303 opened this issue Nov 24, 2023 · 15 comments

Comments

@nono303
Copy link
Contributor

nono303 commented Nov 24, 2023

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:

  • Minimum supported client Windows 8 [desktop apps | UWP apps]
  • Minimum supported server Windows Server 2012 [desktop apps | UWP apps]

It had also been clearly mentioned on #9104 (comment)
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
Copy link
Member

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
Copy link

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

@nono303
Copy link
Contributor Author

nono303 commented Nov 24, 2023

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
Copy link
Member

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
Copy link
Member

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.

@nielsdos nielsdos closed this as not planned Won't fix, can't repro, duplicate, stale Nov 24, 2023
@bukka
Copy link
Member

bukka commented Nov 24, 2023

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
Copy link
Contributor Author

nono303 commented Nov 29, 2023

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));

nono303 pushed a commit to nono303/win-build-scripts that referenced this issue Nov 29, 2023
@kangarko
Copy link

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
Copy link
Contributor

staabm commented Dec 21, 2023

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
Copy link

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.

nono303 pushed a commit to nono303/win-build-scripts that referenced this issue Apr 24, 2024
@nokotto
Copy link

nokotto commented May 10, 2024

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
Copy link
Member

nielsdos commented May 10, 2024

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
Copy link

nokotto commented May 10, 2024

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
Copy link

nokotto commented May 10, 2024

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

Forcing everyone to build is anything but freedom.

@nielsdos
Copy link
Member

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.

@php php locked as resolved and limited conversation to collaborators May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants