php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #66440 Optimisation of conditional JMPs based on pre-evaluate constant function calls
Submitted: 2014-01-08 13:19 UTC Modified: 2014-01-13 10:14 UTC
From: Terry at ellisons dot org dot uk Assigned: dmitry (profile)
Status: Closed Package: opcache
PHP Version: master-Git-2014-01-08 (Git) OS: N/A
Private report: No CVE-ID: None
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If this is not your bug, you can add a comment by following this link.
If this is your bug, but you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: Terry at ellisons dot org dot uk
New email:
PHP Version: OS:

 

 [2014-01-08 13:19 UTC] Terry at ellisons dot org dot uk
Description:
------------
I was hitting this memory error in run-test.php itself if run with optimization enabled.  So I used gdb to work out what in this script was being optimized at the error and used this to generate the attached minimal PHPT test which show the error.

The error is caused by the coupling of two factors:  

* pass1_5.c:435 calls zend_get_persistent_constant() to obtain the constant value.  However, this routine (block_pass.c:11&16) look this symbol up in EG(zend_constants) and return this value.  This is the wrong copy to use as it is not interned within OPcache.  In fact accel_use_shm_interned_strings() has already cloned all EG(zend_constants) into ZCSG(interned_strings), and it is this copy that should be referenced -- for example by passing the return value through accel_new_interned_string().

* pass2.c:106 The JMPZ, JMPNZ processing optimizes the conditional into a absolute JMP or NOP so that block_pass can remove the dead code.  However it also does a literal_dtor(&ZEND_OP1_LITERAL(opline)) on the now unused literal.  This executes _zval_dtor_func() on the zval which then str_efree_rel() the string, which is a bit of a disaster since it wasn't emalloced in the first place.

I really need to question the logic for the widescale use of literal_dtor().  What are you trying to do here?  Surely this is just a case of two mistakes sort of cancelling each other out.  All literals should have been interned by this stage so doing a zval_dtor() is unnecessary and efreeing string which in is_ref zvals with rc > 1 is crazy anyway.  Yes the literal slots need to be compacted out in compact_literals.c, but isn't setting Z_TYPE_P(zv) to IS_NULL adequate here?

So one simple but to fix and a review of the use of literal_dtor() is needed 

Test script:
---------------
--TEST--
Check pre-evaluate constant function call on branch optimization
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.optimization_level=-1
--SKIPIF--
<?php require_once('skipif.inc'); ?>
--FILE--
<?php
if(constant('PHP_BINARY')) {
	echo "OK\n";
}
?>
--EXPECTF--
OK

Expected result:
----------------
As per phpt

Actual result:
--------------
Generates a memory corruption error on debug build of php, e.g.

---------------------------------------
./Optimizer/pass2.c(111) : Block 0x01fa85e0 status:
/home/terry/work/php56/Zend/zend_variables.c(37) : Actual location (location was relayed)
Invalid pointer: ((size=0x00000041) != (next.prev=0x7265742f7261762f))
Invalid pointer: ((prev=0x00000008) != (prev.size=0x455a4953))
---------------------------------------


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2014-01-08 14:49 UTC] Terry at ellisons dot org dot uk
I've just done a quick fix, but this only addresses this single dangerous use of
literal.  The wider review is a separate issue.
_
--- a/Optimizer/pass2.c
+++ b/Optimizer/pass2.c
@@ -108,7 +108,7 @@ if (ZEND_OPTIMIZER_PASS_2 & OPTIMIZATION_LEVEL) {
                                        if (opline->opcode == ZEND_JMPZ) {
                                                should_jmp = !should_jmp;
                                        }
-                                       literal_dtor(&ZEND_OP1_LITERAL(opline));
+                                       Z_TYPE(ZEND_OP1_LITERAL(opline)) = IS_NULL;
                                        ZEND_OP1_TYPE(opline) = IS_UNUSED;
                                        if (should_jmp) {
                                                opline->opcode = ZEND_JMP;
 [2014-01-13 10:14 UTC] dmitry@php.net
-Assigned To: +Assigned To: dmitry
 [2014-01-13 12:17 UTC] dmitry@php.net
Automatic comment on behalf of dmitry@zend.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=9b74dc4a07a2c5178cbedc41b0beedd979537ffa
Log: Fixed bug #66440 (Optimisation of conditional JMPs based on pre-evaluate constant function calls)
 [2014-01-13 12:17 UTC] dmitry@php.net
-Status: Assigned +Status: Closed
 [2014-01-13 13:30 UTC] ab@php.net
Automatic comment on behalf of dmitry@zend.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=9b74dc4a07a2c5178cbedc41b0beedd979537ffa
Log: Fixed bug #66440 (Optimisation of conditional JMPs based on pre-evaluate constant function calls)
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Mar 28 14:01:29 2024 UTC