Phar mount points not working this OPcache enabled.

[TerryE]

This is related to #115, but to do with the other Phar URI mapping function Phar::mount(). This allows the application to bind an external file to a mount point within a Phar. I was testing this to see if the related fix also worked for mount points.

Here is the simple test stub that shows this:

<?php
    # phar.readonly must be 0
    $dir = dirname(__FILE__);
    foreach (['A', 'B'] as $v) {
        file_put_contents("$dir/temp{$v}.txt", "This is test file {$v}\n");
        $stub = "<?php
        Phar::interceptFileFuncs();
        header('Content-Type: text/plain;');
        Phar::mount('temp.txt', '$dir/temp{$v}.txt' );
        require 'phar://this/index.php';
        __HALT_COMPILER(); ?>";
        $p = new Phar( "$dir/$v.phar.php", 0, 'this');
        $p['index.php'] = "<?php
            echo \"Entering Index $v.\n\";
            include 'phar://this/hello.php';
            include 'phar://this/temp.txt'; 
            ";
        $p['hello.php'] = "Hello World says I am $v!\n";    
        $p->setStub($stub);
        unset($p);
    }

Run this in CLI mode with -d phar.readonly=0 and it creates the Phars {A,B}.phar.php in the current directory. Executing in these in the CLI, eg. php A.phar.php gives the output:

Entering Index A.
Hello World says I am A!
This is test file A

and similarly for the B variant. Likewise requesting http:://localhost:8888/A.phar.php against a php -d opcache.enable=0 -S local:host:8888 works fine and the A and B variants can be randomly intermixed giving the expected return.

When repeated with OPcache enabled, A then B gives

• the A then B output with Xinchen's patch
• the A then A again without this patch showing the #115 bug.

However, if you do A, B, A or A, A then Phar throws a PharException on the mount statement in subsequent invocations. This doesn't happen with OPcache disabled.

I'll track down the root-cause and post back. I suspect that the best way to address this and #115 is to review the entire URI mapping strategy within Phar and make it fully OPcache compatible by adding the correct hook function into zend.h and chaining this through Phar. A 5.6 fix rather than 5.5.x.

[TerryE]

OK, the minimum script which exhibits this bug is

<?php
# run with -d phar.readonly=0 to create the Phar
$stub = "<?php header('Content-Type: text/plain;');
Phar::mount('this.file', '". __FILE__ . "');
echo '--Mount completed--\n';
__HALT_COMPILER(); ?>";
$p = new Phar( dirname(__FILE__) .'/A.phar.php' );
$p['index.php'] = "";  # A Phar must have at least one file, hence this dummy
$p->setStub($stub);

when run with php -d opcache.enable=0 -S localhost:8888 and hit with repeated wget "http://localhost:8888/A.phar.php" -o /dev/null -O - produces repeated --Mount completed-- lines. Toggling to enabling OPcache throws the PharException on all but the first wget. I am now debugging this through to find out what cross-request persistent state is causing this.

[TerryE]

OK, Phar hangs a bunch of initialisation off its compile_file hook which intercepts the open of the phar container, in this case A.phar.php. When the compile is cached, then this initialisation is never carried out and hence some key tables point to garbage. In other words, key Phar functionality relies on the container Phar being "compiled", which doesn't occur on cached runs. I am tired, so I'll pick this up tomorrow and trace it out properly, but I suspect that this is really a Phar only bug. :-(

[dstogov]

I've added PHPT tests for #115 and this issues. I think the main problem that the PHAR's phar_resolve_path() doesn't actually resolve the paths and returns NULL. On the other hand, the php_stream_phar_wrapper sets "opened_path" properly. I hope, it must be possible to fix both issues on PHAR side, be fixing phar_resolve_path().

[dstogov]

It seems I've found a way to fix it in OPcahce. Must be fixed now.

[TerryE]

Sorry Dmitri, but we also need to read the manifest for the '_once' variants of cached compiles. See issue0149A.phpt attached.

--TEST--
ISSUE #149A (Phar mount points not working with OPcache enabled)
--INI--
opcache.enable=1
opcache.enable_cli=1
phar.readonly=0
--SKIPIF--
<?php require_once('skipif.inc'); ?>
<?php if (!extension_loaded("phar")) die("skip"); ?>
<?php if (php_sapi_name() != "cli") die("skip CLI only"); ?>
--FILE--
<?php
$stub = "<?php header('Content-Type: text/plain;');
Phar::mount('this.file', '". __FILE__ . "');
if (file_get_contents('". __FILE__ . "') == file_get_contents('phar://this/this.file')) {
  echo 'OK\n';
}
__HALT_COMPILER(); ?>";
$p = new Phar(__DIR__ . '/issue0149A.phar.php', 0, 'this');
$p['index.php'] = "";  # A Phar must have at least one file, hence this dummy
$p->setStub($stub);
unset($p);
file_put_contents(__DIR__ . '/issue0149A-test.php', '<?php include_once "./issue0149A.phar.php";');

include "php_cli_server.inc";
php_cli_server_start('-d opcache.enable=1 -d opcache.enable_cli=1');
echo file_get_contents('http://' . PHP_CLI_SERVER_ADDRESS . '/issue0149A-test.php');
echo file_get_contents('http://' . PHP_CLI_SERVER_ADDRESS . '/issue0149A-test.php');
?>
--CLEAN--
<?php 
@unlink(__DIR__ . '/issue0149A.phar.php');
@unlink(__DIR__ . '/issue0149A-test.php');
?>
--EXPECT--
OK
OK

This fails/succeeds depending on opcache.enable=1/0.

[TerryE]

I believe that we are approaching fixing this issue the wrong way by attempting to put "fixes" into OPcache for what is essentially an architectural flaw in Phar. Your previous "fix" was a workaround in OPcache to overcome this flaw in Phar, but what I tried to show by the previous test is that it only covers one such execution path, and that we've missed others.

The nub of this flaw is that a Phar container has a structure which includes an entry stub, a manifest and zero or more embedded sources. Some of the Phar API calls can only be executed from the stub and the current code assumes that the manifest has been processed during the read of the Phar file. OPcache goes to some great lengths to avoid unnecessary I/O to source files for any cached modules. What I propose is that we change Phar to drop the assumption that the source containing a Phar stub has already been read during the execution of the stub -- as is the case if executing a cached compile -- by adding a few initialisation checks to these Phar API calls which will then read the Phar container and parse the manifest on a just-in-time basis. This way no changes are needed in OPcache, and that any already made to OPcache should be backed out once we apply this Phar fix.

However, to do this I really need to understand the Phar internals properly. It will take a week or so. I don't understand why the urgency to patch over issues which aren't show-stoppers.

[TerryE]

Dmitri, I am still working my way through the Phar internals, but I find that the documentation is somewhat inconsistent with the functionality as coded.

However, there is another class of failures that I've come across and these are also associated with Phar mount points. These mount point are another abstraction mechanism a bit like aliases, and basically allow a php application, such as phpBB say, to be packaged into a single Phar but run from many user accounts. phpBB has a scratch cache directory where is compiles its templates to (think of Smarty), but these cache files will end up being user-specific, so for example the phpBB stub will map the Phar mountpoint cache to the user's local cache within his or her DOCROOT. At the moment, Xinchen's fix resolves alias but not these mount points -- which should be converted back to fully resolved addresses.

Here is issue0149B.phpt which shows this. Again, it works with -d opcache.enable=0:

--TEST--
ISSUE #149B (Phar mount points not working with OPcache enabled)
--INI--
opcache.enable=1
opcache.enable_cli=1
phar.readonly=0
--SKIPIF--
<?php require_once('skipif.inc'); ?>
<?php if (!extension_loaded("phar")) die("skip"); ?>
<?php if (php_sapi_name() != "cli") die("skip CLI only"); ?>
--FILE--
<?php
foreach(['A','B'] as $v) {
  @mkdir("./issue0149B-$v");
  file_put_contents(__DIR__ . "/issue0149B-$v/x.php", "<?php echo 'Hello world $v!\n';");
}
$p = new Phar(__DIR__ . '/issue0149B.phar.php', 0, 'this');
$p['index.php'] = "";  # A Phar must have at least one file, hence this dummy
$p->setStub(sprintf(<<<'ENDSTUB'
<?php header('Content-Type: text/plain;');
Phar::mapPhar();
Phar::mount('this.sub_dir', '%s/issue0149B-' . $_GET['ver']);
include "phar://this/this.sub_dir/x.php";
__HALT_COMPILER(); ?>
ENDSTUB
, __DIR__ ));
unset($p);

include "php_cli_server.inc";
php_cli_server_start('-d opcache.enable=1 -d opcache.enable_cli=1');
echo file_get_contents('http://' . PHP_CLI_SERVER_ADDRESS . '/issue0149B.phar.php?ver=A');
echo file_get_contents('http://' . PHP_CLI_SERVER_ADDRESS . '/issue0149B.phar.php?ver=B');
?>
--CLEAN--   
<?php 
@unlink(__DIR__ . '/issue0149B.phar.php');
@unlink(__DIR__ . '/issue0149B-A/x.php');
@rmdir(__DIR__ . '/issue0149B-A/');
@unlink(__DIR__ . '/issue0149B-B/x.php');
@rmdir(__DIR__ . '/issue0149B-B/');
?>
--EXPECT--
Hello world A!
Hello world B!

[TerryE]

@dstogov, @laruence Dmitri, Xinchen, I've pretty much completed my review of Phar and looked at the issues around building real-world apps using this extension at its current functionality. This isn't the place to write up such a review, and I also doubt that anyone would read it even if I went to the bother of doing so. My overall conclusion is that it was a mistake to bring this extension into the PHP core at its current level of maturity, and any attempts on my part to address the issues that need sorted in Phar would be a frustrating and unrewarding task within the current Internals practices. I feel that would be wise for me to give this extension a pass in terms of any development contribution. It's use seems to be dying anyway apart from one or two provisioning use cases, so maybe the best course is to leave it to so do naturally.

[b7kich]

TL;DR: opc not working with phar. Screw the phar guys! Won't fix. Calling this fixed now! ??