Re: [PHP-DEV] [PATCH] default session serialization

[Arpad Ray]

On Thu, Nov 26, 2009 at 2:23 PM, Antony Dovgal <t...@daylessday.org> wrote:

> On 26.11.2009 03:43, Stanislav Malyshev wrote:
> > I think it makes sense. One note: your code allows numeric session keys,
> > previously not allowed. Not sure if it's important.
>
> This might be important for 32bit<->64bit interaction using serialized
> data.
>
>
Although the patch allows numeric keys to be encoded, they're silently
dropped in decoding. Previously they were silently dropped during encoding.
I prefer this way since it saves the overhead of filtering the array first.

If there's an inherent danger in unserializing between 32/64bit int array
keys, this risk is already present in the form of nested arrays (not to say
it's an unimportant consideration). My testing with out of range values on
32bit and 64bit saw nothing unusual, although it revealed a misplaced
zval_add_ref which the attached updated patch fixes.

Incidentally, is there any need (in HEAD) to check EG(symbol_table) in the
decode function, now that register_globals, session_register et al are gone?

Regards,

Arpad

Index: ext/session/session.c
===================================================================
--- ext/session/session.c	(revision 291322)
+++ ext/session/session.c	(working copy)
@@ -783,45 +783,10 @@
 {
 	smart_str buf = {0};
 	php_serialize_data_t var_hash;
-	PS_ENCODE_VARS;
 
 	PHP_VAR_SERIALIZE_INIT(var_hash);
+	php_var_serialize(&buf, &PS(http_session_vars), &var_hash TSRMLS_CC);
 
-	PS_UENCODE_LOOP(
-		if (!struc) {
-			smart_str_appendc(&buf, PS_UNDEF_MARKER);
-		}
-
-		if (key_type == HASH_KEY_IS_STRING) {
-			if (memchr(key.s, PS_DELIMITER, key_length)) {
-				PHP_VAR_SERIALIZE_DESTROY(var_hash);
-				smart_str_free(&buf);
-				return FAILURE;
-			}
-			smart_str_appendl(&buf, key.s, key_length);
-		} else {
-			/* HASH_KEY_IS_UNICODE */
-			char *str = NULL;
-			int len;
-			UErrorCode status = U_ZERO_ERROR;
-
-			zend_unicode_to_string_ex(UG(utf8_conv), &str, &len, key.u, key_length, &status);
-			if (U_FAILURE(status) || memchr(str, PS_DELIMITER, key_length)) {
-				PHP_VAR_SERIALIZE_DESTROY(var_hash);
-				smart_str_free(&buf);
-				if (str) { efree(str); }
-				return FAILURE;
-			}
-			smart_str_appendl(&buf, str, len);
-			efree(str);
-		}
-		smart_str_appendc(&buf, PS_DELIMITER);
-
-		if (struc) {
-			php_var_serialize(&buf, struc, &var_hash TSRMLS_CC);
-		}
-	);
-
 	if (newlen) {
 		*newlen = buf.len;
 	}
@@ -835,61 +800,57 @@
 
 PS_SERIALIZER_DECODE_FUNC(php) /* {{{ */
 {
-	const char *p, *q;
-	char *name;
 	const char *endptr = val + vallen;
-	zval *current;
-	int namelen;
-	int has_value;
+	zval **current, *storage;
+	zstr name;
+	uint namelen;
+	zend_uchar utype;
+	ulong num_key;
 	php_unserialize_data_t var_hash;
+	HashPosition pos;
 
+	if (!vallen) {
+		return SUCCESS;
+	}
+
 	PHP_VAR_UNSERIALIZE_INIT(var_hash);
+	ALLOC_INIT_ZVAL(storage);
 
-	p = val;
+	if (!php_var_unserialize(&storage, (const unsigned char **)&val, (const unsigned char *)endptr, &var_hash TSRMLS_CC) || Z_TYPE_P(storage) != IS_ARRAY) {
+		zval_ptr_dtor(&storage);
+		PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
+		return FAILURE;
+	}
 
-	while (p < endptr) {
+	zend_hash_internal_pointer_reset_ex(Z_ARRVAL_P(storage), &pos);
+	while (zend_hash_get_current_data_ex(Z_ARRVAL_P(storage), (void **)&current, &pos) == SUCCESS) {
 		zval **tmp;
-		has_value = 1;
 
-		q = p;
-		while (*q != PS_DELIMITER) {
-			if (++q >= endptr) goto break_outer_loop;
+		switch (zend_hash_get_current_key_ex(Z_ARRVAL_P(storage), &name, &namelen, &num_key, 0, &pos)) {
+			case HASH_KEY_IS_STRING:
+				utype = IS_STRING;
+				break;
+			case HASH_KEY_IS_UNICODE:
+				utype = IS_UNICODE;
+				break;
+			default:
+				goto skip;
 		}
 
-		if (*p == PS_UNDEF_MARKER) {
-			if (++p >= endptr) goto break_outer_loop;
-
-			has_value = 0;
-		}
-
-		namelen = q - p;
-		name = estrndup(p, namelen);
-		q++;
-
-		if (zend_hash_find(&EG(symbol_table), name, namelen + 1, (void **) &tmp) == SUCCESS) {
+		if (zend_u_hash_find(&EG(symbol_table), utype, name, namelen, (void **)&tmp) == SUCCESS) {
 			if ((Z_TYPE_PP(tmp) == IS_ARRAY && Z_ARRVAL_PP(tmp) == &EG(symbol_table)) || *tmp == PS(http_session_vars)) {
 				goto skip;
 			}
 		}
 
-		if (has_value) {
-			ALLOC_INIT_ZVAL(current);
-			if (php_var_unserialize(&current, (const unsigned char **) &q, (const unsigned char *) endptr, &var_hash TSRMLS_CC)) {
-				zend_utf8_hash_update(Z_ARRVAL_P(PS(http_session_vars)), name, namelen + 1, &current, sizeof(zval *), NULL);
-			} else {
-				zval_ptr_dtor(&current);
-			}
-		}
-		PS_ADD_VARL(name, namelen);
+		zval_add_ref(current);
+		zend_u_hash_update(Z_ARRVAL_P(PS(http_session_vars)), utype, name, namelen, current, sizeof(current), NULL);
 skip:
-		efree(name);
-
-		p = q;
+		zend_hash_move_forward_ex(Z_ARRVAL_P(storage), &pos);
 	}
-break_outer_loop:
 
+	zval_ptr_dtor(&storage);
 	PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
-
 	return SUCCESS;
 }
 /* }}} */

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php