Fix #69477 ext/zip: Allow extracting to paths with dirs ending with dot

[mudrd8mz]

The function php_zip_make_relative_path() used to stop on the first
right-most occurrence of './' in the ZIP entry path. As a result, the
extractTo() method didn't extract entries like foo/bar./baz/file.txt
into correct location.

[nikic]

Test failure on Windows:

========DIFF========
001+ Warning: ZipArchive::extractTo(): Permission denied in C:\projects\php-src\ext\zip\tests\bug69477.php on line 43
001- ok
002+ failed: a./b/c/file02.txt
========DONE========

[mudrd8mz]

Thanks @nikic for sharing. This was mentioned in https://bugs.php.net/bug.php?id=69477 comment by cmb@php.net as well:

FWIW: Windows doesn't like trailing dots in folder names, so such archives would not be portable.

I don't know if there is some kind of policy for these cases. Should we somehow try and replace the dot with underscore or something like that? Are you able to test how Windows unzip handles archives with paths like this?

Suggestions welcome. TIA.

[mudrd8mz]

Hmm, googling for "windows folder ending with dot" shows this seems to be a common known issue on Windows ...

[nikic]

Probably @cmb69 can suggest some appropriate behavior.

[cmb69]

Windows explorer just swallows trailing dots when extracting; 7-zip replaces each dot with an underscore. I think I'd prefer the 7-zip behavior.

[mudrd8mz]

• Added another commit that replaces the trailing dots in the path with underscores on Windows. I believe it is cleaner to have such change in a separate commit, following the "one commit for one logical change" rule. Happy to merge if you think it would be more appropriate.
• Rebased against latest PHP-7.4

Thank you in advance for re-evaluation.

[cmb69]

The patch looks good to me from the Windows side. It might make sense to add a test for a drive relative path (something like 'C:a./b./file15.txt' => 'a_/b_/file15.txt'; note the missing slash after the colon). And yes, having a separate commit for the trailing dot on Windows issue makes sense (that could even be a separate PR, but it's okay to combine into this PR, if a separate NEWS entry will be created).

Anyhow, it seems this PR fixes #77214 as well (that actually appears to be a duplicate of #69477).

@remicollet, what do you think about this PR?

[mudrd8mz]

Good catch @cmb69 on drive relative paths. It turned out they were not handled correctly, only absolute paths were. I added a commit to handle that special case as well.

From what I understood, the colon sign is not actually allowed character in file names on Windows at all. So there is still potential for troubles - such as colons at the end or in the middle of path - that were not and are not handled well. Maybe we should replace them with underscores on Windows, too?

[mudrd8mz]

Maybe we should replace them with underscores on Windows, too?

If so, would a new issue be more appropriate rather than trying to fix it here as well?

[cmb69]

There are more characters which are not allowed as filenames on NTFS systems, namely redirection symbols (<>|), wildcard characters (*?) and double-quotes ("). I think these should all be replaced with an underscore, but that's likely a topic for anoter PR. :)

[mudrd8mz]

Thanks @cmb69. I think so.

With regards to the failing MACOS_DEBUG_NTS it seems like a failure on the CI infrastructure rather than something actually broken in the patch? Is there something more I am supposed to do here? (sorry to eventually dump question, this is my first contribution here).

[cmb69]

Yes, the macOS failure is unlikely to be related to this patch. This PR is currently waiting on review from Remi, who's got the week off.

[cmb69]

Uh, sorry, completely forgot about this patch. We landed a security fix recently, so this patch has now merge conflicts. I also just submitted PR #7528 (which should solve the colon issues). With these two commits, I think this patch can be massively simplified.

However, it is probably a good idea to address the unsupported character issue on Windows first, so I've filed https://bugs.php.net/81488, and forwarded as feature request to libzip. So I think it's best not to do any character replacements in this PR.

If you still want to work on this, please adapt the PR accordingly.

[Girgias]

Closing as stale.