Skip to content

Fix GH-11016: Heap buffer overflow in ZEND_ADD_ARRAY_UNPACK_SPEC_HANDLER #11021

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
Merged
merged 1 commit into from
Apr 6, 2023
Merged

Fix GH-11016: Heap buffer overflow in ZEND_ADD_ARRAY_UNPACK_SPEC_HANDLER #11021

merged 1 commit into from
Apr 6, 2023

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Apr 5, 2023

Not enough space was reserved for the packed resulting array because of some confusion in the meaning of nr of used slots vs nr of elements.

Bug only exists in master.

@nielsdos nielsdos linked an issue Apr 5, 2023 that may be closed by this pull request
Heap buffer overflow in ZEND_ADD_ARRAY_UNPACK_SPEC_HANDLER #11016
Closed
@nielsdos @iluuu1994
Fix phpGH-11016: Heap buffer overflow in ZEND_ADD_ARRAY_UNPACK_SPEC_H…
56f2fef 
…ANDLER

Not enough space was reserved for the packed resulting array because of
some confusion in the meaning of nr of used slots vs nr of elements.

Co-authored-by: Ilija Tovilo <[email protected]>
iluuu1994
iluuu1994 approved these changes Apr 5, 2023
View reviewed changes
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.

LGTM! Thank you @nielsdos!

@nielsdos nielsdos mentioned this pull request Apr 6, 2023
Closed
@dstogov
Copy link
Member

dstogov commented Apr 6, 2023

This won't fix the problem alone (without #11022). The nNumOfElements is going to have invalid value. Please combine there PRs into one.

@nielsdos
Copy link
Member Author

nielsdos commented Apr 6, 2023

I can't combine them into 1 PR because the code that this PR fixes is only in master.
PR #11022 targets 8.1.
Do you want me to change the target branch of #11022 to master too? In that case I can combine them.

@dstogov
Copy link
Member

dstogov commented Apr 6, 2023

Do we really need fixing ZEND_HASH_FILL_PACKED() in 8.1. and 8.2?
The problem came from incorrect usage in master.
I don't have strong opinion.

Anyway, it's not a big problem to commit this separately. Better to commit this after #11022

@iluuu1994
Copy link
Member

The problem came from incorrect usage in master.

We only realized this in this PR. @nielsdos I think it's fine to delay GH-11022 to master given that there is no issues with the current usages in these branches.

@nielsdos
Copy link
Member Author

nielsdos commented Apr 6, 2023

OK, I agree, I'll rebase that other PR on master (but keep it separate for easier review).
If necessary we can always backport it.
Thanks for checking.

@nielsdos nielsdos mentioned this pull request Apr 6, 2023
Merged
@nielsdos nielsdos merged commit ede8adb into php:master Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iluuu1994 iluuu1994 iluuu1994 approved these changes

@dstogov dstogov dstogov approved these changes

Assignees
No one assigned
Labels
Category: Engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Heap buffer overflow in ZEND_ADD_ARRAY_UNPACK_SPEC_HANDLER
3 participants
@nielsdos @dstogov @iluuu1994