zendtech / ZendOptimizerPlus

Other
915 stars 142 forks source link

Use of traits can cause memory corruption. #102

Closed TerryE closed 11 years ago

TerryE commented 11 years ago

To see this, run Zend/tests/traits/static_get_called_class.php in a PHP debug enabled build with opcache.enable_cli=1 (or convert to apache callable variant). On my build, I get

---------------------------------------
/home/terry/work/php54/Zend/zend_opcode.c(273) : Block 0x7f9ff37292b8 status:
Beginning:      Freed (magic=0x00000005, expected=0x99954317)
    Start:  Overflown (magic=0x00000005 instead of 0x277C9396)
            At least 4 bytes overflown
      End:  Overflown (magic=0x00000065 instead of 0xBD1A2B91)
            At least 4 bytes overflown
---------------------------------------

The problem is caused because zend_clear_trait_method_name() efree's non-aliased function names. The solution is to detect and estrdup these in zend_prepare_function_for_execution(), eg. by adding

#if ZEND_EXTENSION_API_NO == PHP_5_4_X_API_NO
    /* In the case of traits in PHP 5.4.x, zend_clear_trait_method_name() efree's non-aliased function names
       so these need to be dupped to prevent efree cratering */
    if (op_array->scope && (op_array->scope->ce_flags & ZEND_ACC_TRAIT) == ZEND_ACC_TRAIT &&
        op_array->function_name && (op_array->fn_flags & ZEND_ACC_ALIAS) == 0) {
        op_array->function_name = estrdup(op_array->function_name);
    }
#endif

before the /* copy statics */ at line 562.

Apologies for not reporting this earlier -- I picked it up in my own testing about a month ago -- but I only remembered that this was a core OPcache bug and not MLC specific when I was back melding the latest core OPcache changes into MLC-OPcache.

dstogov commented 11 years ago

What PHP version do you use? I don't see any problems using PHP-5.5.

TerryE commented 11 years ago

Sorry. I should have said. I am doing my testing on PHP 5.3.17 and 5.4-9. This is clearly only a PHP >5.3 issue. (I realise that this is not the latest 5.4, but I use this so I can also test against the current Ubuntu prod version (PHP 5.4.9-4ubuntu2) without minor version sensitivities. I haven't started regression into PHP 5.5 yet.

My test harness tests MLC-OPcache against 5.3 and 5.4 (three runs to exercise the compile and execute and execute from cache paths); std OPcache against 5.4 with Ubuntu PHP 5.4 and Ubuntu PHP 5.4 without OPcache (clearly these last two are one run only) for the full PHP test suite (each pass is typically 8K out of the 10K tests). This was a bug that was in std OPcache.

TerryE commented 11 years ago

Dmitry, if you diff the 5.4 and 5.5 versions of zend_opcode.c, you will see that the two changes are (i) the addition of finally handling and (ii) the rewrite of trait destruction which removes this bug as a side-effect so it is PHP 5.4 specific. I am just doing the regression tests of MLC OPcache against 5.5 with the slight tweak to the above patch (it only applies for PHP 5.4). This passes all traits tests for 5.4 and 5.4.

dstogov commented 11 years ago

Please try to use the latest PHP-5.4. We had a lot of troubles with initial trait implementation and had to refactor it a bit few times during 5.4 life cycle. I suppose OPcache assumes the trait behavior implemented in the latest PHP-5.4. The previous implementations just couldn't work in all cases properly.

TerryE commented 11 years ago

OK, will do and move up to 5.4.15. BTW checking back through the github versions, this was fixed in 5.4.11 which was released in Jan this year. The latest Ubuntu 13.04, Fedora 18 and Mint 15 all use 5.4.9 (which is why I do). The various LTS versions use earlier dot releases, so what this really means is that traits don't work with OPcache (or APC) on the packaged 5.4 builds that are available with most stable Linux distros.

Whereas many sysadmins are comfortable with using PECL to build extensions, I suspect that few are willing to tackle a full configure and build of PHP itself, so in practice what this means is that OPcache will still barf on 5.4 source script that use traits when run under a stable distro. So my recommendation is still to apply the above fix, albeit adding an extra clause in the #ifdef to limit its application to PHP versions 5.4.0 thru 5.4.10.

dstogov commented 11 years ago

It was a PHP bug, and it's fixed. We can't fix it in OPcache for old PHP versions. Does it work fine for you with new PHP versions? Can we close this issue?

TerryE commented 11 years ago

It was a PHP bug, and it's fixed. Agreed.

Does it work fine for you with new PHP versions?

Yes, I've tested the current code and it works for the latest versions of 5.4 and 5.5 (and 5.3 in that this code is defined out anyway

We can't fix it in OPcache for old PHP versions. Can we close this issue?

The suggested fix would allow the lastest OPcache to be compiled and support against the current mainstream Linux distro's version (5.4.9).

IMO this is really a policy issue if you are saying as a matter of practicality / resourcing we don't add complexity to workaround bugs in old versions. Then as such, close the issue.