Skip to content

Fix minimal Windows version #15975

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
Closed

Fix minimal Windows version #15975

wants to merge 1 commit into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Sep 21, 2024

As of PHP 8.3.0, Windows 8/Server 2012 are the minimum requirement. However, PR #9104 only updated _WIN32_WINNT, but not WINVER[1], NTDDI_VERSION[2] nor the manifest[3].

[1] https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers
[2] https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers
[3] https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests

As of PHP 8.3.0, Windows 8/Server 2012 are the minimum requirement.
However, PR php#9104 only updated `_WIN32_WINNT`, but not `WINVER`[1],
`NTDDI_VERSION`[2] nor the manifest[3].

[1] <https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers>
[2] <https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers>
[3] <https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests>
@cmb69 cmb69 closed this in 5bcbe8a Sep 22, 2024
@cmb69 cmb69 deleted the cmb/winver branch September 22, 2024 17:31
@cmb69
Copy link
Member Author

cmb69 commented Sep 22, 2024

Only now noticed the "ABI break" label, but I think there is no ABI break for practical purposes. Shall I revert the commit from PHP-8.3 nonetheless?

@nielsdos
Copy link
Member

I don't know, it doesn't run on Windows 7 anyway, but who knows who uses NTDDI_VERSION ?

@cmb69
Copy link
Member Author

cmb69 commented Sep 22, 2024

I don't know, it doesn't run on Windows 7 anyway, […]

I wonder what happens exactly (without this patch). Does it try to start up, and then reports a missing function? Or would it crash?

The change to the manifest should prevent starting up in the first place.

, but who knows who uses NTDDI_VERSION ?

I think WINVER (which is passed as define via CFLAGS) is more relevant, but I guess nobody uses either. Couldn't find any usage in the php organization, nor in win32service.

@cmb69
Copy link
Member Author

cmb69 commented Sep 22, 2024

@bukka, would you consider this change to be a problem for PHP-8.3?

@nielsdos
Copy link
Member

I wonder what happens exactly (without this patch). Does it try to start up, and then reports a missing function? Or would it crash?

It does not complete startup, once you launch the EXE you get a "missing procedure" error from Windows.

@bukka
Copy link
Member

bukka commented Sep 22, 2024

@cmb69 I will trust your judgement on this. I don't mind to leave it in 8.3 if you think it's safe.

@cmb69
Copy link
Member Author

cmb69 commented Sep 24, 2024

I'm lacking the imagination for what these constants could be explicitly used. I mean, clearly so far somebody could have build an extension which works on Windows 7, but when trying to start PHP to use it, it would fail to start anyway.

Of course, someone might use WINVER to define the upper bound of an array in an extension, but then accessing [WINVER] would be a buffer overflow. But why would anyone do this?

I'd say this change is not guilty, until proven otherwise.

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.

3 participants