[ClassLoader] Fix ClassCollectionLoader inlining with __halt_compiler #20455

giosh94mhz · 2016-11-09T11:41:34Z

Q	A
Branch?	2.7+
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

I have some PHP classes which ends with an __halt_compiler call which fail on production.

This is due to the ClassCollectionLoader inlining the class code (which is followed by random binary data) which breaks PHP parsing of the following classes.

I found this issue while using ZendGuardLoader module, but this apply to whatever php class using this function call.

I've made my fix, based on #19859, and tested it with symfony 2.7 and 2.8

nicolas-grekas · 2016-11-09T12:00:30Z

src/Symfony/Component/ClassLoader/ClassCollectionLoader.php

@@ -106,6 +106,7 @@ public static function load($classes, $cacheDir, $name, $autoReload, $adaptive =

        $c = '(?:\s*+(?:(?:#|//)[^\n]*+\n|/\*(?:(?<!\*/).)++)?+)*+';
        $strictTypesRegex = str_replace('.', $c, "'^<\?php\s.declare.\(.strict_types.=.1.\).;'is");
+        $haltCompilerRegex = str_replace('.', $c, "'.__halt_compiler.\(.\).'is");


No need for the leading and trailing dots in the regexp

Right. The trailing dot for sure, the leading one need at least a '\b'. I'll fix it.

giosh94mhz · 2016-11-09T12:47:29Z

I see your point of view @stof, but is not how __halt_compiler works
(AFAIK). It won't stop the whole php compiler, but only the compiling of
the current file-unit.

Try this

<?php

include './halt.php';
echo "after halt\n";
```php
<?php
echo "before halt\n";

__halt_compiler();

dsfsdfsdfsdfsdf

You'll get:

before halt
after halt

2016-11-09 13:18 GMT+01:00 Christophe Coevoet notifications@github.com:

your fix looks wrong to me. A file containing __halt_compiler simply
cannot be inlined at all (well, you could accept 1 such file if it can
be placed at the end, but this becomes much too tricky)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#20455 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AE-mh4rUmf-3goQsr0sAuKxVxthWmnrqks5q8bn5gaJpZM4KtcYJ
.

nicolas-grekas · 2016-11-14T10:25:07Z

src/Symfony/Component/ClassLoader/ClassCollectionLoader.php

@@ -106,6 +106,7 @@ public static function load($classes, $cacheDir, $name, $autoReload, $adaptive =

        $c = '(?:\s*+(?:(?:#|//)[^\n]*+\n|/\*(?:(?<!\*/).)++)?+)*+';
        $strictTypesRegex = str_replace('.', $c, "'^<\?php\s.declare.\(.strict_types.=.1.\).;'is");
+        $haltCompilerRegex = str_replace('.', $c, "'\b__halt_compiler.\(.\)'is");


Looking at issue like twigphp/Twig#2243, I think we should also add __FILE__ and __DIR__ to the regexp. Which means this should maybe be refactored to eg.:
$dontInlineRegex = str_replace('.', $c, "'(?:^<\?php\s.declare.$.strict_types.=.1.$.;|\b__halt_compiler.$.$|\b__(?:DIR|FILE)__\b)'is");
It will catch more cases than strictly required (e.g. __DIR__ in a comment), but since the outcome of this "false positive" is not important (a few "require" vs inlining won't matter), we shouldn't care, esp. since the better way is much more complex than this "good enough" regexp IMHO.
WDYT?

agreed for now

I've read the other issues and I think you are correct, also __DIR__ and __FILE__ should not be inlined.
Since the ClassCollectionLoader main purpose (AFAIK) is to avoid calling the autoloader for common classes, I don't think that we have major performance issue with a bunch of added fstat/includes (which can be opcached btw).

I think the regex you suggest deserve to be multine to be a little more readable, but I'll see how much effort it requires.

giosh94mhz · 2016-11-14T13:48:11Z

Ok, added the fix for __DIR__ and __FILE__. I've also indented the regex and is clearer now IMO.

inso · 2016-11-14T16:47:01Z

Maybe it is better to use token_get_all?

giosh94mhz · 2016-11-14T17:29:47Z

token_get_all is probably much more correct/safe, but I think that the regex could be enough. Worst case, it match more files than required and are included instead of inlined. Dunno, let the maintainer decide :)

stof · 2016-11-14T17:32:36Z

token_get_all can use too much memory on big PHP files, hitting the memory limit

…DIR__

giosh94mhz · 2016-11-15T08:25:33Z

I've fixed some "unrelated" unit tests, and now everything pass correctly on travis & co.
The box above still show a "changes requested" box from @nicolas-grekas which I cannot fix, since I've rebase the code. I never used this GitHub feature, I feel like a noob :)

nicolas-grekas · 2016-11-15T09:22:10Z

Thank you @giosh94mhz.

…lt_compiler (giosh94mhz) This PR was squashed before being merged into the 2.7 branch (closes #20455). Discussion ---------- [ClassLoader] Fix ClassCollectionLoader inlining with __halt_compiler | Q | A | ------------- | --- | Branch? | 2.7+ | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | | License | MIT | Doc PR | | I have some PHP classes which ends with an `__halt_compiler` call which fail on production. This is due to the ClassCollectionLoader inlining the class code (which is followed by random binary data) which breaks PHP parsing of the following classes. I found this issue while using ZendGuardLoader module, but this apply to whatever php class using this function call. I've made my fix, based on #19859, and tested it with symfony 2.7 and 2.8 Commits ------- a60cd15 [ClassLoader] Fix ClassCollectionLoader inlining with __halt_compiler

carsonbot added Status: Needs Review ClassLoader Bug labels Nov 9, 2016

nicolas-grekas requested changes Nov 9, 2016

View reviewed changes

giosh94mhz force-pushed the fix_halt_compiler_class_load branch from aef93dc to a9729f4 Compare November 9, 2016 12:40

nicolas-grekas reviewed Nov 14, 2016

View reviewed changes

giosh94mhz added 2 commits November 15, 2016 09:12

[ClassLoader] Fix ClassCollectionLoader inlining with __halt_compiler

a00ca56

[ClassLoader] Fix ClassCollectionLoader inlining with __FILE__ and __…

11cf77a

…DIR__

giosh94mhz force-pushed the fix_halt_compiler_class_load branch from 8fef9f5 to 11cf77a Compare November 15, 2016 08:13

stof approved these changes Nov 15, 2016

View reviewed changes

nicolas-grekas approved these changes Nov 15, 2016

View reviewed changes

nicolas-grekas closed this Nov 15, 2016

This was referenced Nov 17, 2016

Release v3.2.0-RC1 #20552

Merged

Release v2.7.21 #20573

Merged

Release v2.8.14 #20574

Merged

Release v3.1.7 #20575

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ClassLoader] Fix ClassCollectionLoader inlining with __halt_compiler #20455

[ClassLoader] Fix ClassCollectionLoader inlining with __halt_compiler #20455

giosh94mhz commented Nov 9, 2016

nicolas-grekas Nov 9, 2016

giosh94mhz Nov 9, 2016

giosh94mhz commented Nov 9, 2016

nicolas-grekas Nov 14, 2016 •

edited

Loading

stof Nov 14, 2016

giosh94mhz Nov 14, 2016

giosh94mhz commented Nov 14, 2016

inso commented Nov 14, 2016

giosh94mhz commented Nov 14, 2016

stof commented Nov 14, 2016

giosh94mhz commented Nov 15, 2016

nicolas-grekas commented Nov 15, 2016

[ClassLoader] Fix ClassCollectionLoader inlining with __halt_compiler #20455

[ClassLoader] Fix ClassCollectionLoader inlining with __halt_compiler #20455

Conversation

giosh94mhz commented Nov 9, 2016

nicolas-grekas Nov 9, 2016

Choose a reason for hiding this comment

giosh94mhz Nov 9, 2016

Choose a reason for hiding this comment

giosh94mhz commented Nov 9, 2016

nicolas-grekas Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

stof Nov 14, 2016

Choose a reason for hiding this comment

giosh94mhz Nov 14, 2016

Choose a reason for hiding this comment

giosh94mhz commented Nov 14, 2016

inso commented Nov 14, 2016

giosh94mhz commented Nov 14, 2016

stof commented Nov 14, 2016

giosh94mhz commented Nov 15, 2016

nicolas-grekas commented Nov 15, 2016

nicolas-grekas Nov 14, 2016 •

edited

Loading