@cmb69 Why do you think C: is an absolute path? According to https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats it is not absolute and if you run cd C: in Windows command line the path do not change to C: neither. C:/ or C:\ paths are absolute.

C: test in C#: https://dotnetfiddle.net/y6w0Eu

using System;
using System.IO;				

public class Program
{
	public static void Main()
	{
		Console.WriteLine(Path.GetFullPath(@"C:"));
		Console.WriteLine(Path.GetFullPath(@"C:/"));
	}
}

output:

C:\Resources\directory\64b308a5de824a85b48a86202ba65723.DotNetFiddle.Web.LocalResources\Sandbox\e1338a4e-4596-4049-88f8-3431465a922d
C:\

@cmb69 code updated, now only this test is failing:

• Phar - Bug #71625 - Crash in php7.dll [C:\projects\php-src\ext\phar\tests\bug71625.phpt]
  local output:

Warning: Phar::offsetSet(C:/phpbuild/php-src/x64/Release_TS/A:A:.phar): failed to open stream: No such file or directory in C:\phpbuild\php-src\ext\phar\tests\bug71625.phpt on line 17

Fatal error: Uncaught PharException: unable to open new phar "C:/phpbuild/php-src/x64/Release_TS/A:A:.phar" for writing in C:\phpbuild\php-src\ext\phar\tests\bug71625.phpt:17
Stack trace:
#0 C:\phpbuild\php-src\ext\phar\tests\bug71625.phpt(17): Phar->offsetSet('hello_habr.txt', '<? Hello Habr!?...')
#1 {main}
  thrown in C:\phpbuild\php-src\ext\phar\tests\bug71625.phpt on line 17

It seems that the bug #71625 was solved with this PR, how to update this test?

Why do you think C: is an absolute path?

Because I misinterpreted a quick test, and didn't read the docs thoroughly, which state that C:Projects\apilibrary\apilibrary.sln is "a relative path from the current directory of the C: drive." So apparently each drive has its own current directory. Let's check:

@echo off

:: create test folder layout, unless already there
for %%i in (C:\test5001 C:\test5001\c D:\test5001 D:\test5001\d) do (
    if not exist %%i mkdir %%i
)

:: set cwd for each drive
cd C:\test5001
cd D:\test5001

:: verify cwd is set independently for each drive
C:
echo %CD%
D:
echo %CD%

:: check PHP's behavior
php -r "var_dump(ZEND_THREAD_SAFE)";
php -r "var_dump(file_exists('C:c'));"
php -r "var_dump(realpath('C:c'));"

I get

C:\test5001
D:\test5001
bool(true)
bool(false)
bool(false)

and

C:\test5001
D:\test5001
bool(false)
bool(true)
bool(false)

So realpath() (and scandir(); I didn't check other functions) do not support drive relative paths at all; for NTS builds, the behavior is inconsistent wrt. file_exists(). Anyhow, PHP, and particularly TSRM, has no notion of a current working directory per drive, so supporting this is hardly possible, and certainly not something that we'd like to introduce in a stable version. Instead we may consider to let file_exists() fail for drive relative paths in NTS builds as well. And of course, we should document that.

So apparently each drive has its own current directory.

According to https://devblogs.microsoft.com/oldnewthing/20101011-00/?p=12563 there should be only one current working directory and the behaviour in cmd should be considered cmd only. And the linked article (link in the previous article does not work) https://devblogs.microsoft.com/oldnewthing/20100506-00/?p=14133

So if there is only one cwd and this cwd is very well set/reset by php (otherwise standard relative paths will not work) and if relative links are properly detected (by this PR), all relative links should work if the resolution is done by the OS itself.

The different results of file_exists (and also of all other file functions) should be fixed by this commit to behave exactly the same between TS and NTS build (prior this PR one build used buggy IS_ABSOLUTE_PATH). Did you run your code with PHP with this PR patch?

Additionally, this PR fixes:

• https://github.com/php/php-src/pull/5001/files#diff-6348a66bca1dbd140e8886d403f07f40L1293 - fix no longer needed as code with the updated IS_ABSOLUTE_PATH is never needed as the topmost condition is never satisfied
• https://github.com/php/php-src/pull/5001/files#diff-093fc4d3c1dfbb6819809690e36128d7R1265 in recursive mkdir PHP tries to figure the first parent existing directory using specific code, it is covered by tests and I fixed it

It seems that the bug #71625 was solved with this PR, how to update this test?

How should I fix this test? The bug is about A:A filename, which no longer resolves to absolute path and the nature of the bug changed - this filename is now just like any other unsuported filename (colon is not allowed in filenames on Windows). Should the test be simply updated with the new exception message or what is the standard process to fix these tests when the tested bug is no longer presented?

Instead we may consider to let file_exists() fail for drive relative paths in NTS builds as well. And of course, we should document that.

This was my first thought too and I support it. Even Microsoft is writing in every 2nd line that this is common source of errors. But for now I would propose to fix the original issue, i.e. wrong IS_ABSOLUTE_PATH.

@cmb69 Can you please review the changes and merge?

If there are no any other questions, the PR is ready to be merged.

<?php
var_dump(chdir('C:/'));
var_dump(file_exists('C:/test5001'));
var_dump(file_exists('C:test5001'));

prints now for NTS:

bool(true)
bool(true)
bool(false)

and for ZTS:

bool(true)
bool(true)
bool(true)

That doesn't look like the desired output. What am I missing?

@cmb69 Please add a test case and I will try to make the code to pass.

Pretty strange, but I can no longer reproduce the different behavior of NTS vs. ZTS with or without this patch. Maybe that is related to the latest Windows updates I got yesterday?

Anyhow, I think it's best to start with a simple test, just to show the desired change:

--TEST--
Bug #78939 (Windows relative path with drive letter)
--SKIPIF--
<?php
if (PHP_OS_FAMILY !== 'Windows') die('skip this test is for Windows platforms only');
?>
--FILE--
<?php
list($drive, $topdir) = (explode('\\', __FILE__, 3));
var_dump(chdir("$drive\\"));
var_dump(file_exists("$drive\\$topdir"));
var_dump(file_exists("$drive$topdir"));
?>
--EXPECT--
bool(true)
bool(true)
bool(false)

This passes for me with the patch, while versions without this patch would have printed bool(true) three times.

And please rebase onto any of the actively supported PHP versions or master.

@cmb69 I have added a test case.

I have also found that file_exists (exists functions in general) and is_dir (is some file type functions) is processed differently. I have identified the cause and fixed that code as well. Even if I do not like the relative Windows paths, they should now work as specified by Microsoft on both TS & NTS builds and the PR is complete now.

@cmb69 Can you review and merge to close it?

The patch doesn't look bad, but should have also tests to actually check whether drive relative paths work as expected for different file system functions (e.g. fopen(), file_get_contents()), but also for other functions which could be affected (cf. 3d3f11e). Also there should be tests regarding the behavior of open_basedir, but that may not be possible. It also would be good to have which show the behavior of changing to a drive relative path on another drive, but that may also be a bit of an issue (test must be skipped if other drive is not available; how to get info of available drives?) If automated tests are not possible, this would have to be tested manually.

Standard system functions use the same underlaying path resolution. Feel free to add more tests if needed, same for the manual multidrive test if needed.

Anyhow, neither I nor the PHP 7.4 RM @derickr would like to see this patch going into the release branches for stability reason. Therefore, please rebase onto master.

Rebased and changed the merge target. All tests passed.

@cmb69 , @nikic Is this PR welcomed?

Sorry, I don't have familiarity with Windows.

Thanks for the ping, @cmb69. I would recommend against implementing this feature. We might not be able to guarantee the drive relative path is safe. There are similar issues with resolving symlinks, thanks for reminding on #1243 which looks quite similar . What comes in mind that needs to be tested

• thread safe SAPI, in particular Apache with a good concurrency level
• open_basedir compatibility
• chdir() done internally in the runtime

I would at least recommend to be hesitant merging at once, until these and possibly more tests have succeeded. The discrepancy between TS/NTS and sensitivity to internal chdir() calls might be hard to solve.

Thanks.

@weltling Appreciate your feedback. I am also more again supporting the drive relative paths as almost noone knows about this feature and when someone uses it, in a lot of cases it is by a mistake.

I have implemented this patch as there is a partial support currently (in a few places in the PHP code) and I decided to improve the support.

What do you think @weltling, @cmb69, @nikic about completely removing the support of drive relative paths and to return false from all file functions with warning? More specifically the same result as if path is outside open_basedir.

[…] completely removing the support of drive relative paths […]

I think that would be reasonable, but probably we should first deprecate using these drive relative paths.

@cmb69 I think given that a) this is very niche, b) it's already broken / support is hit and miss, and c) we're at a major version, it makes sense to remove support directly (with a nice error message). Otherwise we have to carry around this potentially dangerous functionality for the whole major version.

@nikic well, that actually makes sense.

I think given that a) this is very niche, b) it's already broken / support is hit and miss, and c) we're at a major version, it makes sense to remove support directly (with a nice error message). Otherwise we have to carry around this potentially dangerous functionality for the whole major version.

well, that actually makes sense.

I agree as well, Windows relative path with drive letter should be not supported, almost noone knows about it and thus very confusing.

Can someone update this PR to reject all Windows relative pathed with drive letter in all functions/extensions?

closing as I have no time to remove the support as discussed in #5001 (comment)