Skip to content

Fix #77177: Serializing or unserializing COM objects crashes #3672

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.

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

Closed
wants to merge 1 commit into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Nov 20, 2018

Firstly, we avoid returning NULL from the get_property handler, but
instead return an empty HashTable, which already prevents the crashes.
Secondly, since (de-)serialization obviously makes no sense for COM,
DOTNET and VARIANT objects (at least with the current implementation),
we prohibit it right away.

Firstly, we avoid returning NULL from the get_property handler, but
instead return an empty HashTable, which already prevents the crashes.
Secondly, since (de-)serialization obviously makes no sense for COM,
DOTNET and VARIANT objects (at least with the current implementation),
we prohibit it right away.
@cmb69 cmb69 added the Bug label Nov 20, 2018
@cmb69
Copy link
Member Author

cmb69 commented Nov 20, 2018

Note to eventual merger: as of PHP-7.3 zend_empty_array can be used instead of the ad-hoc com_dotnet_object_properties and it's implementation.

@KalleZ
Copy link
Member

KalleZ commented Nov 21, 2018

I think the patch looks good.

Side note; ext/com_dotnet has barely been updated with many changes to newer internal APIs, sometimes I worry that it only just compiles. It wasn't until a few years ago that Reflection also kept crashing on COM instances[1]. All in all, I think it perhaps is worth taking a look at the extension as a whole, as I doubt this is the only similar issue.

[1] https://bugs.php.net/bug.php?id=45280

@weltling
Copy link
Contributor

@CMB i've tested and everything seems ok. Please merge it when have time. Not sure we can get it into 7.1, perhaps at RMs convenience.

Thanks.

@cmb69
Copy link
Member Author

cmb69 commented Nov 21, 2018

All in all, I think it perhaps is worth taking a look at the extension as a whole, as I doubt this is the only similar issue.

Well, com_dotnet is listed as maintained, so everything should be fine, ;)

@weltling My Github nick is @cmb69 – guess Chris Brannon is not too happy to get unrelated mentions. :)

Not sure we can get it into 7.1, perhaps at RMs convenience.

Since php-7.1.25RC1 has been tagged in the meantime, I think we should only target PHP-7.2+. @sgolemon?

@cmb69
Copy link
Member Author

cmb69 commented Nov 23, 2018

Applied to PHP-7.2 and merged up.

@cmb69 cmb69 closed this Nov 23, 2018
@cmb69 cmb69 deleted the com-serialize branch November 23, 2018 15:39
php-pulls pushed a commit that referenced this pull request Nov 26, 2018
No need to define our own `com_dotnet_object_properties` as empty hash,
since we can use `zend_empty_array` instead as of PHP 7.3.0.

Cf. <#3672 (comment)>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants