Properly handle serialization locking

ext/session/session.c

@@ -243,11 +243,21 @@ static zend_string *php_session_encode(void) /* {{{ */
 
 static int php_session_decode(zend_string *data) /* {{{ */
 {
+	int res;
+	struct php_unserialize_data *prev_data;
+	unsigned prev_lock_level;
+
 	if (!PS(serializer)) {
 		php_error_docref(NULL, E_WARNING, "Unknown session.serialize_handler. Failed to decode session object");
 		return FAILURE;
 	}
-	if (PS(serializer)->decode(ZSTR_VAL(data), ZSTR_LEN(data)) == FAILURE) {
+	/* Make sure that any uses of unserialize() during session decoding do not share
+	 * state with any unserialize() that is already in progress (e.g. because we are
+	 * currently inside Serializable::unserialize(). */
+	PHP_VAR_UNSERIALIZE_LOCK(prev_data, prev_lock_level);
+	res = PS(serializer)->decode(ZSTR_VAL(data), ZSTR_LEN(data));
+	PHP_VAR_UNSERIALIZE_UNLOCK(prev_data, prev_lock_level);
+	if (res == FAILURE) {
 		php_session_destroy();
 		php_session_track_init();
 		php_error_docref(NULL, E_WARNING, "Failed to decode session object. Session has been destroyed");

ext/standard/basic_functions.c

@@ -909,7 +909,6 @@ static void basic_globals_ctor(php_basic_globals *basic_globals_p) /* {{{ */
 	BG(left) = -1;
 	BG(user_tick_functions) = NULL;
 	BG(user_filter_map) = NULL;
-	BG(serialize_lock) = 0;
 
 	memset(&BG(serialize), 0, sizeof(BG(serialize)));
 	memset(&BG(unserialize), 0, sizeof(BG(unserialize)));
@@ -1160,7 +1159,6 @@ PHP_RINIT_FUNCTION(basic) /* {{{ */
 {
 	memset(BG(strtok_table), 0, 256);
 
-	BG(serialize_lock) = 0;
 	memset(&BG(serialize), 0, sizeof(BG(serialize)));
 	memset(&BG(unserialize), 0, sizeof(BG(unserialize)));
 

ext/standard/basic_functions.h

@@ -202,14 +202,15 @@ typedef struct _php_basic_globals {
 
 	/* var.c */
 	zend_class_entry *incomplete_class;
-	unsigned serialize_lock; /* whether to use the locally supplied var_hash instead (__sleep/__wakeup) */
 	struct {
 		struct php_serialize_data *data;
 		unsigned level;
+		unsigned lock_level;
 	} serialize;
 	struct {
 		struct php_unserialize_data *data;
 		unsigned level;
+		unsigned lock_level;
 	} unserialize;
 
 	/* url_scanner_ex.re */

ext/standard/php_var.h

@@ -66,6 +66,26 @@ PHPAPI zend_long php_var_unserialize_get_cur_depth(php_unserialize_data_t d);
 #define PHP_VAR_UNSERIALIZE_DESTROY(d) \
 	php_var_unserialize_destroy(d)
 
+#define PHP_VAR_SERIALIZE_LOCK(prev_data, prev_lock_level) \
+	prev_data = BG(serialize).data; \
+	prev_lock_level = BG(serialize).lock_level; \
+	BG(serialize).data = NULL; \
+	BG(serialize).lock_level = BG(serialize).level;
+
+#define PHP_VAR_SERIALIZE_UNLOCK(prev_data, prev_lock_level) \
+	BG(serialize).data = prev_data; \
+	BG(serialize).lock_level = prev_lock_level;
+
+#define PHP_VAR_UNSERIALIZE_LOCK(prev_data, prev_lock_level) \
+	prev_data = BG(unserialize).data; \
+	prev_lock_level = BG(unserialize).lock_level; \
+	BG(unserialize).data = NULL; \
+	BG(unserialize).lock_level = BG(unserialize).level;
+
+#define PHP_VAR_UNSERIALIZE_UNLOCK(prev_data, prev_lock_level) \
+	BG(unserialize).data = prev_data; \
+	BG(unserialize).lock_level = prev_lock_level;
+
 PHPAPI void var_replace(php_unserialize_data_t *var_hash, zval *ozval, zval *nzval);
 PHPAPI void var_push_dtor(php_unserialize_data_t *var_hash, zval *val);
 PHPAPI zval *var_tmp_var(php_unserialize_data_t *var_hashx);

ext/standard/tests/serialize/bug70219_1.phpt

@@ -18,6 +18,7 @@ class obj implements Serializable {
     }
     function unserialize($data) {
         session_decode($data);
+        return null;
     }
 }
 
@@ -33,20 +34,18 @@ for ($i = 0; $i < 5; $i++) {
 var_dump($data);
 var_dump($_SESSION);
 ?>
---EXPECTF--
+--EXPECT--
 array(2) {
   [0]=>
-  object(obj)#%d (1) {
+  object(obj)#1 (1) {
     ["data"]=>
     NULL
   }
   [1]=>
-  object(obj)#%d (1) {
+  object(obj)#2 (1) {
     ["data"]=>
     NULL
   }
 }
-object(obj)#1 (1) {
-  ["data"]=>
-  NULL
+array(0) {
 }

ext/standard/tests/serialize/nested_serialize_under_lock.phpt

@@ -0,0 +1,64 @@
+--TEST--
+Check that nested serialization/unserialization still works correctly while under lock
+--FILE--
+<?php
+
+class SerializableClass implements Serializable {
+    public $sharedProp;
+    public function __construct($prop)
+    {
+        $this->sharedProp = $prop;
+    }
+    public function __set($key, $value)
+    {
+        $this->$key = $value;
+    }
+    public function serialize()
+    {
+        return serialize(get_object_vars($this));
+    }
+    public function unserialize($data)
+    {
+        $ar = unserialize($data);
+        if ($ar === false) {
+            return;
+        }
+        foreach ($ar as $k => $v) {
+            $this->__set($k, $v);
+        }
+    }
+}
+
+// Going through spl_autoload_register() to lock serialization
+spl_autoload_register(function($class) {
+    $testPropertyObj = new stdClass();
+    $testPropertyObj->name = 'test';
+    $array = [
+        'obj1' => new SerializableClass($testPropertyObj),
+        'obj2' => new SerializableClass($testPropertyObj),
+    ];
+    var_dump(unserialize(serialize($array)));
+    class X {}
+});
+
+unserialize('O:1:"X":0:{}');
+?>
+--EXPECT--
+array(2) {
+  ["obj1"]=>
+  object(SerializableClass)#5 (1) {
+    ["sharedProp"]=>
+    object(stdClass)#6 (1) {
+      ["name"]=>
+      string(4) "test"
+    }
+  }
+  ["obj2"]=>
+  object(SerializableClass)#7 (1) {
+    ["sharedProp"]=>
+    object(stdClass)#6 (1) {
+      ["name"]=>
+      string(4) "test"
+    }
+  }
+}

ext/standard/var.c

@@ -716,11 +716,12 @@ static int php_var_serialize_call_sleep(zval *retval, zval *struc) /* {{{ */
 {
 	zval fname;
 	int res;
-
+	struct php_serialize_data *prev_data;
+	unsigned prev_lock_level;
 	ZVAL_STRINGL(&fname, "__sleep", sizeof("__sleep") - 1);
-	BG(serialize_lock)++;
+	PHP_VAR_SERIALIZE_LOCK(prev_data, prev_lock_level);
 	res = call_user_function(NULL, struc, &fname, retval, 0, 0);
-	BG(serialize_lock)--;
+	PHP_VAR_SERIALIZE_UNLOCK(prev_data, prev_lock_level);
 	zval_ptr_dtor_str(&fname);
 
 	if (res == FAILURE || Z_ISUNDEF_P(retval)) {
@@ -742,11 +743,13 @@ static int php_var_serialize_call_magic_serialize(zval *retval, zval *obj) /* {{
 {
 	zval fname;
 	int res;
+	struct php_serialize_data *prev_data;
+	unsigned prev_lock_level;
 
 	ZVAL_STRINGL(&fname, "__serialize", sizeof("__serialize") - 1);
-	BG(serialize_lock)++;
+	PHP_VAR_SERIALIZE_LOCK(prev_data, prev_lock_level);
 	res = call_user_function(CG(function_table), obj, &fname, retval, 0, 0);
-	BG(serialize_lock)--;
+	PHP_VAR_SERIALIZE_UNLOCK(prev_data, prev_lock_level);
 	zval_ptr_dtor_str(&fname);
 
 	if (res == FAILURE || Z_ISUNDEF_P(retval)) {
@@ -1111,29 +1114,28 @@ PHPAPI void php_var_serialize(smart_str *buf, zval *struc, php_serialize_data_t
 
 PHPAPI php_serialize_data_t php_var_serialize_init() {
 	struct php_serialize_data *d;
-	/* fprintf(stderr, "SERIALIZE_INIT      == lock: %u, level: %u\n", BG(serialize_lock), BG(serialize).level); */
-	if (BG(serialize_lock) || !BG(serialize).level) {
+	/* fprintf(stderr, "SERIALIZE_INIT      == lock_level: %u, level: %u\n", BG(serialize).lock_level, BG(serialize).level); */
+	if (BG(serialize).lock_level == BG(serialize).level) {
 		d = emalloc(sizeof(struct php_serialize_data));
 		zend_hash_init(&d->ht, 16, NULL, ZVAL_PTR_DTOR, 0);
 		d->n = 0;
-		if (!BG(serialize_lock)) {
-			BG(serialize).data = d;
-			BG(serialize).level = 1;
-		}
+		BG(serialize).data = d;
 	} else {
 		d = BG(serialize).data;
-		++BG(serialize).level;
+		ZEND_ASSERT(d);
 	}
+	++BG(serialize).level;
 	return d;
 }
 
 PHPAPI void php_var_serialize_destroy(php_serialize_data_t d) {
-	/* fprintf(stderr, "SERIALIZE_DESTROY   == lock: %u, level: %u\n", BG(serialize_lock), BG(serialize).level); */
-	if (BG(serialize_lock) || BG(serialize).level == 1) {
+	/* fprintf(stderr, "SERIALIZE_DESTROY   == lock: %u, level: %u\n", BG(serialize).lock_level, BG(serialize).level); */
+	ZEND_ASSERT(BG(serialize).level > 0);
+	ZEND_ASSERT(BG(serialize).lock_level < BG(serialize).level);
+	--BG(serialize).level;
+	if (BG(serialize).lock_level == BG(serialize).level) {
 		zend_hash_destroy(&d->ht);
 		efree(d);
-	}
-	if (!BG(serialize_lock) && !--BG(serialize).level) {
 		BG(serialize).data = NULL;
 	}
 }