Bug #64291 [Com]: Indeterminate GC of evaled lambda function resources

Edit report at https://bugs.php.net/bug.php?id=64291&edit=1 ID: 64291 Comment by: Terry at ellisons dot org dot uk Reported by: Terry at ellisons dot org dot uk Summary: Indeterminate GC of evaled lambda function resources Status: Open Type: Bug Package: Scripting Engine problem Operating System: Ubuntu 12.10 PHP Version: 5.4.12 Block user comment: N Private report: N New Comment: Dmitry, I have just realised that this "mangled names should be unique" issue applied to any runtime bound function or class as the following -- albeit perverse example shows: --TEST-- ISSUE #65915A Temporary class entries are not unique --INI-- opcache.enable=0 --SKIPIF-- --FILE-- <?php $tmp = tempnam(__DIR__, 'test'); foreach (['a','b'] as $f) { file_put_contents($tmp, <<<END <?php function $f() { class Hello { const WORLD = "Hello world from $f\\n"; } } END ); require $tmp; } a(); echo Hello::WORLD; unlink($tmp); ?> --CLEAN-- --EXPECT-- Hello world from a Here the two functions both compile a class with a mangled name "\0$class$filename$string_addr" which is the same for the a() and b() copies so b() version overwrites the a() one, and the DECLARE_CLASS opcode in a() incorrecly binds to the wrong class, hence Hello::WORLD incorrectly prints out the "from b" version. However, I suspect in practice that this is unlikely to manifest itself in real word apps. Previous Comments: ------------------------------------------------------------------------ [2013-11-25 13:26:28] Terry at ellisons dot org dot uk Sorry, the above should read opcache.enable=0 though the failure is the same for opcache enabled and not enabled in the eval case. ------------------------------------------------------------------------ [2013-11-25 13:24:08] Terry at ellisons dot org dot uk And here's the eval version: --TEST-- ISSUE #64291 Temporary function entries for closures are not unique --INI-- opcache.enable=1 --FILE-- <?php foreach (['a','b'] as $f) { $tmp = "function $f() {return function(){ return '$f'; };}\n"; eval($tmp); echo $tmp; } $a = a(); $b = b(); printf( "%s, %s\n ", $a(), $b()); ?> --EXPECT-- function a() {return function(){ return 'a'; };} function b() {return function(){ return 'b'; };} a, b ------------------------------------------------------------------------ [2013-11-25 12:42:16] Terry at ellisons dot org dot uk What threw me was the botch with the temporary entries "\0{closure}$filenane$offset" are used in the EG(function_table). This is as clear as mud. When a file is compiled, a function table entry is created for each closue in the source. This entry is never executed directly, but is used by the ZEND_DECLARE_LAMBDA_FUNCTION to construct the closure object which contains a deep copy of this zend_function record. It is this copy that used when the closure is called. So long as the ZEND_DECLARE_LAMBDA_FUNCTION are executed within the same scope as the compile, this should normally be unique, but it is quite easy to construct a test case which the unique assumption fails: --TEST-- ISSUE #65915 Temporary function entries for closures are not unique --INI-- opcache.enable=0 --SKIPIF-- --FILE-- <?php $tmp = tempnam(__DIR__, 'test'); foreach (['a','b'] as $f) { file_put_contents($tmp, "<?php function $f() {return function(){ return '$f'; };}"); echo file_get_contents($tmp), "\n"; require $tmp; } $a = a(); $b = b(); printf( "%s, %s\n ", $a(), $b()); unlink($tmp); ?> --CLEAN-- --EXPECT-- <?php function a() {return function(){ return 'a'; };} <?php function b() {return function(){ return 'b'; };} a, b ------------------------------------------------------------------------ [2013-11-25 08:53:35] dmitry@php.net Your second script prints "a, a" only with OPCache, because it caches the included temporary file. Without OPCache it prints the expected "a, b". I also don't think that the first script indicates a bug. The more functions you create the more memory it requires. ------------------------------------------------------------------------ [2013-11-22 12:34:08] Terry at ellisons dot org dot uk Just to note that my patch is not strong enough when used with OPcache, since a simple static running count can fail when the interpreter is forked. Better alternatives include * some form of unique id, e.g. a UUID or uniquid(true) * a content based hash, such as md5file(__FILE__) -- though if this were to be adopted then it would be better always to generate this as part of the compile creating say __FILE_MD5__, though this would add a few % to the compile time. This all needs wider discussion of these issues on the Internals ML. ------------------------------------------------------------------------ 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=64291 -- Edit this bug report at https://bugs.php.net/bug.php?id=64291&edit=1