[patch] Session RSHUTDOWN, shutdown_destructors() issues

2011/3/16 Tim Starling tstarling@wikimedia.org

This is a followup to http://bugs.php.net/bug.php?id=54157 . Johannes
said I should post here.

In the course of my MediaWiki development work, I hit a strange issue
involving session_set_save_handler().

PHP's built-in session handler is pretty much useless when you have
more than one server serving a given site, so
session_set_save_handler() is essential. MediaWiki has a feature
allowing it to save sessions via its memcached client. A suitable
client object is put in $wgMemc during setup, which is used when the
session is closed. A minor change to the way this variable was
initialised caused it to disappear from the global symbol table before
the session save handler was called.

This turned out to be due to a broken algorithm in
shutdown_destructors() in zend_execute_API.c. The algorithm basically
does this:

1. Delete all objects from the global symbol table which have a zval
   reference count of 1
2. If any objects were deleted, go to step 1.

Obviously the intention is to first delete the references, and then to
delete the objects which were referred to. It doesn't work that way:
if you have a reference, both symbol table entries point to the same
zval, so they both have a reference count of 2, so the algorithm skips
both and leaves them intact for RSHUTDOWN. My $wgMemc initialisation
change caused the reference count to drop from 2 to 1, so the variable
was deleted.

Fixing this function to correctly delete all objects from the global
symbol table would be easy enough (against 5.3):

http://tstarling.com/stuff/shutdown_destructors-sanity.patch

But please don't apply that, because it would break all released
versions of MediaWiki.

We now come to the next strange thing about this code: why is it
attempting to delete all objects from the global symbol table anyway?
There are plenty of other ways to store objects: function static
variables, class static variables, etc. Deleting them from $GLOBALS
doesn't stop them from being accessed. And besides, we know that
there's no problem with accessing an object after __destruct() has
been called on it, because that was the whole point of splitting out
shutdown_destructors() in the first place, as a comment in
shutdown_executor() explains:

/* Removed because this can not be safely done, e.g. in this situation:
Object 1 creates object 2
Object 3 holds reference to object 2.
Now when 1 and 2 are destroyed, 3 can still access 2 in its
destructor, with
very problematic results /
/ zend_objects_store_call_destructors(&EG(objects_store)
TSRMLS_CC); */

Just removing the whole loop would suit me, as a 5.3.x stopgap measure:

http://tstarling.com/stuff/no-global-deletion.patch

But that still leaves the problem that the session save handler is
called in the strange and scary world of half-shut-down PHP. We don't
know what RSHUTDOWN functions have been called before the session
RSHUTDOWN, so we don't know what userspace code will work and what
won't. A workaround is easy enough:

http://www.mediawiki.org/wiki/Special:Code/MediaWiki/83147

but I thought I would be a good open source citizen and come up with a
proper solution, against trunk. It is attached and at:

http://tstarling.com/stuff/session-pre-deactivate.patch

The idea is to introduce a pre-RSHUTDOWN hook, allowing modules to
call user code in a relatively sane environment. It's implemented in a
way that is closely analogous to the post-deactivate hook. I had
trouble testing it because trunk was so broken that it wouldn't run
MediaWiki, but the patch appears to work for simple CLI test cases.

-- Tim Starling

--

Hi.

Could you please also open a ticket in the bug tracker?
thanks

Tyrael

I guess I'm suffering from some tl;dr here. Here's the short version:
please apply the attached patch to the 5.3 branch. It removes some
code which is broken and unnecessary.