Bug #72936 [Opn->Csd]: Zend API's zend_symtable_str_update() asserts key should end with '\0'

Edit report at https://bugs.php.net/bug.php?id=72936&edit=1 ID: 72936 Updated by: laruence@php.net Reported by: arseny dot vakhrushev at gmail dot com Summary: Zend API's zend_symtable_str_update() asserts key should end with '\0' -Status: Open +Status: Closed Type: Bug Package: Arrays related Operating System: All PHP Version: 7.0.10 Block user comment: N Private report: N New Comment: Automatic comment on behalf of laruence@gmail.com Revision: http://git.php.net/?p=php-src.git;a=commit;h=986d0f87ec873c88dedbac334fa5896c068ed987 Log: Fixed bug #72936 (Zend API's zend_symtable_str_update() asserts key should end with '\0') Previous Comments: ------------------------------------------------------------------------ [2016-08-25 06:15:39] arseny dot vakhrushev at gmail dot com Description: ------------ Hello everyone, I'm a developer of a PHP extension, namely php-amf3, and I'm having an issue with zend_symtable_str_update() defined in Zend/zend_hash.h. The code seems to be encouraged to use zend_symtable_str_update() instead of zend_hash_str_update() for string keys because otherwise in case of "numeric" string keys like "123", "-10", etc, there is no way to index such arrays in PHP scripts when using zend_hash_str_update(). More specifically, a C code: ... zval arr, val; array_init(&arr); ZVAL_TRUE(&val); zend_hash_str_update(HASH_OF(&arr), "123", 3, &val); // zend_symtable_str_update() needs to be used instead ... builds an array ["123" => true] which is impossible to index as $arr["123"]. Internally, the key is tested whether it's "numeric" (which is the case) and then the array is indexed with an integer value of 123 which yields NULL. Ok, switching to zend_symtable_str_update() instead of zend_hash_str_update() seems like a Sunday picnic in the park. They are compatible, and everything looks neat and tidy. The problem arises when PHP is compiled with assertions (my default development mode). zend_symtable_str_update() is essentially a wrapper around zend_hash_index_update() and zend_hash_str_update() calling either of them depending on the provided key being "numeric" or not: zval *zend_symtable_str_update(HashTable *ht, const char *str, size_t len, zval *pData) { zend_ulong idx; if (ZEND_HANDLE_NUMERIC_STR(str, len, idx)) { return zend_hash_index_update(ht, idx, pData); } else { return zend_hash_str_update(ht, str, len, pData); } } The ZEND_HANDLE_NUMERIC_STR macro is essentially a call to: int _zend_handle_numeric_str_ex(const char *key, size_t length, zend_ulong *idx) { register const char *tmp = key; const char *end = key + length; ZEND_ASSERT(*end == '\0'); ... } The problem lies in the ZEND_ASSERT which incurs the following: - It reads beyond 'length' bytes (in contrast, zend_hash_str_update() does not); - Requests keys to have a trailing zero making the whole point of the 'length' argument useless (please note that it DOES NOT rely on a key to end with '\0' anywhere else in the body); - Makes it impossible to avoid allocating a temporary buffer in the case when a source key buffer does not have a trailing zero. It seems that the whole point of having a ZEND_ASSERT here does not make a lot of sense since there is no similar restriction in the underlying zend_hash_str_update() function. I infer that this assertion has been there for some "historical" reasons from the era of trailing zeroes in strings/keys and should be removed. Of course, one can work around the whole issue by allocating a temporary buffer, copying a key into it, finalizing it with a trailing zero and calling zend_symtable_str_update() with it. But I hope everyone can see that little issues like this lead to more issues in the future essentially masking problems at the expense of the user (additional overhead) instead of solving them. Cheers, Arseny ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=72936&edit=1