ext/session: only return false when could not encode session at all (#21181)

* ext/session: only return false when could not encode session at all

This also fixes bug 71162

ext/session/session.c

@@ -507,24 +507,24 @@ static void php_session_save_current_state(bool write)
 		IF_SESSION_VARS() {
 			zval *handler_function = &PS(mod_user_names).ps_write;
 			if (PS(mod_data) || PS(mod_user_implemented)) {
-				zend_string *val;
-
-				val = php_session_encode();
-				if (val) {
-					if (PS(lazy_write) && PS(session_vars)
-						&& PS(mod)->s_update_timestamp
-						&& PS(mod)->s_update_timestamp != php_session_update_timestamp
-						&& zend_string_equals(val, PS(session_vars))
-					) {
-						ret = PS(mod)->s_update_timestamp(&PS(mod_data), PS(id), val, PS(gc_maxlifetime));
-						handler_function = &PS(mod_user_names).ps_update_timestamp;
-					} else {
-						ret = PS(mod)->s_write(&PS(mod_data), PS(id), val, PS(gc_maxlifetime));
-					}
-					zend_string_release_ex(val, false);
-				} else {
-					ret = PS(mod)->s_write(&PS(mod_data), PS(id), ZSTR_EMPTY_ALLOC(), PS(gc_maxlifetime));
+				zend_string *val = php_session_encode();
+				/* Not being able to encode the session data means there is some kind of issue that prevents a write
+				 * (e.g. a key containing the '|' character with the default serialization) */
+				if (UNEXPECTED(val == NULL)) {
+					return;
 				}
+
+				if (PS(lazy_write) && PS(session_vars)
+					&& PS(mod)->s_update_timestamp
+					&& PS(mod)->s_update_timestamp != php_session_update_timestamp
+					&& zend_string_equals(val, PS(session_vars))
+				) {
+					ret = PS(mod)->s_update_timestamp(&PS(mod_data), PS(id), val, PS(gc_maxlifetime));
+					handler_function = &PS(mod_user_names).ps_update_timestamp;
+				} else {
+					ret = PS(mod)->s_write(&PS(mod_data), PS(id), val, PS(gc_maxlifetime));
+				}
+				zend_string_release_ex(val, false);
 			}
 
 			if ((ret == FAILURE) && !EG(exception)) {
@@ -929,7 +929,7 @@ PS_SERIALIZER_ENCODE_FUNC(php_serialize)
 		php_var_serialize(&buf, Z_REFVAL(PS(http_session_vars)), &var_hash);
 		PHP_VAR_SERIALIZE_DESTROY(var_hash);
 	}
-	return buf.s;
+	return smart_str_extract(&buf);
 }
 
 PS_SERIALIZER_DECODE_FUNC(php_serialize)
@@ -980,11 +980,9 @@ PS_SERIALIZER_ENCODE_FUNC(php_binary)
 			smart_str_append(&buf, key);
 			php_var_serialize(&buf, struc, &var_hash);
 	);
-
-	smart_str_0(&buf);
 	PHP_VAR_SERIALIZE_DESTROY(var_hash);
 
-	return buf.s;
+	return smart_str_extract(&buf);
 }
 
 PS_SERIALIZER_DECODE_FUNC(php_binary)
@@ -1038,7 +1036,6 @@ PS_SERIALIZER_ENCODE_FUNC(php)
 	PS_ENCODE_LOOP(
 		smart_str_append(&buf, key);
 		if (memchr(ZSTR_VAL(key), PS_DELIMITER, ZSTR_LEN(key))) {
-			PHP_VAR_SERIALIZE_DESTROY(var_hash);
 			smart_str_free(&buf);
 			fail = true;
 			php_error_docref(NULL, E_WARNING, "Failed to write session data. Data contains invalid key \"%s\"", ZSTR_VAL(key));
@@ -1047,15 +1044,15 @@ PS_SERIALIZER_ENCODE_FUNC(php)
 		smart_str_appendc(&buf, PS_DELIMITER);
 		php_var_serialize(&buf, struc, &var_hash);
 	);
+	PHP_VAR_SERIALIZE_DESTROY(var_hash);
 
 	if (fail) {
 		return NULL;
 	}
 
-	smart_str_0(&buf);
+	zend_string *encoded = smart_str_extract(&buf);
 
-	PHP_VAR_SERIALIZE_DESTROY(var_hash);
-	return buf.s;
+	return encoded;
 }
 
 PS_SERIALIZER_DECODE_FUNC(php)
@@ -2280,7 +2277,6 @@ PHP_FUNCTION(session_id)
 PHP_FUNCTION(session_regenerate_id)
 {
 	bool del_ses = false;
-	zend_string *data;
 
 	if (zend_parse_parameters(ZEND_NUM_ARGS(), "|b", &del_ses) == FAILURE) {
 		RETURN_THROWS();
@@ -2307,14 +2303,17 @@ PHP_FUNCTION(session_regenerate_id)
 			RETURN_FALSE;
 		}
 	} else {
-		zend_result ret;
-		data = php_session_encode();
-		if (data) {
-			ret = PS(mod)->s_write(&PS(mod_data), PS(id), data, PS(gc_maxlifetime));
-			zend_string_release_ex(data, false);
-		} else {
-			ret = PS(mod)->s_write(&PS(mod_data), PS(id), ZSTR_EMPTY_ALLOC(), PS(gc_maxlifetime));
+		zend_string *old_session_data = php_session_encode();
+		/* If we have no data we must destroy the related session ID */
+		if (UNEXPECTED(old_session_data == NULL)) {
+			PS(mod)->s_close(&PS(mod_data));
+			PS(session_status) = php_session_none;
+			RETURN_FALSE;
 		}
+
+		zend_result ret = PS(mod)->s_write(&PS(mod_data), PS(id), old_session_data, PS(gc_maxlifetime));
+		zend_string_release_ex(old_session_data, false);
+
 		if (ret == FAILURE) {
 			PS(mod)->s_close(&PS(mod_data));
 			PS(session_status) = php_session_none;
@@ -2368,6 +2367,7 @@ PHP_FUNCTION(session_regenerate_id)
 		// TODO warn that ID cannot be verified? else { }
 	}
 	/* Read is required to make new session data at this point. */
+	zend_string *data = NULL;
 	if (PS(mod)->s_read(&PS(mod_data), PS(id), &data, PS(gc_maxlifetime)) == FAILURE) {
 		PS(mod)->s_close(&PS(mod_data));
 		PS(session_status) = php_session_none;

ext/session/tests/session_encode_error2.phpt

@@ -51,28 +51,28 @@ ob_end_flush();
 bool(true)
 
 Warning: session_encode(): Skipping numeric key 0 in %s on line %d
-bool(false)
+string(0) ""
 bool(true)
 
 -- Iteration 2 --
 bool(true)
 
 Warning: session_encode(): Skipping numeric key 1 in %s on line %d
-bool(false)
+string(0) ""
 bool(true)
 
 -- Iteration 3 --
 bool(true)
 
 Warning: session_encode(): Skipping numeric key 12345 in %s on line %d
-bool(false)
+string(0) ""
 bool(true)
 
 -- Iteration 4 --
 bool(true)
 
 Warning: session_encode(): Skipping numeric key -2345 in %s on line %d
-bool(false)
+string(0) ""
 bool(true)
 
 -- Iteration 5 --

ext/session/tests/session_encode_partial_data.phpt

@@ -0,0 +1,30 @@
+--TEST--
+session_encode(): partial session data
+--INI--
+serialize_precision=100
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php include('skipif.inc'); ?>
+--FILE--
+<?php
+
+ob_start();
+
+session_start();
+
+$_SESSION['data1'] = 'hello';
+$_SESSION['data2'] = 'PHP';
+$_SESSION['partial|data'] = 'key with pipe';
+$_SESSION['data3'] = 'world';
+
+var_dump(session_encode());
+
+var_dump(session_destroy());
+
+ob_end_flush();
+?>
+--EXPECTF--
+Warning: session_encode(): Failed to write session data. Data contains invalid key "partial|data" in %s on line %d
+bool(false)
+bool(true)

ext/session/tests/session_encode_variation1.phpt

@@ -30,11 +30,11 @@ ob_end_flush();
 Warning: session_encode(): Cannot encode non-existent session in %s on line %d
 bool(false)
 bool(true)
-bool(false)
+string(0) ""
 bool(true)
-bool(false)
+string(0) ""
 bool(true)
-bool(false)
+string(0) ""
 bool(true)
 
 Warning: session_encode(): Cannot encode non-existent session in %s on line %d

ext/session/tests/session_encode_variation2.phpt

@@ -22,7 +22,7 @@ ob_end_flush();
 ?>
 --EXPECTF--
 *** Testing session_encode() : variation ***
-bool(false)
+string(0) ""
 bool(true)
 
 Warning: session_encode(): Cannot encode non-existent session in %s on line %d

ext/session/tests/session_encode_variation6.phpt

@@ -32,16 +32,16 @@ ob_end_flush();
 bool(true)
 
 Warning: session_encode(): Skipping numeric key 0 in %s on line %d
-bool(false)
+string(0) ""
 bool(true)
 bool(true)
 
 Warning: session_encode(): Skipping numeric key 1234567890 in %s on line %d
-bool(false)
+string(0) ""
 bool(true)
 bool(true)
 
 Warning: session_encode(): Skipping numeric key -1234567890 in %s on line %d
-bool(false)
+string(0) ""
 bool(true)
 Done

ext/session/tests/user_session_module/bug71162.phpt

@@ -4,8 +4,6 @@ updateTimestamp never called when session data is empty
 session
 --INI--
 session.use_strict_mode=0
---XFAIL--
-Current session module is designed to write empty session always.
 --FILE--
 <?php
 class MySessionHandler extends SessionHandler implements SessionUpdateTimestampHandlerInterface
@@ -71,7 +69,7 @@ session_commit();
 --EXPECT--
 string(40) "da39a3ee5e6b4b0d3255bfef95601890afd80709"
 bool(true)
-write
+updateTimestamp
 string(40) "da39a3ee5e6b4b0d3255bfef95601890afd80709"
 bool(true)
 updateTimestamp

ext/session/tests/user_session_module/session_set_save_handler_variation5.phpt

@@ -83,7 +83,7 @@ Read [%s,PHPT-%d]
 GC [0]
 1 deleted
 bool(true)
-Write [%s,PHPT-%d,]
+Update [%s,PHPT-%d]
 Close [%s,PHPSESSID]
 bool(true)
 string(%d) "PHPT-%d"