Skip to content

Fix #73753 - Unpacked Arrays and Duplication #2248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
bp1222 wants to merge 2 commits into from
Closed

Fix #73753 - Unpacked Arrays and Duplication #2248

bp1222 wants to merge 2 commits into from

Conversation

@bp1222
Copy link
Contributor

@bp1222 bp1222 commented Dec 20, 2016

Bug was exposed in trying to call next() on an array that is not
packed. What ends up happening is when separating a call to
zend_array_dup() is called and falls into the else case for type of
array. This sets the nInternalPointer to invalid-idx, with the
expectation of being set during the zend_array_dup_elements() call.
However, there, we see that if we success duplicating the element,
we never check to see if the sources pointer is the current index.

This fix checks to see if we can dup the element, and if so, check
to see if the idx is the sources internal pointer, to set it.

@bp1222
Fix #73753 - Unpacked Arrays and Duplication
ed66424 
Bug was exposed in trying to call next() on an array that is not
packed.  What ends up happening is when separating a call to
zend_array_dup() is called and falls into the else case for type of
array.  This sets the nInternalPointer to invalid-idx, with the
expectation of being set during the zend_array_dup_elements() call.
However, there, we see that if we success duplicating the element,
we never check to see if the sources pointer is the current index.

This fix checks to see if we can dup the element, and if so, check
to see if the idx is the sources internal pointer, to set it.
@nikic
Copy link
Member

nikic commented Dec 21, 2016

I think this can also be solved (more efficiently) by changing the target->nInternalPointer = HT_INVALID_IDX initialization to target->nInternalPointer = source->nInternalPointer, as is done in the packed case. If the IAP is after the first hole, then the already existing code in dup_elements should take care of moving it back to the appropriate position.

@bp1222
Copy link
Contributor Author

bp1222 commented Dec 21, 2016 •
edited
Loading

I was first thinking that could be a solution, however the check at the end of the else case, if (idx > 0 && target->nInternalPointer == HT_INVALID_IDX) { wouldn't make much sense if you did that, since the zend_array_dupe_elements() wouldn't ever set the nInternalPointer to the HT_INVALID_IDX.

So I could do the set, and just drop that portion setting to 0 if HT_INVALID_IDX

@nikic
Copy link
Member

nikic commented Dec 21, 2016

Not sure I understand. source->nInternalPointer may be HT_INVALID_IDX, in which case target->nInternPointer (after copying) would be as well. This clause resets it to zero.

@bp1222
Copy link
Contributor Author

bp1222 commented Dec 21, 2016

The way I was reading it would be that target->nInternalPointer would not again be set to HT_INVALID_INDEX through zend_array_dup_elements and zend_array_dupe_element, so the end case there in the else if (idx > 0 && target->nInternalIndex == HT_INVALID_IDX) would always fail. Unless i'm missing where the index could be set to invalid within that execution. I'll update with your comment

@nikic
Copy link
Member

nikic commented Dec 21, 2016

Merged via 5733fd1, thanks!

@nikic nikic closed this Dec 21, 2016
@bp1222 bp1222 deleted the fix-73753 branch December 28, 2016 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bp1222 @nikic