Fix object->array/array->object casts wrt numeric string keys #2142

hikari-no-yume · 2016-09-24T00:49:26Z

This fixes the long-standing issue where casting an object to an array renders any numeric string properties inaccessible, and likewise casting an array to an object renders any integer keys inaccessible. This problem also affects get_object_vars().

The issue is fixed by converting the keys in the hashtable as appropriate, using two new functions for these purposes: zend_proptable_to_symtable and zend_symtable_to_proptable, respectively. These functions are specifically designed to avoid duplicating the hashtables in question if possible, i.e. when no keys need converting, which should improve performance.

I have done a simple benchmark. The results show that this change has only a small performance impact for the common case of array to object casts where the array has only string keys, and object to array casts where the object has only non-numeric string keys.

This is currently pointed at master, ~~~but since it is a bug fix, there might be a case for putting it in PHP 7.1.~~~ (That ship has sailed.)

Ocramius · 2016-09-24T00:52:05Z

Zend/tests/object_array_cast.phpt

@@ -14,11 +14,11 @@ var_dump($obj);
 ?>
 --EXPECT--
 object(stdClass)#1 (3) {
-  [0]=>


Possibly a BC break here, although it shouldn't change much for consumers, unless they rely on array_keys()

PHP's weak typing probably helps here. But, yes, there is the possibility someone relies on this.

Minor BC to be documented then?

Yeah, might get a mention in UPGRADING.

Ocramius · 2016-09-24T00:52:37Z

Zend/zend_builtin_functions.c

+						/* We assume here that a mangled property name is never
+						 * numeric. This is probably a safe assumption, but
+						 * theoretically someone might write an extension with
+						 * private, numeric properties. Well, too bad.


Any place where this should be documented, besides in this piece of code?

I mean, in theory, but I don't see why anyone would actually write a class that defines properties like ->{'123'}, it would just be awkward in use. After all, you can't do that in userland (you can set them dynamically, though).

Yes, but since you know this NOW, it's probably better to dump that knowledge somewhere. Sadly, I can't help with deciding where, heh...

However, isn't it still possible to get a mangled numeric property through an array cast (even though it may not actually be declared on the class)?

In theory, yes, but bear in mind you can't just stick a null byte in front of anything here, you'd have to deliberately form a correct mangled property name that zend_unmangle_property_name_ex won't barf at here.

Nonetheless, one less edge case might be better. Who knows, someone might decide to be annoying.

Ocramius · 2016-09-24T00:53:45Z

Zend/zend_hash.c

+
+	return ht;
+
+convert:


I have no clue about the internals conventions here, but shouldn't this be an inlined function?

It could be, it's not that big. But for comparison, zend_array_dup isn't inline either.

Ocramius · 2016-09-24T00:54:00Z

Zend/zend_hash.c

+
+	return ht;
+
+convert:


Same remarks as above

Ocramius · 2016-09-24T00:54:24Z

Zend/zend_hash.c

+
+		ZEND_HASH_FOREACH_KEY_VAL(ht, num_key, str_key, zv) {
+			Z_TRY_ADDREF_P(zv);
+			/* Again, thank ArrayObject for `!str_key ||`. */


A bit cryptic. Maybe an example?

It is a bit cryptic, I should maybe put a more helpful comment. The dilemma it's complaining about is that while strictly speaking an object's hashtable should only contain string keys, and while this pull request would fix up the main cases that cause that not to be the case, ArrayObject is weird and lets you use an arbitrary array as an object hashtable, and such arrays can have integer keys, thus we must check for them.

👍 that is a better explanation already :-)

Should ArrayObject be fixed as well (in this diff or a separate one)?

ArrayObject may not be the only remaining case here, but it was one that cropped up in testing. It could be modified to not use the properties hashtable to store the “array”, sure.

There's now a more helpful comment here.

nikic · 2016-09-26T22:54:47Z

Zend/zend_hash.c

+				zend_string_delref(str_key);
+			}
+			Z_TRY_ADDREF_P(zv);
+			zend_hash_add_new(new_ht, str_key, zv);


Note that this assumes that the symtable is indeed a symtable, otherwise this might end up with duplicate keys. How sure are we that everything apart from object to array conversions is conforming?

Ah, are you saying zend_hash_add_new is an optimised function that doesn't overwrite?

Yes, that's the "new" part of it. I don't know whether we need to be defensive about this here.

Given the issues with actual symbol tables, it looks like we do need to be defensive here.

Yeah. And given the rest of the code is defensive about tables not being as consistent as they ought to be, this sticks out. I'll fix it.

nikic · 2016-09-26T22:56:12Z

Zend/zend_hash.c

+				str_key = zend_long_to_str(num_key);
+				zend_string_delref(str_key);
+			}
+			Z_TRY_ADDREF_P(zv);


Presumably rc=1 refs need to be deref'd here.

Indeed they do:

$ sapi/cli/php -r '$a = 1; $b = [0 => 0, 1 => &$a]; unset($a); $c = (object)$b; $c->{1} = 2; var_dump($b);' array(2) { [0]=> int(0) [1]=> &int(2) }

This has been fixed.

nikic · 2016-09-26T23:02:35Z

Zend/zend_operators.c

-							arr = zend_array_dup(obj_ht);
-							zval_dtor(op);
-							ZVAL_ARR(op, arr);
+							arr = zend_proptable_to_symtable(obj_ht, 1);


Would be nice to combine these two cases, the extra split for one out for conditions is odd.

nikic · 2016-09-30T23:46:04Z

Zend/zend_hash.c

@@ -2523,7 +2523,7 @@ ZEND_API HashTable* ZEND_FASTCALL zend_symtable_to_proptable(HashTable *ht)
 					Z_ADDREF_P(zv);
 				}
 			} while (0);
-			zend_hash_add_new(new_ht, str_key, zv);
+			zend_hash_add(new_ht, str_key, zv);


You probably meant zend_hash_update in all three cases.

What's the difference between zend_hash_add and zend_hash_update?

zend_hash_update overwrites, while zend_hash_add does not. Either is fine generally, but if you use zend_hash_add you also need to perform error handling for the case where the element already exists.

Ah, okay, that makes sense. I'll change it.

hikari-no-yume · 2016-10-10T21:35:25Z

Hey @dstogov, how do you feel about this?

krakjoe · 2016-10-17T14:35:14Z

So that we can have a clear understanding of the backwards compatibility implications here, and a clear consensus on accepting those, I think this one requires an RFC.

Would be happy to see that written up ASAP @TazeTSchnitzel

hikari-no-yume · 2016-10-17T14:37:36Z

Alright then. I'll do that when I get the chance.

cmb69 · 2016-10-18T15:37:45Z

Note that PR #2091 also targets this issue for get_object_vars only.

hikari-no-yume · 2016-10-22T00:45:25Z

RFC: https://wiki.php.net/rfc/convert_numeric_keys_in_object_array_casts

hikari-no-yume · 2016-11-05T17:57:57Z

I've made another branch which incorporates @dstogov's patch for tracking the key types HashTables have, and uses it to attempt to reduce the performance hit for array-to-object conversions: hikari-no-yume/php-src@fixArrayObjectCastsZippy...fixArrayObjectCastsZippyDmitry

I'll update my benchmark with it.

I've not merged it into this pull request's branch, because I'm not sure if it's worth it (it involves adding extra instructions to hash table insertions). I defer to Dmitry's judgement on that. Maybe this new use case for it might make it seem more worthwhile.

hikari-no-yume · 2016-11-05T18:51:31Z

Ah, the speedup is negligible for that patch. Probably not worth it.

hikari-no-yume · 2016-11-14T17:48:02Z

RFC was accepted, so I will probably merge this soon. It'll be as a squashed commit, because the commit history of this is unimportant clutter.

nikic · 2016-11-14T17:53:50Z

Why is Zend/tests/cast_to_object.phpt a binary file?

hikari-no-yume · 2016-11-14T17:55:19Z

@nikic I think it might contain null bytes in the test output.

cmb69 · 2016-11-14T18:10:24Z

I think it might contain null bytes in the test output.

Indeed, it does. Is it important to test the \0 conversion? If so, a respective comment in the PHPT might be appropriate; otherwise we could drop it.

hikari-no-yume · 2016-11-14T18:13:26Z

@cmb69 Yes, we should test it, it's an important edge case.

The problem here is really var_dump, which doesn't escape strings on output, so various special characters aren't visible and can make the file appear to be “binary”.

hikari-no-yume · 2016-11-14T18:22:50Z

Merged as: a0502b8

Ocramius reviewed Sep 24, 2016

View reviewed changes

nikic reviewed Sep 26, 2016

View reviewed changes

nikic reviewed Sep 30, 2016

View reviewed changes

hikari-no-yume mentioned this pull request Oct 17, 2016

Fix object->array and array->object casts for numeric properties #1700

Closed

smalyshev added the RFC label Oct 30, 2016

hikari-no-yume force-pushed the fixArrayObjectCastsZippy branch from 6a73c45 to 4109e55 Compare November 5, 2016 16:24

hikari-no-yume added 13 commits November 5, 2016 17:08

Fix object->array and array->object casts wrt numeric string keys

89ba23f

Die Hash-Tabelle ist nicht zu zerstören

7d27d1a

Vereinfachen

58138e2

Fix get_object_vars() too

a32ac84

oop

529d721

ArrayObject is ａｂｏｍｉｎａｔｉｏｎ

2f6bd61

Update tests

d47b8a3

Merge redundant if statement

a38dc56

Strip refs with refcount=1 when duplicating

ad9bf82

More helpful comment

7b679f3

Be defensive and overwrite duplicate keys

af2fce9

Overwrite, rather than ignoring, duplicate keys

43bec18

Use HT_IS_PACKED() for readability

8f67845

hikari-no-yume force-pushed the fixArrayObjectCastsZippy branch from 4109e55 to 8f67845 Compare November 5, 2016 17:17

NEWS and UPGRADING

93329b0

Fix typo

e3a9a4d

hikari-no-yume closed this Nov 14, 2016

php-pulls mentioned this pull request Dec 10, 2016

fix bug #72254 - get_object_vars with numerical properties #2091

Closed

Fix object->array/array->object casts wrt numeric string keys #2142

Fix object->array/array->object casts wrt numeric string keys #2142

Conversation

hikari-no-yume commented Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hikari-no-yume Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hikari-no-yume Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hikari-no-yume commented Oct 10, 2016

krakjoe commented Oct 17, 2016

hikari-no-yume commented Oct 17, 2016

cmb69 commented Oct 18, 2016

hikari-no-yume commented Oct 22, 2016

hikari-no-yume commented Nov 5, 2016 • edited Loading

hikari-no-yume commented Nov 5, 2016

hikari-no-yume commented Nov 14, 2016

nikic commented Nov 14, 2016

hikari-no-yume commented Nov 14, 2016

cmb69 commented Nov 14, 2016

hikari-no-yume commented Nov 14, 2016 • edited Loading

hikari-no-yume commented Nov 14, 2016

hikari-no-yume commented Sep 24, 2016 •

edited

Loading

hikari-no-yume Sep 26, 2016 •

edited

Loading

hikari-no-yume Sep 26, 2016 •

edited

Loading

hikari-no-yume commented Nov 5, 2016 •

edited

Loading

hikari-no-yume commented Nov 14, 2016 •

edited

Loading