Skip to content

ext/spl: Use zend_hash_index_add_new() for missing array keys#21535

Open
arshidkv12 wants to merge 3 commits intophp:masterfrom
arshidkv12:spl-2
Open

ext/spl: Use zend_hash_index_add_new() for missing array keys#21535
arshidkv12 wants to merge 3 commits intophp:masterfrom
arshidkv12:spl-2

Conversation

@arshidkv12
Copy link
Contributor

Previously, zend_hash_index_update() was used, which handles both insertion and overwriting.
Since we already check that the key does not exist, zend_hash_index_add_new() is more appropriate.

  • Avoids unnecessary overwrite logic

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are going to provide such refactorings it would be nice that you found all applicable cases and submit them in one PR.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this reproducer, which will segfault.

set_error_handler(function () {
    global $ao;
    $ao[0]++;
});

$ao = new ArrayObject();
$ao[0]++;

var_dump($ao);

retval = &EG(uninitialized_zval);
break;
case BP_VAR_RW:
zend_error(E_WARNING, "Undefined array key " ZEND_LONG_FMT, key.h);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, not sound because of this warning...

retval = &EG(uninitialized_zval);
break;
case BP_VAR_RW:
zend_error(E_WARNING,"Undefined array key \"%s\"", ZSTR_VAL(key.key));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants