Skip to content

Commit

Permalink
[BUGFIX] Enhance alias resolving and reduce file system invocation
Browse files Browse the repository at this point in the history
* Consider direct alias invocation without being registered
  Invoking an aliased phar like `phar:///path/to/phar.phar/autoload.php`
  would allow to extract and register a potential alias name of the base
  name at `/path/to/phar.phar`. Using alias names directly inside phar
  archive does provide any possibility to map its full path to an alias
  (e.g. directly invoking `phar//alias.phar/autoload.php` would fail).
  This change aims to resolve the original base name by walking the
  current stack trace backwards. In terms of performance this is not
  ideal, but it's the only chance to retrieve the required information.
  That's also why the next section addresses performance.

* Enhance performance by reducing file system invocation
  Interceptors have to resolve the base file name from some given
  Phar invocation request. Internally the given path is traversed until
  a valid file is found in the file system and considered as the base
  name of the Phar archive.
  This change reduces superfluous calls to `is_file` when splitting
  the path in order to resolve the actual base name.

Resolves: #21
Resolves: #23
  • Loading branch information
ohader committed Apr 27, 2019
1 parent 7ca0312 commit b9d2f0b
Show file tree
Hide file tree
Showing 9 changed files with 161 additions and 88 deletions.
2 changes: 1 addition & 1 deletion src/Helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public static function removePharPrefix(string $path): string
public static function normalizePath(string $path): string
{
return rtrim(
static::getCanonicalPath(
static::normalizeWindowsPath(
static::removePharPrefix($path)
),
'/'
Expand Down
169 changes: 134 additions & 35 deletions src/Resolver/PharInvocationResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ class PharInvocationResolver implements Resolvable
'require_once'
];

/**
* Contains resolved base names in order to reduce file IO.
*
* @var string[]
*/
private $baseNames = [];

/**
* Resolves PharInvocation value object (baseName and optional alias).
*
Expand All @@ -56,21 +63,38 @@ public function resolve(string $path, int $flags = null)

if ($hasPharPrefix && $flags & static::RESOLVE_ALIAS) {
$invocation = $this->findByAlias($path);
if ($invocation !== null && $this->assertInternalInvocation($invocation, $flags)) {
if ($invocation !== null) {
return $invocation;
} elseif ($invocation !== null) {
return null;
}
}

$baseName = Helper::determineBaseFile($path);
$baseName = $this->resolveBaseName($path, $flags);
if ($baseName === null) {
return null;
}

if ($flags & static::RESOLVE_REALPATH) {
$baseName = realpath($baseName);
$baseName = $this->baseNames[$baseName];
}

return $this->retrieveInvocation($baseName, $flags);
}

/**
* Retrieves PharInvocation, either existing in collection or created on demand
* with resolving a potential alias name used in the according Phar archive.
*
* @param string $baseName
* @param int $flags
* @return PharInvocation
*/
private function retrieveInvocation(string $baseName, int $flags): PharInvocation
{
$invocation = $this->findByBaseName($baseName);
if ($invocation !== null) {
return $invocation;
}

if ($flags & static::RESOLVE_ALIAS) {
$alias = (new Reader($baseName))->resolveContainer()->getAlias();
} else {
Expand All @@ -82,53 +106,128 @@ public function resolve(string $path, int $flags = null)

/**
* @param string $path
* @return null|PharInvocation
* @param int $flags
* @return null|string
*/
private function findByAlias(string $path)
private function resolveBaseName(string $path, int $flags)
{
$normalizedPath = Helper::normalizePath($path);
$possibleAlias = strstr($normalizedPath, '/', true);
if (empty($possibleAlias)) {
$baseName = $this->findInBaseNames($path);
if ($baseName !== null) {
return $baseName;
}

$baseName = Helper::determineBaseFile($path);
if ($baseName !== null) {
$this->addBaseName($baseName);
return $baseName;
}

$possibleAlias = $this->resolvePossibleAlias($path);
if (!($flags & static::RESOLVE_ALIAS) || $possibleAlias === null) {
return null;
}

$trace = debug_backtrace();
foreach ($trace as $item) {
if (!isset($item['function']) || !isset($item['args'][0])
|| !in_array($item['function'], $this->invocationFunctionNames, true)) {
continue;
}
$currentPath = $item['args'][0];
if (Helper::hasPharPrefix($currentPath)) {
continue;
}
$currentBaseName = Helper::determineBaseFile($currentPath);
if ($currentBaseName === null) {
continue;
}
// ensure the possible alias name (how we have been called initially) matches
// the resolved alias name that was retrieved by the current possible base name
$currentAlias = (new Reader($currentBaseName))->resolveContainer()->getAlias();
if ($currentAlias !== $possibleAlias) {
continue;
}
$this->addBaseName($currentBaseName);
return $currentBaseName;
}

return null;
}

/**
* @param string $path
* @return null|string
*/
private function resolvePossibleAlias(string $path)
{
$normalizedPath = Helper::normalizePath($path);
return strstr($normalizedPath, '/', true) ?: null;
}

/**
* @param string $baseName
* @return null|PharInvocation
*/
private function findByBaseName(string $baseName)
{
return Manager::instance()->getCollection()->findByCallback(
function (PharInvocation $candidate) use ($possibleAlias) {
return $candidate->getAlias() === $possibleAlias;
function (PharInvocation $candidate) use ($baseName) {
return $candidate->getBaseName() === $baseName;
},
true
);
}

/**
* @param PharInvocation $invocation
* @param int $flags
* @return bool
* @experimental
* @param string $path
* @return null|string
*/
private function assertInternalInvocation(PharInvocation $invocation, int $flags): bool
private function findInBaseNames(string $path)
{
if (!($flags & static::ASSERT_INTERNAL_INVOCATION)) {
return true;
// return directly if the resolved base name was submitted
if (in_array($path, $this->baseNames, true)) {
return $path;
}

$trace = debug_backtrace(0);
$firstIndex = count($trace) - 1;
// initial invocation, most probably a CLI tool
if (($trace[$firstIndex]['file'] ?? null) === $invocation->getBaseName()) {
return true;
}
// otherwise search for include/require invocations
foreach ($trace as $item) {
if (!isset($item['function']) || !isset($item['args'][0])) {
continue;
}
if ($item['args'][0] === $invocation->getBaseName()
&& in_array($item['function'], $this->invocationFunctionNames, true)
) {
return true;
$parts = explode('/', Helper::normalizePath($path));

while (count($parts)) {
$currentPath = implode('/', $parts);
if (isset($this->baseNames[$currentPath])) {
return $currentPath;
}
array_pop($parts);
}

return null;
}

/**
* @param string $baseName
*/
private function addBaseName(string $baseName)
{
if (isset($this->baseNames[$baseName])) {
return;
}
$this->baseNames[$baseName] = realpath($baseName);
}

return false;
/**
* @param string $path
* @return null|PharInvocation
*/
private function findByAlias(string $path)
{
$possibleAlias = $this->resolvePossibleAlias($path);
if ($possibleAlias === null) {
return null;
}
return Manager::instance()->getCollection()->findByCallback(
function (PharInvocation $candidate) use ($possibleAlias) {
return $candidate->getAlias() === $possibleAlias;
},
true
);
}
}
Empty file.
10 changes: 7 additions & 3 deletions tests/Functional/HelperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,16 @@ public function baseFileIsResolvedDataProvider(): array
'{DIR}/bundle.phar'
],
[
'phar://{DIR}/other/../bundle.phar/path/../other/content.txt',
'{DIR}/bundle.phar'
'phar://{DIR}/Existing/../bundle.phar/path/../other/content.txt',
'{DIR}/Existing/../bundle.phar'
],
[
'phar://{DIR}/../Fixtures/bundle.phar',
'{DIR}/bundle.phar'
'{DIR}/../Fixtures/bundle.phar'
],
[
'phar://{DIR}/NotExisting/../bundle.phar/path/../other/content.txt',
null
],
[
'phar://{DIR}/not-existing.phar/path/../other/content.txt',
Expand Down
17 changes: 0 additions & 17 deletions tests/Functional/Interceptor/AbstractTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -454,23 +454,6 @@ public function streamOpenAllowsInvocationForIncludeOnAliasedPhar(string $allowe
static::assertNotFalse($result);
}

/**
* @param string $allowedPath
*
* @test
* @dataProvider allowedPathsDataProvider
*/
public function streamOpenDeniesInvocationForAliasedIncludeOutsideAliasedPhar(string $allowedPath)
{
// used to trigger registration of Phar alias
include('phar://' . $allowedPath . '/Classes/Domain/Model/DemoModel.php');

self::expectException(Exception::class);
self::expectExceptionCode(static::EXPECTED_EXCEPTION_CODE);
// using Phar alias outside(!) of according Phar archive
include('phar://bndl.phar/Classes/Domain/Model/DemoModel.php');
}

/**
* @param string $deniedPath
*
Expand Down
8 changes: 3 additions & 5 deletions tests/Functional/Interceptor/ConjunctionInterceptorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class ConjunctionInterceptorTest extends AbstractTestCase
*/
protected $allowedAliasedPaths = [
__DIR__ . '/../Fixtures/geoip2.phar',
// __DIR__ . '/../Fixtures/alias-no-path.phar',
__DIR__ . '/../Fixtures/alias-no-path.phar',
__DIR__ . '/../Fixtures/alias-with-path.phar',
];

Expand Down Expand Up @@ -91,15 +91,13 @@ public function isFileSystemInvocationAcceptableDataProvider(): array
$fixturePath = __DIR__ . '/../Fixtures';

return [
/*
'include phar' => [
$fixturePath . '/geoip2.phar',
[Helper::class . '::determineBaseFile' => 1, Reader::class . '->resolveContainer' => 2]
[Helper::class . '::determineBaseFile' => 1, Reader::class . '->resolveContainer' => 19]
],
*/
'include autoloader' => [
'phar://' . $fixturePath . '/geoip2.phar/vendor/autoload.php',
[Helper::class . '::determineBaseFile' => 45, Reader::class . '->resolveContainer' => 60]
[Helper::class . '::determineBaseFile' => 1, Reader::class . '->resolveContainer' => 18]
],
];
}
Expand Down
6 changes: 2 additions & 4 deletions tests/Functional/Interceptor/PharExtensionInterceptorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class PharExtensionInterceptorTest extends AbstractTestCase
*/
protected $allowedAliasedPaths = [
__DIR__ . '/../Fixtures/geoip2.phar',
// __DIR__ . '/../Fixtures/alias-no-path.phar',
__DIR__ . '/../Fixtures/alias-no-path.phar',
__DIR__ . '/../Fixtures/alias-with-path.phar',
];

Expand Down Expand Up @@ -130,15 +130,13 @@ public function isFileSystemInvocationAcceptableDataProvider(): array
$fixturePath = __DIR__ . '/../Fixtures';

return [
/*
'include phar' => [
$fixturePath . '/geoip2.phar',
[Helper::class . '::determineBaseFile' => 1, Reader::class . '->resolveContainer' => 2]
],
*/
'include autoloader' => [
'phar://' . $fixturePath . '/geoip2.phar/vendor/autoload.php',
[Helper::class . '::determineBaseFile' => 30, Reader::class . '->resolveContainer' => 30]
[Helper::class . '::determineBaseFile' => 1, Reader::class . '->resolveContainer' => 2]
],
];
}
Expand Down
8 changes: 3 additions & 5 deletions tests/Functional/Interceptor/PharMetaDataInterceptorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class PharMetaDataInterceptorTest extends AbstractTestCase
*/
protected $allowedAliasedPaths = [
__DIR__ . '/../Fixtures/geoip2.phar',
// __DIR__ . '/../Fixtures/alias-no-path.phar',
__DIR__ . '/../Fixtures/alias-no-path.phar',
__DIR__ . '/../Fixtures/alias-with-path.phar',
];

Expand Down Expand Up @@ -86,15 +86,13 @@ public function isFileSystemInvocationAcceptableDataProvider(): array
$fixturePath = __DIR__ . '/../Fixtures';

return [
/*
'include phar' => [
$fixturePath . '/geoip2.phar',
[Helper::class . '::determineBaseFile' => 1, Reader::class . '->resolveContainer' => 2]
[Helper::class . '::determineBaseFile' => 1, Reader::class . '->resolveContainer' => 18]
],
*/
'include autoloader' => [
'phar://' . $fixturePath . '/geoip2.phar/vendor/autoload.php',
[Helper::class . '::determineBaseFile' => 30, Reader::class . '->resolveContainer' => 45]
[Helper::class . '::determineBaseFile' => 1, Reader::class . '->resolveContainer' => 17]
],
];
}
Expand Down
29 changes: 11 additions & 18 deletions tests/Unit/HelperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,24 +57,17 @@ public function pharPrefixIsRemoved(string $path, string $expectation)
public function pathIsNormalizedDataProvider(): array
{
$dataSet = [
['.', ''],
['..', ''],
['../x', 'x'],
['./././x', 'x'],
['./.././../x', 'x'],
['a/../x', 'x'],
['a/b/../../x', 'x'],
['/a/b/../../x', '/x'],
['c:\\a\\b\..\..\x', 'c:/x'],
['phar://../x', 'x'],
['phar://../x/file.phar', 'x/file.phar'],
['phar:///../x/file.phar', '/x/file.phar'],
['phar://a/b/../../x', 'x'],
['phar:///a/b/../../x', '/x'],
['phar://a/b/../../x/file.phar', 'x/file.phar'],
['phar:///a/b/../../x/file.phar', '/x/file.phar'],
['phar://c:\\a\\b\\..\\..\\x\\file.phar', 'c:/x/file.phar'],
[' phar:///a/b/../../x/file.phar ', '/x/file.phar'],
['.', '.'],
['..', '..'],
['./x', './x'],
['../x', '../x'],
['c:\\a\\b\..\..\x', 'c:/a/b/../../x'],
['phar://../x', '../x'],
['phar:///../x', '/../x'],
['phar://c:\\a\\b\..\..\x', 'c:/a/b/../../x'],
[' phar://../x ', '../x'],
[' phar:///../x ', '/../x'],
[' phar://c:\\a\\b\..\..\x ', 'c:/a/b/../../x'],
];

return array_merge($this->pharPrefixIsRemovedDataProvider(), $dataSet);
Expand Down

0 comments on commit b9d2f0b

Please sign in to comment.