Bug #66337 [Nab]: Wrong size calculation on optimization of class constants

Edit report at https://bugs.php.net/bug.php?id=66337&edit=1 ID: 66337 Updated by: dmitry@php.net Reported by: Terry at ellisons dot org dot uk Summary: Wrong size calculation on optimization of class constants Status: Not a bug Type: Bug Package: opcache Operating System: N/A PHP Version: master-Git-2013-12-22 (Git) Assigned To: dmitry Block user comment: N Private report: N New Comment: Your patch is committed. Previous Comments: ------------------------------------------------------------------------ [2014-01-07 18:20:41] Terry at ellisons dot org dot uk I have since localised this to 5.6's introduction of the interned_empty_string constant. My build was missing the changes that you made re #66429, which is why I was seeing this error and you weren't. Nonetheless, the logic for ADD_INTERNED_STRING() is wrong. If the string is already interned then the calc routine will not need to allocate any memory. It can only require memory for currently non-interned strings So shouldn't this read: # define ADD_INTERNED_STRING(str, len) \ if(!IS_INTERNED(str)) { \ const char *tmp = accel_new_interned_string((str), (len), 1 TSRMLS_CC); \ if (tmp != (str)) { \ (str) = (char*)tmp; \ } else { \ ADD_DUP_SIZE((str), (len)); \ } \ } ------------------------------------------------------------------------ [2013-12-31 05:49:42] Terry at ellisons dot org dot uk Also <?php var_dump(''); doesn't generate the error either even though vld shows this as the same opcode sequence. So its only some constants that are being in interned. ------------------------------------------------------------------------ [2013-12-31 05:41:04] Terry at ellisons dot org dot uk BTW if you use: <?php namespace Fqrrewrtwt; var_dump(__NAMESPACE__); then you don't get the leak, so I suspect that the compiler is only using interned strings if the interned string already exists. ------------------------------------------------------------------------ [2013-12-31 05:35:55] Terry at ellisons dot org dot uk Dmitry, I am using a pretty standard core build, on 64bit Ubuntu pulled from php-src PHP-5.6 with the last commit: commit 809eb77689fc3c4f960dad3ec85a7d7bfde87ea0 Merge: 4680986 464c219 Author: Remi Collet <remi@php.net> Date: Sat Dec 28 14:29:27 2013 +0100 Merge branch 'PHP-5.5' into PHP-5.6 * PHP-5.5: minor fix on previous The minimum script which shows this is: $ php56 -d opcache.enable=1 -d opcache.enable_cli=1 <?php var_dump(__NAMESPACE__); ^d Tue Dec 31 02:59:58 2013 (27180): Warning Internal error: wrong size calculation: - start=0x39368e08, end=0x39369148, real=0x39369140 You need both enables to get the leak, of course, but it is independent of optimization. > but how did you get interned string there? Follow through this in gdb from breaks at zend_persist_calc.c:173. __NAMSPACE__ is '' so the first opcode is a SENDVAL '' and this is the first element of op_array->literals. So 173 ADD_SIZE(zend_persist_zval_calc(&p->constant TSRMLS_CC)); (gdb) p p->constant.value.str.val - accel_shared_globals.interned_strings_start $5 = 72 that is the literal is an interned string and the bug follows. Note that this literal isn't an interned string in PHP 5.5, as you can see by doing same print at the corresponding line in the 5.5 version of zend_persist_calc.c. Because 5.5 doesn't generate interned literals and 5.6 does, this exposes the logic flaw path in the ADD_INTERNED_STRING()macro. BTW, we can simply this because my 5.6 changes will also work in 5.5 As to why you can't replicated this -- I don't know. Are you pulling a complete PHP-5.6 because this interning patch is in the compiler. ------------------------------------------------------------------------ [2013-12-30 12:40:19] dmitry@php.net I still can't reproduce this. $ sapi/cli/php -n -d zend_extension="opcache.so" -d opcache.enable_cli=1 Zend/tests/heredoc_015.phpt You probably right, about the condition change, but how did you get interned string there? ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=66337 -- Edit this bug report at https://bugs.php.net/bug.php?id=66337&edit=1