php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #66337 Wrong size calculation on optimization of class constants
Submitted: 2013-12-22 18:07 UTC Modified: 2014-01-09 10:39 UTC
From: Terry at ellisons dot org dot uk Assigned: dmitry (profile)
Status: Not a bug Package: opcache
PHP Version: master-Git-2013-12-22 (Git) OS: N/A
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: Terry at ellisons dot org dot uk
New email:
PHP Version: OS:

 

 [2013-12-22 18:07 UTC] Terry at ellisons dot org dot uk
Description:
------------
I've been trying the current 5.6 against apps such as Mediawiki and was getting Wrong size calculation errors logged.  After some horrible debugging I've reduced this down to a simple test case as attached.

When I run this:

Sun Dec 22 18:04:53 2013 (11692): Warning Internal error: wrong size calculation: /tmp/Terry_006.inc start=0x258a88d0, end=0x258a9088, real=0x258a9078

Now that I've got a simple repeatable test case, I'll degub the bug and post a patch.

Test script:
---------------
--TEST--
Wrong size calculation on optimization of class constants
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.optimization_level=-1
opcache.file_update_protection=0
--SKIPIF--
<?php if (!extension_loaded('Zend OPcache') || php_sapi_name() != "cli") die("skip CLI only"); ?>
--FILE--
<?php
class X { const A = "Constant"; }
$file = str_replace( ".php", ".inc", __file__ );

file_put_contents( $file, 
  '<?php class B { function __constuct() { $a = X::A; } }');

require $file;
new B;
@unlink($file);
echo "done\n";
?>
--EXPECT--
done


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2013-12-22 23:42 UTC] Terry at ellisons dot org dot uk
OK, the peephole optimizer for references to string class constants replaces the instruction sequence:

  FETCH_CONSTANT      ~n      'SomeClass', 'CONSTANT'
  ASSIGN                      !n, ~m

with

  ASSIGN                      !0, Interned_string(Value of SomeClass::CONSTANT)

The while loop at zend_persist.c:263 calls zend_persist_zval() which is effectively a NOOP for existing interned strings.

On the otherhand the corresponding loop at zend_persist_calc.c:172 calls zend_persist_zval_calc() to compute the size of the literal string.  This invokes the ADD_INTERNED_STRING() macro at zend_persist_calc.c:118.

However this logic is flawed is the string is ALREADY interned and it therefore incorrectly adds the size of the string to the computed size.  This triggers the subsequent "Wrong size calculation" warning.

I won't include the obvious fix here because I suspect that this exposes a couple of separate but related bugs which I want to explore and raise in their own bugreps.
 [2013-12-23 12:22 UTC] dmitry@php.net
-Assigned To: +Assigned To: dmitry
 [2013-12-23 12:22 UTC] dmitry@php.net
I can't reproduce this.
In my opinion
 [2013-12-23 12:23 UTC] dmitry@php.net
In my opinion optimizer shouldn't create interned strings.
 [2013-12-23 12:23 UTC] dmitry@php.net
-Status: Assigned +Status: Feedback
 [2013-12-23 13:12 UTC] Terry at ellisons dot org dot uk
-Status: Feedback +Status: Assigned
 [2013-12-23 13:12 UTC] Terry at ellisons dot org dot uk
OK, I'll refresh my dev snapshot from GiT -- it's a week out of date.  This might have been separately fixed by Xinchen. If so it's a pity because it took quite a few hours to locate the exact failure :-(  Let me examine and post back.
 [2013-12-23 16:32 UTC] Terry at ellisons dot org dot uk
Sorry my bad. You can close this one: the test works, now that I've synced my dev config to current PHP-5.6.  One of the updates to 5.6 in the last few has already fixed this.
 [2013-12-23 16:46 UTC] dmitry@php.net
-Status: Assigned +Status: Not a bug
 [2013-12-23 16:46 UTC] dmitry@php.net
It seems like it's a false alarm or the bug is already fixed.
 [2013-12-30 10:08 UTC] 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
 [2013-12-30 12:40 UTC] 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-31 05:35 UTC] 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-31 05:41 UTC] 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:49 UTC] 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.
 [2014-01-07 18:20 UTC] 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)); \
		} \
	}
 [2014-01-09 10:39 UTC] dmitry@php.net
Your patch is committed.
 
PHP Copyright © 2001-2020 The PHP Group
All rights reserved.
Last updated: Sun Sep 20 18:01:26 2020 UTC