Everything

php

16-bit reference count in PHP 4's zval

php.internals

Tim Starling

20 years ago

I've had some trouble with large arrays in my PHP programs causing
corruption of the heap. I tracked the problem back to PHP 4's 16-bit
reference count. If you have more than 64K references to a given zval, the
counter will overflow, then when the references are freed, the object will
be double-freed, causing a segfault. Dangling references are also possible,
allowing reading and writing of subsequently allocated blocks.

No doubt the PHP dev team are aware of this already, since it was fixed in
PHP 5, by using a 32-bit type instead. My question is: is there any
intention to backport this simple but important bugfix to PHP 4? Many PHP
users are still using PHP 4, and it's not a very well advertised fact that
it does not properly support arrays with more than 64K entries.

-- Tim Starling

Marcus Börger

20 years ago

Hello Tim,

that's a major API break so we would need to make that 4.5. Which is kind
of out of question atm. Just change to 5.

marcus

Saturday, January 7, 2006, 5:47:27 AM, you wrote:

> I've had some trouble with large arrays in my PHP programs causing
> corruption of the heap. I tracked the problem back to PHP 4's 16-bit
> reference count. If you have more than 64K references to a given zval, the
> counter will overflow, then when the references are freed, the object will
> be double-freed, causing a segfault. Dangling references are also possible,
> allowing reading and writing of subsequently allocated blocks.

> No doubt the PHP dev team are aware of this already, since it was fixed in
> PHP 5, by using a 32-bit type instead. My question is: is there any
> intention to backport this simple but important bugfix to PHP 4? Many PHP
> users are still using PHP 4, and it's not a very well advertised fact that
> it does not properly support arrays with more than 64K entries.

> -- Tim Starling

Best regards,
Marcus

Zeev Suraski

20 years ago

At 06:47 07/01/2006, Tim Starling wrote:

>I've had some trouble with large arrays in my PHP programs causing
>corruption of the heap. I tracked the problem back to PHP 4's 16-bit
>reference count. If you have more than 64K references to a given zval, the
>counter will overflow, then when the references are freed, the object will
>be double-freed, causing a segfault. Dangling references are also possible,
>allowing reading and writing of subsequently allocated blocks.
>
>No doubt the PHP dev team are aware of this already, since it was fixed in
>PHP 5, by using a 32-bit type instead. My question is: is there any
>intention to backport this simple but important bugfix to PHP 4? Many PHP
>users are still using PHP 4, and it's not a very well advertised fact that
>it does not properly support arrays with more than 64K entries.

Tim,

Your analysis was correct until the last sentence - PHP surely does
support arrays with more than 64K entries.  It just doesn't support
the same entry being linked from more than 64K locations (which is
much, much more rare occurrence).

Zeev

Tim Starling

20 years ago

Zeev Suraski wrote:

> Tim,
>
> Your analysis was correct until the last sentence - PHP surely does
> support arrays with more than 64K entries.  It just doesn't support the
> same entry being linked from more than 64K locations (which is much,
> much more rare occurrence).

Well yes, I was just stirring. It's more common than you might think though.
My own case was somewhat similar to this:

<?
class CacheEntry {
var $name;
var $stuff = array();

function CacheEntry( $name ) {
$this->name = $name;
}
}

$cache = array();
for ( $i=0; $i<100000; $i++) {
$cache[] = new CacheEntry( "Cache entry $i" );
}
?>

...which will segfault on exit due to the excess references to the empty
array object created for the initialisation of the unused member variable
$stuff. Generally speaking, arrays of objects with initialised but unused
member variables will cause problems. Our code (MediaWiki) has plenty of
such arrays, the reason we don't see this more often is because the arrays
usually contain less than 64K entries. The size of the arrays depend on user
input.

-- Tim Starling

Zeev Suraski

20 years ago

At 00:00 09/01/2006, Tim Starling wrote:

>Zeev Suraski wrote:
>  > Tim,
> >
> > Your analysis was correct until the last sentence - PHP surely does
> > support arrays with more than 64K entries.  It just doesn't support the
> > same entry being linked from more than 64K locations (which is much,
> > much more rare occurrence).
>
>Well yes, I was just stirring. It's more common than you might think though.

I'm not saying that it's four-leafed clover, just that it's
significantly more rare than plain arrays that contain 64K
(different) entries...
We did decide to increase the refcount size in PHP 5, but as Marcus
said, releasing a binary-compat-breaking version of 4 at this stage
is pretty much out of the question.

Zeev

Alan Pinstein

20 years ago

It seems that the problem you're encountering could be fixed
trivially by moving the initialization of $stuff into the constructor.

However, it's difficult to tell from the provided code whether $stuff
is intended as a static member...  but if it's not, seems like a very
simple fix...

I don't personally know about PHP4 internals, but it would seem that
the reason it breaks as-is is that ALL CacheEntry objects initially
report to the same (statically created) empty array. So, moving it
into the constructor would at least create n empty arrays rather than
segfaulting.

Right? Then you wouldn't even need to change your runtime environment
(ie PHP version) to solve the problem.

Alan

On Jan 8, 2006, at 5:00 PM, Tim Starling wrote:

Tim Starling

20 years ago

Alan Pinstein wrote:

> It seems that the problem you're encountering could be fixed  trivially
> by moving the initialization of $stuff into the constructor.
>
> However, it's difficult to tell from the provided code whether $stuff
> is intended as a static member...  but if it's not, seems like a very
> simple fix...
>
> I don't personally know about PHP4 internals, but it would seem that
> the reason it breaks as-is is that ALL CacheEntry objects initially
> report to the same (statically created) empty array. So, moving it  into
> the constructor would at least create n empty arrays rather than
> segfaulting.
>
> Right? Then you wouldn't even need to change your runtime environment
> (ie PHP version) to solve the problem.

The easy way to solve it was to limit the array to less than 64K entries,
which I did before I made my original post. The problem only showed up
because of a bug causing the cache to grow in an unconstrained way. Thanks
for the suggestion, but the reason I started this thread was because I
wanted to make sure that this doesn't happen again, regardless of bad
programming.

Upgrading to PHP 5 is certainly a reasonable solution, it's been on our
wishlist for a while. We will probably drop support for PHP 4 in MediaWiki
1.6 or 1.7, so that we can take advantage of the new features. Upgrading
won't be a trivial task, we're apparently one of the larger PHP
installations in the world, with about 100 servers running PHP, and there's
a number of custom extensions to check as well. Still, it has to be done
some time.

-- Tim Starling

Jani Taskinen

20 years ago

On Tue, 10 Jan 2006, Tim Starling wrote:

> Upgrading to PHP 5 is certainly a reasonable solution, it's been on our
> wishlist for a while. We will probably drop support for PHP 4 in MediaWiki
> 1.6 or 1.7, so that we can take advantage of the new features. Upgrading

We've used MediaWiki at work with PHP 5.1.x (from CVS) for a long time
without any problems. I'd suggest you just start another branch of it
e.g. 2.0 which is for PHP 5.1 only. Then just don't upgrade 1.x anymore. :)
People will just have to upgrade. They just don't know it's for their
own good yet..

--Jani