Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:1553
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Message-ID: <3EC43A1D.1040203@3gstech.com>
Date: Thu, 15 May 2003 18:08:45 -0700
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030312
MIME-Version: 1.0
Cc: internals@lists.php.net
References: <Pine.LNX.4.44.0305160227540.5992-100000@cs181147.pp.htv.fi>
In-Reply-To: <Pine.LNX.4.44.0305160227540.5992-100000@cs181147.pp.htv.fi>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] bug in set_error_handler
From: waboring@3gstech.com (walt boring)

Howdy,
  It seems the patch to fix the set_error_handler() didn't make it in 
the RC3 build.

set_error_handler( array(&$this, "errorhandler")) is broken.

This bug is also described in http://bugs.php.net/?id=23619 
<http://bugs.php.net/?id=23619&edit=2>
but it seems that the only intelligent response from sniper@php.net 
<mailto:sniper@php.net> seems to be
"this patch is bogus" and fails to explain himself at all.  This is not 
very productive, and is not helping to solve the issue.

  I have tried it in php 4.3.1 and php 4.3.2 RC2 and RC3.  It doesn't 
work.  I have provided a very simple php script which shows the 
problem.  the TestError::error() method is never called, and it should 
be.  You can comment the TestError error handler out, and use the 
global_error() function and see it working.

It is a very simple bug.  set_error_handler() assumes that the param 
passed in is a string, and tries to test the string the length, which is 
bogus in the case of it being an array.  Z_STRLEN_PP of an array?


<?php
ini_set("error_reporting" , E_ALL);
ini_set("display_errors", true);

// This handler works
function global_error($errno, $errstr, $errfile, $errline) {
    echo("global_error HANDLER CALLED (".$errstr.")\n");
}

class TestError {
    function TestError() {

        //This set_error_handler does not work.
        //it returns TRUE (which is wrong, because it never is set)
        $res = set_error_handler(array(&$this, "error"));
        echo("set_error_handler::error done retcode =" . 
var_export($res, true) . "\n");
    }

    //This handler FAILS to be called.
    function error($errno, $errstr, $errfile, $errline) {
        echo("TestError::error HANDLER CALLED (".$errstr.")\n");
    }

    function runTest() {
        $bar = $foo;   
    }
}

$t = new TestError();
//uncomment and use this handler to see it working.
//$res = set_error_handler("global_error");
//echo("set_error_handler::global_error done retcode =" . 
var_export($res, true) . "\n");
$t->runTest();
?>




The TestError::error handler fails to ever be called without the patch I 
provided, because
set_error_handler() on line 894
...
    if (Z_STRLEN_PP(error_handler)==0) { /* unset user-defined handler */
        FREE_ZVAL(EG(user_error_handler));
        EG(user_error_handler) = NULL;
        RETURN_TRUE;
    }
...
it assumes error_handler zval is a string.  It is an array.  Clearly 
doing a Z_STRLEN_PP on an array is completely wrong, and causes this to 
fall through.  Why are we returning TRUE in this case anyways, and isn't 
this error case handled in the is_callable() check anyway?  This code is 
broken, and will never work in its current state.

It should include a test to make sure its a string before trying to see 
what the string length is!


    if (Z_TYPE_PP(error_handler) == IS_STRING && 
Z_STRLEN_PP(error_handler)==0) { /* unset user-defined handler */
        FREE_ZVAL(EG(user_error_handler));
        EG(user_error_handler) = NULL;
        RETURN_TRUE;
    }

This prevents the test of the string length unless its actually a 
string!  go figure.


Walt