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
Index: ext/session/session.c
===================================================================
--- ext/session/session.c (revision 309263)
+++ ext/session/session.c (working copy)
@@ -1953,6 +1953,13 @@
}
/* }}} */
+static ZEND_MODULE_PRE_DEACTIVATE_D(session) /* {{{ */
+{
+ php_session_flush(TSRMLS_C);
+ return SUCCESS;
+}
+/* }}} */
+
static PHP_RSHUTDOWN_FUNCTION(session) /* {{{ */
{
int i;
@@ -2373,7 +2380,8 @@
PHP_GINIT(ps),
NULL,
NULL,
- STANDARD_MODULE_PROPERTIES_EX
+ ZEND_MODULE_PRE_DEACTIVATE_N(session),
+ STANDARD_MODULE_PROPERTIES_EX2
};
#ifdef COMPILE_DL_SESSION
Index: Zend/zend.h
===================================================================
--- Zend/zend.h (revision 309263)
+++ Zend/zend.h (working copy)
@@ -645,6 +645,7 @@
void zend_call_destructors(TSRMLS_D);
void zend_activate_modules(TSRMLS_D);
void zend_deactivate_modules(TSRMLS_D);
+void zend_pre_deactivate_modules(TSRMLS_D);
void zend_post_deactivate_modules(TSRMLS_D);
#if ZEND_DEBUG
Index: Zend/zend_modules.h
===================================================================
--- Zend/zend_modules.h (revision 309263)
+++ Zend/zend_modules.h (working copy)
@@ -33,7 +33,7 @@
#define ZEND_MODULE_INFO_FUNC_ARGS zend_module_entry *zend_module TSRMLS_DC
#define ZEND_MODULE_INFO_FUNC_ARGS_PASSTHRU zend_module TSRMLS_CC
-#define ZEND_MODULE_API_NO 20100525
+#define ZEND_MODULE_API_NO 20110316
#ifdef ZTS
#define USING_ZTS 1
#else
@@ -48,7 +48,7 @@
#define ZEND_MODULE_BUILD_ID "API" ZEND_TOSTR(ZEND_MODULE_API_NO) ZEND_BUILD_TS ZEND_BUILD_DEBUG ZEND_BUILD_SYSTEM ZEND_BUILD_EXTRA
-#define STANDARD_MODULE_PROPERTIES_EX 0, 0, NULL, 0, ZEND_MODULE_BUILD_ID
+#define STANDARD_MODULE_PROPERTIES_EX2 0, 0, NULL, 0, ZEND_MODULE_BUILD_ID
#define NO_MODULE_GLOBALS 0, NULL, NULL, NULL
@@ -58,6 +58,8 @@
# define ZEND_MODULE_GLOBALS(module_name) sizeof(zend_##module_name##_globals), &module_name##_globals
#endif
+#define STANDARD_MODULE_PROPERTIES_EX NULL, STANDARD_MODULE_PROPERTIES_EX2
+
#define STANDARD_MODULE_PROPERTIES \
NO_MODULE_GLOBALS, NULL, STANDARD_MODULE_PROPERTIES_EX
@@ -94,6 +96,7 @@
void (*globals_ctor)(void *global TSRMLS_DC);
void (*globals_dtor)(void *global TSRMLS_DC);
int (*post_deactivate_func)(void);
+ int (*pre_deactivate_func)(void);
int module_started;
unsigned char type;
void *handle;
Index: Zend/zend_API.c
===================================================================
--- Zend/zend_API.c (revision 309263)
+++ Zend/zend_API.c (working copy)
@@ -2317,8 +2317,23 @@
}
/* }}} */
-static int exec_done_cb(zend_module_entry *module TSRMLS_DC) /* {{{ */
+static int pre_deactivate_cb(zend_module_entry *module TSRMLS_DC) /* {{{ */
{
+ if (module->pre_deactivate_func) {
+ module->pre_deactivate_func();
+ }
+ return 0;
+}
+/* }}} */
+
+void zend_pre_deactivate_modules(TSRMLS_D) /* {{{ */
+{
+ zend_hash_apply(&module_registry, (apply_func_t) pre_deactivate_cb TSRMLS_CC);
+}
+/* }}} */
+
+static int post_deactivate_cb(zend_module_entry *module TSRMLS_DC) /* {{{ */
+{
if (module->post_deactivate_func) {
module->post_deactivate_func();
}
@@ -2329,7 +2344,7 @@
void zend_post_deactivate_modules(TSRMLS_D) /* {{{ */
{
if (EG(full_tables_cleanup)) {
- zend_hash_apply(&module_registry, (apply_func_t) exec_done_cb TSRMLS_CC);
+ zend_hash_apply(&module_registry, (apply_func_t) post_deactivate_cb TSRMLS_CC);
zend_hash_reverse_apply(&module_registry, (apply_func_t) module_registry_unload_temp TSRMLS_CC);
} else {
zend_module_entry **p = module_post_deactivate_handlers;
Index: Zend/zend_API.h
===================================================================
--- Zend/zend_API.h (revision 309263)
+++ Zend/zend_API.h (working copy)
@@ -113,6 +113,7 @@
#define ZEND_MODULE_SHUTDOWN_N(module) zm_shutdown_##module
#define ZEND_MODULE_ACTIVATE_N(module) zm_activate_##module
#define ZEND_MODULE_DEACTIVATE_N(module) zm_deactivate_##module
+#define ZEND_MODULE_PRE_DEACTIVATE_N(module) zm_pre_deactivate_##module
#define ZEND_MODULE_POST_ZEND_DEACTIVATE_N(module) zm_post_zend_deactivate_##module
#define ZEND_MODULE_INFO_N(module) zm_info_##module
#define ZEND_MODULE_GLOBALS_CTOR_N(module) zm_globals_ctor_##module
@@ -123,6 +124,7 @@
#define ZEND_MODULE_SHUTDOWN_D(module) int ZEND_MODULE_SHUTDOWN_N(module)(SHUTDOWN_FUNC_ARGS)
#define ZEND_MODULE_ACTIVATE_D(module) int ZEND_MODULE_ACTIVATE_N(module)(INIT_FUNC_ARGS)
#define ZEND_MODULE_DEACTIVATE_D(module) int ZEND_MODULE_DEACTIVATE_N(module)(SHUTDOWN_FUNC_ARGS)
+#define ZEND_MODULE_PRE_DEACTIVATE_D(module) int ZEND_MODULE_PRE_DEACTIVATE_N(module)(SHUTDOWN_FUNC_ARGS)
#define ZEND_MODULE_POST_ZEND_DEACTIVATE_D(module) int ZEND_MODULE_POST_ZEND_DEACTIVATE_N(module)(void)
#define ZEND_MODULE_INFO_D(module) void ZEND_MODULE_INFO_N(module)(ZEND_MODULE_INFO_FUNC_ARGS)
#define ZEND_MODULE_GLOBALS_CTOR_D(module) void ZEND_MODULE_GLOBALS_CTOR_N(module)(zend_##module##_globals *module##_globals TSRMLS_DC)
Index: Zend/zend_execute_API.c
===================================================================
--- Zend/zend_execute_API.c (revision 309263)
+++ Zend/zend_execute_API.c (working copy)
@@ -198,24 +198,9 @@
}
/* }}} */
-static int zval_call_destructor(zval **zv TSRMLS_DC) /* {{{ */
-{
- if (Z_TYPE_PP(zv) == IS_OBJECT && Z_REFCOUNT_PP(zv) == 1) {
- return ZEND_HASH_APPLY_REMOVE;
- } else {
- return ZEND_HASH_APPLY_KEEP;
- }
-}
-/* }}} */
-
void shutdown_destructors(TSRMLS_D) /* {{{ */
{
zend_try {
- int symbols;
- do {
- symbols = zend_hash_num_elements(&EG(symbol_table));
- zend_hash_reverse_apply(&EG(symbol_table), (apply_func_t) zval_call_destructor TSRMLS_CC);
- } while (symbols != zend_hash_num_elements(&EG(symbol_table)));
zend_objects_store_call_destructors(&EG(objects_store) TSRMLS_CC);
} zend_catch {
/* if we couldn't destruct cleanly, mark all objects as destructed anyway */
Index: main/main.c
===================================================================
--- main/main.c (revision 309263)
+++ main/main.c (working copy)
@@ -1614,12 +1614,17 @@
php_call_shutdown_functions(TSRMLS_C);
} zend_end_try();
- /* 2. Call all possible __destruct() functions */
+ /* 2. Call all extension pre-RSHUTDOWN functions */
+ if (PG(modules_activated)) zend_try {
+ zend_pre_deactivate_modules();
+ } zend_end_try();
+
+ /* 3. Call all possible __destruct() functions */
zend_try {
zend_call_destructors(TSRMLS_C);
} zend_end_try();
- /* 3. Flush all output buffers */
+ /* 4. Flush all output buffers */
zend_try {
zend_bool send_buffer = SG(request_info).headers_only ? 0 : 1;
@@ -1637,18 +1642,18 @@
php_output_deactivate(TSRMLS_C);
} zend_end_try();
- /* 4. Send the set HTTP headers (note: This must be done AFTER php_output_discard_all() / php_output_end_all() !!) */
+ /* 5. Send the set HTTP headers (note: This must be done AFTER php_output_discard_all() / php_output_end_all() !!) */
zend_try {
sapi_send_headers(TSRMLS_C);
} zend_end_try();
- /* 5. Call all extensions RSHUTDOWN functions */
+ /* 6. Call all extensions RSHUTDOWN functions */
if (PG(modules_activated)) {
zend_deactivate_modules(TSRMLS_C);
php_free_shutdown_functions(TSRMLS_C);
}
- /* 6. Destroy super-globals */
+ /* 7. Destroy super-globals */
zend_try {
int i;
@@ -1659,7 +1664,7 @@
}
} zend_end_try();
- /* 6.5 free last error information */
+ /* 7.5 free last error information */
if (PG(last_error_message)) {
free(PG(last_error_message));
PG(last_error_message) = NULL;
@@ -1669,31 +1674,31 @@
PG(last_error_file) = NULL;
}
- /* 7. Shutdown scanner/executor/compiler and restore ini entries */
+ /* 8. Shutdown scanner/executor/compiler and restore ini entries */
zend_deactivate(TSRMLS_C);
- /* 8. Call all extensions post-RSHUTDOWN functions */
+ /* 9. Call all extensions post-RSHUTDOWN functions */
zend_try {
zend_post_deactivate_modules(TSRMLS_C);
} zend_end_try();
- /* 9. SAPI related shutdown (free stuff) */
+ /* 10. SAPI related shutdown (free stuff) */
zend_try {
sapi_deactivate(TSRMLS_C);
} zend_end_try();
- /* 10. Destroy stream hashes */
+ /* 11. Destroy stream hashes */
zend_try {
php_shutdown_stream_hashes(TSRMLS_C);
} zend_end_try();
- /* 11. Free Willy (here be crashes) */
+ /* 12. Free Willy (here be crashes) */
zend_try {
shutdown_memory_manager(CG(unclean_shutdown) || !report_memleaks, 0 TSRMLS_CC);
} zend_end_try();
zend_interned_strings_restore(TSRMLS_C);
- /* 12. Reset max_execution_time */
+ /* 13. Reset max_execution_time */
zend_try {
zend_unset_timeout(TSRMLS_C);
} zend_end_try();
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
