ext/phar: improve .phar madic directory preservation logic in phar::addEmptyDir()#22146
ext/phar: improve .phar madic directory preservation logic in phar::addEmptyDir()#22146LamentXU123 wants to merge 1 commit into
Conversation
|
I will try to merge this myself to the 8.4 and 8.5 branch tomorrow since this is quite a decided fix. I will take this as an opportunity to learn php's merging workflow :) |
|
Did you merge 8.5 into master? |
That part was done by you in #22011 I assume :) |
|
You still need to merge up all the way from the base branch to master, even if this means an empty merge. |
…ddEmptyDir()
Now, the .phar directory is a magic dir for phar files, and in phar::addEmptyDir(), users couldn't create a dir naming .phar
The implementation is:
```c
if (zend_string_starts_with_literal(dir_name, ".phar")) {
zend_throw_exception_ex(spl_ce_BadMethodCallException, 0, "Cannot create a directory in magic \".phar\" directory");
RETURN_THROWS();
```
This has two bugs.
Firstly, people can use /.phar to create the .phar dir. The leading / will be ignored. (no need to concern about ../ though, it will be ignored.)
```php
<?php
$phar = new Phar(__DIR__ . '/test.phar', 0, 'test.phar');
$phar->addEmptyDir('/.phar');
var_dump(is_dir('phar://' . __DIR__ . '/test.phar/.phar'));
```
Will return true with the .phar dir created, while if the dir is .phar it will raise an error.
Secondly, it only matches the prefix. That means, /.pharxxx will not be allowed to create, which is not a magic dir.
```php
<?php
$phar = new Phar(__DIR__ . '/test.phar', 0, 'test.phar');
$phar->addEmptyDir('.pharx');
```
This will raise an error.
```
PHP Fatal error: Uncaught BadMethodCallException: Cannot create a directory in magic ".phar" directory in C:\Users\admin\Desktop\bench.php:3
```
This PR fix both by 1. adding a trailing check of the path to make .pharx valid 2. adding a check to /.phar
Closes phpGH-22146.
|
I see. The empty merge is now pushed 951c891. Thanks! |
|
@LamentXU123 since you now have merge rights - can you please configure |
|
That's configured now, I've also make sure every other initial setup is correct. Thanks! |
#22011 targeting 8.4. cc @Girgias we also need to merge this into 8.5 :)