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 User updated by: Terry at ellisons dot org dot uk 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: 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)); \ } \ } Previous Comments: ------------------------------------------------------------------------ [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? ------------------------------------------------------------------------ [2013-12-30 10:08:53] Terry at ellisons dot org dot uk My bad**2. Xinchen's fix removed this manifestation, but the underlying bug is still there. I've just done a make test on the current PHP-56 and 51 tests fail with this (see below). In PHP 5.6 zend_accel_store_interned_string() does not bump ZCG(mem) because interned strings donn't require a compiled script zval, however the corresponding macro in zend_persist_calc.c bumps memory_used by 8. This is because the logic of ADD_INTERNED_STRING(str, len) is wrong. If str is already interned then there is nothing to do, but in the current macro in this case tmp == str so sizeof(zval *) is added to memory_used. Surely this should be: --- a/ext/opcache/zend_persist_calc.c +++ b/ext/opcache/zend_persist_calc.c @@ -31,7 +31,16 @@ #define ADD_SIZE(m) memory_used += ZEND_ALIGNED_SIZE(m) #define RETURN_SIZE() return memory_used -#if ZEND_EXTENSION_API_NO > PHP_5_3_X_API_NO +#if ZEND_EXTENSION_API_NO > PHP_5_5_X_API_NO +# define ADD_INTERNED_STRING(str, len) do { \ + const char *tmp = accel_new_interned_string((str), (len), !IS_INTERNED((str)) TSRMLS + if (tmp != (str)) { \ + (str) = (char*)tmp; \ + } else if (!IS_INTERNED(str)) { \ + ADD_DUP_SIZE((str), (len)); \ + } \ + } while (0) +#elif ZEND_EXTENSION_API_NO > PHP_5_3_X_API_NO # define ADD_INTERNED_STRING(str, len) do { \ const char *tmp = accel_new_interned_string((str), (len), !IS_INTERNED((str)) TSRMLS if (tmp != (str)) { \ This certainly fixed the 51 failing tests. The existing allocation logic works for PHP-5.3 - PHP-5.5 so I've only made the change for 5.6 == List of tests which fail == Zend/tests heredoc_008.phpt heredoc_015.phpt heredoc_016.phpt nowdoc_008.phpt nowdoc_015.phpt nowdoc_016.phpt ns_024.phpt ns_069.phpt Zend/tests/traits trait_constant_002.phpt ext/standard/tests/array array_filter_variation5.phpt array_flip_variation2.phpt array_flip_variation3.phpt array_rand_variation6.phpt array_unshift_variation9.phpt shuffle_variation5.phpt uasort_variation3.phpt uasort_variation5.phpt usort_variation5.phpt ext/standard/tests/file fscanf_variation14.phpt ext/standard/tests/general_functions is_string.phpt strval.phpt ext/standard/tests/strings addslashes_variation2.phpt chop_variation3.phpt chunk_split_variation12.phpt chunk_split_variation4.phpt htmlspecialchars_decode_variation3.phpt lcfirst.phpt sprintf_variation15.phpt str_replace_variation3.phpt str_split_variation5.phpt strcspn_variation5.phpt strcspn_variation6.phpt strcspn_variation7.phpt strcspn_variation8.phpt strip_tags_variation5.phpt stripos_variation7.phpt stripslashes_variation2.phpt strrchr_variation8.phpt strrpos_variation7.phpt strspn_variation5.phpt strspn_variation6.phpt strspn_variation7.phpt strspn_variation8.phpt strtok_variation3.phpt substr_count_variation_002.phpt ucfirst.phpt ucwords_variation2.phpt vfprintf_variation7.phpt vprintf_variation7.phpt vsprintf_variation7.phpt ------------------------------------------------------------------------ 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