Fix bug #32330 (session_destroy, "Failed to initialize storage module", custom session handler)

ext/session/mod_user.c

@@ -65,13 +65,16 @@ static zval *ps_call_handler(zval *func, int argc, zval **argv TSRMLS_DC)
 	return retval;
 }
 
-#define STDVARS 							\
+#define STDVARS1							\
 	zval *retval; 							\
-	int ret = FAILURE; 						\
-	ps_user *mdata = PS_GET_MOD_DATA();		\
+	int ret = FAILURE
+
+#define STDVARS								\
+	STDVARS1;								\
+	char *mdata = PS_GET_MOD_DATA();		\
 	if (!mdata) { return FAILURE; }
 
-#define PSF(a) mdata->name.ps_##a
+#define PSF(a) PS(mod_user_names).name.ps_##a
 
 #define FINISH 								\
 	if (retval) {							\
@@ -84,29 +87,32 @@ static zval *ps_call_handler(zval *func, int argc, zval **argv TSRMLS_DC)
 PS_OPEN_FUNC(user)
 {
 	zval *args[2];
-	STDVARS;
+	static char dummy = 0;
+	STDVARS1;
 	
 	SESS_ZVAL_STRING((char*)save_path, args[0]);
 	SESS_ZVAL_STRING((char*)session_name, args[1]);
 	
 	retval = ps_call_handler(PSF(open), 2, args TSRMLS_CC);
+	if (retval) {
+		/* This is necessary to fool the session module. Yes, it's safe to
+		 * use a static. Neither mod_user nor the session module itself will
+		 * ever touch this pointer. It could be set to 0xDEADBEEF for all the
+		 * difference it makes, but for the sake of paranoia it's set to some
+		 * valid value.
+		*/
+		PS_SET_MOD_DATA(&dummy);
+	}
 	
 	FINISH;
 }
 
 PS_CLOSE_FUNC(user)
 {
-	int i;
-	STDVARS;
+	STDVARS1;
 
 	retval = ps_call_handler(PSF(close), 0, NULL TSRMLS_CC);
 
-	for (i = 0; i < 6; i++) {
-		zval_ptr_dtor(&mdata->names[i]);
-	}
-
-	efree(mdata);
-
 	PS_SET_MOD_DATA(NULL);
 
 	FINISH;

ext/session/mod_user.h

@@ -21,18 +21,6 @@
 #ifndef MOD_USER_H
 #define MOD_USER_H
 
-typedef union {
-	zval *names[6];
-	struct {
-		zval *ps_open;
-		zval *ps_close;
-		zval *ps_read;
-		zval *ps_write;
-		zval *ps_destroy;
-		zval *ps_gc;
-	} name;
-} ps_user;
-
 extern ps_module ps_mod_user;
 #define ps_user_ptr &ps_mod_user
 

ext/session/php_session.h

@@ -116,6 +116,17 @@ typedef struct _php_ps_globals {
 	long gc_maxlifetime;
 	int module_number;
 	long cache_expire;
+	union {
+		zval *names[6];
+		struct {
+			zval *ps_open;
+			zval *ps_close;
+			zval *ps_read;
+			zval *ps_write;
+			zval *ps_destroy;
+			zval *ps_gc;
+		} name;
+	} mod_user_names;
 	const struct ps_serializer_struct *serializer;
 	zval *http_session_vars;
 	zend_bool auto_start;

ext/session/session.c

@@ -82,6 +82,7 @@ static inline void php_rinit_session_globals(TSRMLS_D)
 	PS(id) = NULL;
 	PS(session_status) = php_session_none;
 	PS(mod_data) = NULL;
+	/* Do NOT init PS(mod_user_names) here! */
 	PS(http_session_vars) = NULL;
 }
 
@@ -92,6 +93,7 @@ static inline void php_rshutdown_session_globals(TSRMLS_D)
 		zval_ptr_dtor(&PS(http_session_vars));
 		PS(http_session_vars) = NULL;
 	}
+	/* Do NOT destroy PS(mod_user_names) here! */
 	if (PS(mod_data)) {
 		zend_try {
 			PS(mod)->s_close(&PS(mod_data) TSRMLS_CC);
@@ -1476,7 +1478,6 @@ static PHP_FUNCTION(session_set_save_handler)
 {
 	zval **args[6];
 	int i;
-	ps_user *mdata;
 	zval name;
 
 	if (PS(session_status) != php_session_none) {
@@ -1499,14 +1500,13 @@ static PHP_FUNCTION(session_set_save_handler)
 	
 	zend_alter_ini_entry("session.save_handler", sizeof("session.save_handler"), "user", sizeof("user")-1, PHP_INI_USER, PHP_INI_STAGE_RUNTIME);
 
-	mdata = emalloc(sizeof(*mdata));
-	
 	for (i = 0; i < 6; i++) {
+		if (PS(mod_user_names).names[i] != NULL) {
+			zval_ptr_dtor(&PS(mod_user_names).names[i]);
+		}
 		Z_ADDREF_P(*args[i]);
-		mdata->names[i] = *args[i];
+		PS(mod_user_names).names[i] = *args[i];
 	}
-
-	PS(mod_data) = (void *) mdata;
 	
 	RETURN_TRUE;
 }
@@ -1830,8 +1830,16 @@ static PHP_RINIT_FUNCTION(session)
 
 static PHP_RSHUTDOWN_FUNCTION(session)
 {
+	int i;
+	
 	php_session_flush(TSRMLS_C);
 	php_rshutdown_session_globals(TSRMLS_C);
+	/* this should NOT be done in php_rshutdown_session_globals() */
+	for (i = 0; i < 6; i++) {
+		if (PS(mod_user_names).names[i] != NULL) {
+			zval_ptr_dtor(&PS(mod_user_names).names[i]);
+		}
+	}
 
 	return SUCCESS;
 }
@@ -1839,12 +1847,17 @@ static PHP_RSHUTDOWN_FUNCTION(session)
 
 static PHP_GINIT_FUNCTION(ps)
 {
+	int i;
+	
 	ps_globals->save_path = NULL;
 	ps_globals->session_name = NULL;
 	ps_globals->id = NULL;
 	ps_globals->mod = NULL;
 	ps_globals->mod_data = NULL;
 	ps_globals->session_status = php_session_none;
+	for (i = 0; i < 6; i++) {
+		ps_globals->mod_user_names.names[i] = NULL;
+	}
 	ps_globals->http_session_vars = NULL;
 }
 

ext/session/tests/bug32330.phpt

@@ -0,0 +1,101 @@
+--TEST--
+Bug #32330 (session_destroy, "Failed to initialize storage module", custom session handler)
+--SKIPIF--
+<?php include('skipif.inc'); ?>
+--INI--
+session.use_trans_sid=0
+session.use_cookies=1
+session.name=sid
+session.save_path=/
+session.gc_probability=1
+session.gc_divisor=1
+--FILE--
+<?php
+error_reporting(E_ALL);
+
+function sOpen($path, $name)
+{
+    echo "open: path = {$path}, name = {$name}\n";
+    return TRUE;
+}
+
+function sClose()
+{
+    echo "close\n";
+    return TRUE;
+}
+
+function sRead($id)
+{
+    echo "read: id = {$id}\n";
+    return '';
+}
+
+function sWrite($id, $data)
+{
+    echo "write: id = {$id}, data = {$data}\n";
+    return TRUE;
+}
+
+function sDestroy($id)
+{
+    echo "destroy: id = {$id}\n";
+    return TRUE;
+}
+
+function sGC($maxlifetime)
+{
+    echo "gc: maxlifetime = {$maxlifetime}\n";
+    return TRUE;
+}
+
+session_set_save_handler( 'sOpen', 'sClose', 'sRead', 'sWrite', 'sDestroy', 'sGC' );
+
+// without output buffering, the debug messages will cause all manner of warnings
+ob_start();
+
+session_start();
+$_SESSION['A'] = 'B';
+session_write_close();
+
+session_start();
+$_SESSION['C'] = 'D';
+session_destroy();
+
+session_start();
+$_SESSION['E'] = 'F';
+// Don't try to destroy this time!
+
+?>
+--EXPECTF--
+open: path = /, name = sid
+read: id = %s
+gc: maxlifetime = %d
+write: id = %s, data = A|S:1:"B";
+close
+open: path = /, name = sid
+read: id = %s
+gc: maxlifetime = %d
+destroy: id = %s
+close
+open: path = /, name = sid
+read: id = %s
+gc: maxlifetime = %d
+write: id = %s, data = E|S:1:"F";
+close
+--UEXPECTF--
+open: path = /, name = sid
+read: id = %s
+gc: maxlifetime = %d
+write: id = %s, data = A|U:1:"B";
+close
+open: path = /, name = sid
+read: id = %s
+gc: maxlifetime = %d
+destroy: id = %s
+close
+open: path = /, name = sid
+read: id = %s
+gc: maxlifetime = %d
+write: id = %s, data = E|U:1:"F";
+close