B3, pear segfault, and msgs about deprecated

Pierre-Alain Joye

22 years ago

On Thu, 18 Dec 2003 12:43:03 +0100
Pierre-Alain Joye <paj@pearfr.org> wrote:

> Hello,
>
> I'm trying to see why $ make install-pear-packages segfaults with
> HEAD. I hope I'll commit the fixes today. I suspect references
> problem...
>
> I got new messages (which where not displayed before):
> : var: Deprecated. Please use the public/private/protected modifiers
> in XXX.php on line XX
>
> Have these warning/notices been introduced recently?
>
> Or is it related to the e_pedantic thing?

Ok segfault is on line pear/PEAR/Registry.php line 345.

This is a call to unserialize. I do not know yet why it fails. Maybe
(again) the serialized data is corrupted. Anyway unserialize should
""never"" segfault.

hth

pierre

Derick Rethans

22 years ago

On Thu, 18 Dec 2003, Pierre-Alain Joye wrote:

> Ok segfault is on line pear/PEAR/Registry.php line 345.
>
> This is a call to unserialize. I do not know yet why it fails. Maybe
> (again) the serialized data is corrupted. Anyway unserialize should
> ""never"" segfault.

Can you put the datafile + serialize call in a test case?

Derick

Pierre-Alain Joye

22 years ago

On Thu, 18 Dec 2003 13:50:10 +0100 (CET)
Derick Rethans <derick@php.net> wrote:

> On Thu, 18 Dec 2003, Pierre-Alain Joye wrote:
>
> > Ok segfault is on line pear/PEAR/Registry.php line 345.
> >
> > This is a call to unserialize. I do not know yet why it fails. Maybe
> > (again) the serialized data is corrupted. Anyway unserialize should
> > ""never"" segfault.
>
> Can you put the datafile + serialize call in a test case?

I tried (see links below), but a simple test case does not segfault
with the same file.

Please note a weird notice about an undefined $data variable. The
problem is that $data is a function argument and is used in the 1st line
of the functon... Any idea?

One of the serialized data which cause the segfault:
http://www.pearfr.org/~paj/pear/packageserialized

A patch against /php-src/pear/ to see where that happens and to
reproduce the weird notice:
http://www.pearfr.org/~paj/pear/pear-install-seg.patch

A useless test case:
http://www.pearfr.org/~paj/pear/test_seg.php

It works perfectly, but the same file and code segfaults in PEAR.

A bit lost right now, memory issue?

pierre

Derick Rethans

22 years ago

On Thu, 18 Dec 2003, Pierre-Alain Joye wrote:

> On Thu, 18 Dec 2003 13:50:10 +0100 (CET)
> Derick Rethans <derick@php.net> wrote:
>
> > On Thu, 18 Dec 2003, Pierre-Alain Joye wrote:
> >
> > > Ok segfault is on line pear/PEAR/Registry.php line 345.
> > >
> > > This is a call to unserialize. I do not know yet why it fails. Maybe
> > > (again) the serialized data is corrupted. Anyway unserialize should
> > > ""never"" segfault.
> >
> > Can you put the datafile + serialize call in a test case?
>
> I tried (see links below), but a simple test case does not segfault
> with the same file.
>
> Please note a weird notice about an undefined $data variable. The
> problem is that $data is a function argument and is used in the 1st line
> of the functon... Any idea?
>
> One of the serialized data which cause the segfault:
> http://www.pearfr.org/~paj/pear/packageserialized
>
> A patch against /php-src/pear/ to see where that happens and to
> reproduce the weird notice:
> http://www.pearfr.org/~paj/pear/pear-install-seg.patch
>
> A useless test case:
> http://www.pearfr.org/~paj/pear/test_seg.php
>
> It works perfectly, but the same file and code segfaults in PEAR.
>
> A bit lost right now, memory issue?

Guesso... can you try to run it in valgrind, it should show all memory
overruns etc..

Derick

Pierre-Alain Joye

22 years ago

On Thu, 18 Dec 2003 14:44:29 +0100 (CET)
Derick Rethans <derick@php.net> wrote:

> Guesso... can you try to run it in valgrind, it should show all memory
> overruns etc..

Already done, no overuns, and obviously non freed mem after the crash ;)

Important note (dunno why I did not say that before :P ) It does not
crash in debug mode (bt ==> /dev/null :/), but:

/home/paj/cvs/builds/php5/Zend/zend_hash.c(504) : ht=0x40c09dc4 is being
destroyed

and valgrind still does see any problem.

pierre

Marcus Börger

22 years ago

Hello Pierre-Alain,

Thursday, December 18, 2003, 12:43:03 PM, you wrote:

> Hello,

> I'm trying to see why $ make install-pear-packages segfaults with HEAD.
> I hope I'll commit the fixes today. I suspect references problem...

> I got new messages (which where not displayed before):
> : var: Deprecated. Please use the public/private/protected modifiers in
> XXX.php on line XX

> Have these warning/notices been introduced recently?

> Or is it related to the e_pedantic thing?

marcus@zaphod /usr/src/php5 $ php -derror_reporting=4095 -r 'class t {var $x;}'

Strict Standards: var: Deprecated. Please use the public/private/protected modifiers in Command line code on line 1

marcus@zaphod /usr/src/php5 $ php -derror_reporting=2047 -r 'class t {var $x;}'

marcus@zaphod /usr/src/php5 $ php -r 'var_dump(E_STRICT);'
int(2048)

regards
marcus

Daniel Convissor

22 years ago

Hi Pierre:

On Thu, Dec 18, 2003 at 12:43:03PM +0100, Pierre-Alain Joye wrote:

>
> I got new messages (which where not displayed before):
> : var: Deprecated. Please use the public/private/protected modifiers in
> XXX.php on line XX

I noticed the same thing.  The new E_STRICT reporting level produces such
output.  But, E_STRICT is turned off by default.  Didn't have time to
track down exactly what was going on, but I have a hunch that it is due to
some of the PEAR install scripts set error_reporting by using numbers
rather than constants.

--Dan

--
FREE scripts that make web and database programming easier
http://www.analysisandsolutions.com/software/
T H E   A N A L Y S I S   A N D   S O L U T I O N S   C O M P A N Y
4015 7th Ave #4AJ, Brooklyn NY    v: 718-854-0335   f: 718-854-0409

Derick Rethans

22 years ago

On Thu, 18 Dec 2003, Daniel Convissor wrote:

> Hi Pierre:
>
> On Thu, Dec 18, 2003 at 12:43:03PM +0100, Pierre-Alain Joye wrote:
> >
> > I got new messages (which where not displayed before):
> > : var: Deprecated. Please use the public/private/protected modifiers in
> > XXX.php on line XX
>
> I noticed the same thing.  The new E_STRICT reporting level produces such
> output.  But, E_STRICT is turned off by default.  Didn't have time to
> track down exactly what was going on, but I have a hunch that it is due to
> some of the PEAR install scripts set error_reporting by using numbers
> rather than constants.

That shouldn't matter. As 2047 was the highest number before, this would
still exclude E_STRICT which is 2048. E_ALL also doesn't include it.

Derick

Greg Beaver

22 years ago

The only call to error_reporting() is in pearcmd.php:

error_reporting(E_ALL & ~E_NOTICE);

I added a var_dump(error_reporting()); and got 2039 out, as expected,
but I am still getting E_STRICT warnings galore.  I wonder if E_STRICT
is on no matter what in head?  I added & ~E_STRICT (which of course does
nothing) and STILL get the warnings.

Greg

Daniel Convissor wrote:

Daniel Convissor

22 years ago

On Thu, Dec 18, 2003 at 11:44:27AM -0500, Greg Beaver wrote:

> I wonder if E_STRICT is on no matter what in head?

It's not.  Something in the pear install scripts is turning it on.

--Dan

--
FREE scripts that make web and database programming easier
http://www.analysisandsolutions.com/software/
T H E   A N A L Y S I S   A N D   S O L U T I O N S   C O M P A N Y
4015 7th Ave #4AJ, Brooklyn NY    v: 718-854-0335   f: 718-854-0409

Greg Beaver

22 years ago

Unless it is possible to turn on E_STRICT without using the
error_reporting() function, then this is not true - there is only 1
error_reporting() in all of the .php files that make up the PEAR core,
try grep if you don't believe me.

Greg

Daniel Convissor wrote:

Daniel Convissor

22 years ago

Hi:

On Thu, Dec 18, 2003 at 12:08:09PM -0500, Greg Beaver wrote:

> Unless it is possible to turn on E_STRICT without using the
> error_reporting() function

It's the custom error handler in pearcmd.php.  PHP doesn't pay attention
to any error_reporting() settings when a custom error handler is
established.  Then, error_handler() doesn't account for the new warning
level.

So, the call to "error_reporting(E_ALL & ~E_NOTICE);" on line 36 can be
blown away because it's supurfluous and error_handler() needs tweaking.

--Dan

--
FREE scripts that make web and database programming easier
http://www.analysisandsolutions.com/software/
T H E   A N A L Y S I S   A N D   S O L U T I O N S   C O M P A N Y
4015 7th Ave #4AJ, Brooklyn NY    v: 718-854-0335   f: 718-854-0409

Christian Schneider

22 years ago

Daniel Convissor wrote:

> It's the custom error handler in pearcmd.php.  PHP doesn't pay attention
> to any error_reporting() settings when a custom error handler is
> established.  Then, error_handler() doesn't account for the new warning
> level.

Is it just me or is this undesired behaviour? I noticed this back when I
was playing around with error_handlers.

I think this should be treated as a bug and be changed for two reasons:
1) It seems illogical that an error_handler is called for an error level
which is disabled in php.ini
2) It's a performance issue since the custom error handler is called for
e.g. every access to an undefined array index even though I disabled
that warning.

Opinions?
- Chris

Brad Fisher

22 years ago

Personally, I both like and dislike this behavior.  I like it for debugging
purposes, since it allows me full control over whether I ignore
error_reporting level or perhaps just log disabled levels instead of
displaying them, etc.  On the other hand, there is a minor performance issue,
but if you write your code to not emit E_NOTICES, it shouldn't really be a big
problem.

-Brad

Christian Schneider wrote:

Derick Rethans

22 years ago

On Thu, 18 Dec 2003, Christian Schneider wrote:

> Daniel Convissor wrote:
> > It's the custom error handler in pearcmd.php.  PHP doesn't pay attention
> > to any error_reporting() settings when a custom error handler is
> > established.  Then, error_handler() doesn't account for the new warning
> > level.
>
> Is it just me or is this undesired behaviour? I noticed this back when I
> was playing around with error_handlers.

No, it's desired behavior.

> I think this should be treated as a bug and be changed for two reasons:
> 1) It seems illogical that an error_handler is called for an error level
> which is disabled in php.ini
> 2) It's a performance issue since the custom error handler is called for
> e.g. every access to an undefined array index even though I disabled
> that warning.

Performance and error handling don't go together. It's not a valid
argument and the current behavior IS the way it should be. (Please
search the archives for reasons, we discussed this before here).

Derick

Greg Beaver

22 years ago

At the least, this needs to be added to the thin-changes file, as it
will bork any script that uses a custom error handler and has PHP 4 objects.

Thanks for the catch Dan, that fixes it.

Greg

Christian Schneider wrote:

Derick Rethans

22 years ago

On Thu, 18 Dec 2003, Greg Beaver wrote:

> At the least, this needs to be added to the thin-changes file, as it
> will bork any script that uses a custom error handler and has PHP 4 objects.

It won't bork all scripts with a custom errorhandler if their default
action is to ignore core and other errors but only handle the ones they
should handle. I do agree that it should be mentioned in the changes
file though.

Derick

Christian Schneider

22 years ago

Derick Rethans wrote:

> It won't bork all scripts with a custom errorhandler if their default
> action is to ignore core and other errors but only handle the ones they
> should handle. I do agree that it should be mentioned in the changes
> file though.

On the other hand you have to be very careful (another way of saying
'avoid') using custom error_handlers anyway because

http://bugs.php.net/bug.php?id=25547

will break your application. Still there in PHP 5.0.0b3RC1

- Chris

Andi Gutmans

22 years ago

At 07:20 PM 12/18/2003 +0100, Christian Schneider wrote:

>Derick Rethans wrote:
>>It won't bork all scripts with a custom errorhandler if their default
>>action is to ignore core and other errors but only handle the ones they
>>should handle. I do agree that it should be mentioned in the changes
>>file though.
>
>On the other hand you have to be very careful (another way of saying
>'avoid') using custom error_handlers anyway because
>
>http://bugs.php.net/bug.php?id=25547
>
>will break your application. Still there in PHP 5.0.0b3RC1

Yeah this is a known bug and will have to be fixed in RC1. It's not a
showstopper for B3.
In any case, I do think that the custom error handler being called for any
error is bogus. Why should E_STRICT or E_NOTICES be sent to the handler if
they are off?
Anyway, I'm holding off with Beta 3 until we fix the
--install-pear-packages problem. I'd call that a showstopper because it's
something pretty basic.
Has anyone managed to create a reproducing script or backtrace?

Andi

Daniel Convissor

22 years ago

Greg:

On Thu, Dec 18, 2003 at 12:48:01PM -0500, Greg Beaver wrote:

>
> Thanks for the catch Dan, that fixes it.

But your fix uses the E_STRICT constant, which of course doesn't exist in
PHP 4, so an undefined constant notice gets produced.

I'm wondering why there's even a custom error handler in that file.  It
doesn't seem to be performing any purpose that isn't met with PHP's own
error handling procedures.

--Dan

--
FREE scripts that make web and database programming easier
http://www.analysisandsolutions.com/software/
T H E   A N A L Y S I S   A N D   S O L U T I O N S   C O M P A N Y
4015 7th Ave #4AJ, Brooklyn NY    v: 718-854-0335   f: 718-854-0409

Magnus Maatta

22 years ago

On Thursday 18 December 2003 18.25, Christian Schneider wrote:

> Daniel Convissor wrote:
> > It's the custom error handler in pearcmd.php.  PHP doesn't pay attention
> > to any error_reporting() settings when a custom error handler is
> > established.  Then, error_handler() doesn't account for the new warning
> > level.
>
> Is it just me or is this undesired behaviour? I noticed this back when I
> was playing around with error_handlers.
>
> I think this should be treated as a bug and be changed for two reasons:
> 1) It seems illogical that an error_handler is called for an error level
> which is disabled in php.ini
> 2) It's a performance issue since the custom error handler is called for
> e.g. every access to an undefined array index even though I disabled
> that warning.

I find this very useful and don't think it should be changed, maybe
it's just me.


/Magnus

--
Marriage is a lot like the army, everyone complains, but you'd be
surprised at the large number that re-enlist.
-- James Garner

Zeev Suraski

22 years ago

Any chance you can dissect it to the smallest piece of code that still
produces the crash?

At 14:46 18/12/2003, Pierre-Alain Joye wrote:

Johannes Schlueter

22 years ago

Hi,

as I wrote in my initial post: Even changing the paramters for
pear/install-pear.php "helps" and I am not able to create a simpler
script reproducing the crash. But meanwhile I've found the time when it
broke:
zend_execute.c,v 1.565 2003/12/14 16:09:07 still works but
zend_execute.c,v 1.566 2003/12/15 07:22:09 breaks. So this seems to be
the causing commit:
http://lists.php.net/article.php?group=php.zend-engine.cvs&article=2118

maybe it helps...

Zeev Suraski wrote:

Rob Richards

22 years ago

Did the problem end up being due to zend_execute.c 1.566 patch?

Within the zend_assign_to_object function, the temp variable needs to be
free'd, otherwise it needs to be handled within the extensions themselves.
Not sure about the other functions in that patch as I only ran into issues
with this function so far in some of the extensions.

ex.

$dom=new domDocument();
$dom->preserveWhiteSpace=false; <-  leaks

$array = simplexml_load_string('<array><data>sample data</data></array>');
$array->data = false; <- leaks and is valid syntax as it gets converted to
string

Within dom (pre 1.566 patch) , I had been handling this in my property write
function (removed the other day) via:
if (! PZVAL_IS_REF(value) && value->refcount == 0) {
value->refcount++;
zval_ptr_dtor(&value);
}

Does this need to go back in or is the code from the 1.566 patch going to be
revisited and re-implemented?

Rob

Andi Gutmans

22 years ago

At 06:44 AM 12/19/2003 -0500, Rob Richards wrote:

>Did the problem end up being due to zend_execute.c 1.566 patch?
>
>Within the zend_assign_to_object function, the temp variable needs to be
>free'd, otherwise it needs to be handled within the extensions themselves.
>Not sure about the other functions in that patch as I only ran into issues
>with this function so far in some of the extensions.
>
>ex.
>
>$dom=new domDocument();
>$dom->preserveWhiteSpace=false; <-  leaks
>
>$array = simplexml_load_string('<array><data>sample data</data></array>');
>$array->data = false; <- leaks and is valid syntax as it gets converted to
>string
>
>Within dom (pre 1.566 patch) , I had been handling this in my property write
>function (removed the other day) via:
>if (! PZVAL_IS_REF(value) && value->refcount == 0) {
>  value->refcount++;
>  zval_ptr_dtor(&value);
>}
>
>Does this need to go back in or is the code from the 1.566 patch going to be
>revisited and re-implemented?

We're going to have to revisit this problem for RC1. It definitely needs
solving. Right now I suggest you don't put the hack back into DOM and we'll
try and fix and well define the rules of how to return values and with what
reference count.

Andi