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

Check if a given PHP Binary is actually valid #25

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

asgrim
Copy link
Collaborator

@asgrim asgrim commented Aug 2, 2024

Fixes #12

@asgrim asgrim added the enhancement New feature or request label Aug 2, 2024
@asgrim asgrim self-assigned this Aug 2, 2024
@asgrim asgrim force-pushed the check-if-given-php-binary-is-valid branch from 5773317 to 1fc2996 Compare August 13, 2024 13:05
@asgrim asgrim force-pushed the check-if-given-php-binary-is-valid branch from 1fc2996 to df28f0c Compare August 13, 2024 13:46
@asgrim asgrim merged commit d96c349 into php:main Aug 13, 2024
14 checks passed
@asgrim asgrim deleted the check-if-given-php-binary-is-valid branch August 13, 2024 13:53
@ghostwriter
Copy link

You are using the wrong directory separator on windows, possibly causing is_executable to “always” return false.

@asgrim
Copy link
Collaborator Author

asgrim commented Aug 13, 2024

You are using the wrong directory separator on windows, possibly causing is_executable to “always” return false.

Hmm, looking at this test:

https://github.com/php/php-src/blob/589cfbb24fde06836df9dbb50ee0093a69c6eeba/ext/standard/tests/file/windows_acls/bug44859_3.phpt#L13-L31

The commented-out tiny.bat suggests that the .bat workaround that I tried won't work. I think it would have to be a true executable on Windows. That suggests the doc:

On Windows, a file is considered executable, if it is a properly executable file as reported by the Win API GetBinaryType();

is correct, but:

for BC reasons, files with a .bat or .cmd extension are also considered executable.

is outdated.

@ghostwriter
Copy link

I’m talking about the path you hardcoded in the test.

image

/ for Unix-based systems (Linux, macOS).

\ for Windows.

use DIRECTORY_SEPARATOR to automatically use the correct one for the platform at runtime.

@asgrim
Copy link
Collaborator Author

asgrim commented Aug 13, 2024

I’m talking about the path you hardcoded in the test.
image

/ for Unix-based systems (Linux, macOS).

\ for Windows.

use DIRECTORY_SEPARATOR to automatically use the correct one for the platform at runtime.

I understand, but that is not the issue, I tried backslashes too 👍

@ghostwriter
Copy link

The path: "z:\phpf\pie\test\unit\Platform\TargetPhp/../../assets/fake-php-windows.bat”

using both \ and / in the path.

image

@asgrim
Copy link
Collaborator Author

asgrim commented Aug 13, 2024

The path: "z:\phpf\pie\test\unit\Platform\TargetPhp/../../assets/fake-php-windows.bat”

using both \ and / in the path.

image

Yes, I understand what you're saying, but I tried fixing the slashes since my earlier stream, and that is still not the issue. I explored the code some more, and the .bat/.cmd thing does not appear hold true.

@ghostwriter
Copy link

I see, "normalizing" the path was just the first step.


… the .bat/.cmd thing does not appear hold true.

you are correct, it does not appear hold true in this case, but the code does reflect what is documented.

https://github.com/php/php-src/blob/PHP-8.3.11/win32/ioutil.c#L924-L940


My understanding was, and please correct me If i’m wrong, even if we change the file extension, on windows, if the file’s internal format is NOT recognized as executable, is_executable() will return FALSE (except for file paths with *.bat and *.cmd extension).

Executability is determined by:

Windows returns TRUE:

  • If the given file path exists and has a *.exe or *.com extension, with the PE format and internal structure that is recognized as executable usingGetBinaryTypeW.

or

  • If the given file path exists and has a *.bat or *.cmd extension

Unix-like Systems returns TRUE:

  • If the given file path exists and the file has the executable bit set.

I would not say outdated but there might be an undocumented behavior/s when we set the file type and permissions or when checking executable permission or on files with empty DACLs.

(empty DACL: no permissions explicitly granted or denied to anyone, effectively making the file inaccessible to all users, including the file's owner.)


P.S. Thanks for being patient and indulging my curiosity.

@asgrim asgrim added this to the 0.1.0 - initial release milestone Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verify that the PhpBinaryPath created is really a PHP
3 participants