Fix property access of PHP objects wrapped in variant#16331
Closed
cmb69 wants to merge 1 commit into
Closed
Conversation
First, we fix the long standing issue that property access throws a
`com_exception` ("0x80020003: member not found), because the `HRESULT`
was not properly set after accessing the property.
Next, we fix an issue introduced as of PHP 7.0.0, where the string
length for write access had been properly adapted, but the string
length for read access had been overlooked.
Then we fix an issue introduced as of PHP 8.0.0, where new `HashTable`s
no longer set `nNextFreeElement` to zero, but to `ZEND_LONG_MIN`. This
doesn't work well with the `DISPID` lookup, which is a `LONG`.
Finally we fix a potential double-free due to erroneously destroying
the return value of `zend_read_property()`.
arnaud-lb
reviewed
Oct 10, 2024
Member
arnaud-lb
left a comment
There was a problem hiding this comment.
I haven't tested, but the changes look ok to me apart from the TODO
Comment on lines
-308
to
+310
| zval_ptr_dtor(retval); | ||
| // zval_ptr_dtor(retval); // TODO needed for function calls? |
Member
There was a problem hiding this comment.
Yes I believe this is needed for function calls
Member
Author
There was a problem hiding this comment.
See
Note that this doesn't fix calling methods, which is broken since 2005, because DISPATCH_METHOD|DISPATCH_PROPERTYGET is passed as flag, and the latter is always prioritized in disp_invokeex() (what is likely good for regular COM objects).
I'm planning to fix this in a follow-up PR. Thus I left the comment to not forget about destroying.
Member
There was a problem hiding this comment.
Oh, I overlooked this. This looks ok to me then
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First, we fix the long standing issue that property access throws a
com_exception("0x80020003: member not found), because theHRESULTwas not properly set after accessing the property.Next, we fix an issue introduced as of PHP 7.0.0, where the string length for write access had been properly adapted, but the string length for read access had been overlooked.
Then we fix an issue introduced as of PHP 8.0.0, where new
HashTables no longer setnNextFreeElementto zero, but toZEND_LONG_MIN. This doesn't work well with theDISPIDlookup, which is aLONG.Finally we fix a potential double-free due to erroneously destroying the return value of
zend_read_property().Given that this is completely broken as of PHP 8.0.0 at least, and apparently nobody has complained, I think we're good targeting master (we can backport later if necessary).
Note that this doesn't fix calling methods, which is broken since 2005, because
DISPATCH_METHOD|DISPATCH_PROPERTYGETis passed as flag, and the latter is always prioritized indisp_invokeex()(what is likely good for regular COM objects).