Skip to content

Fix GH-9698: stream_wrapper_register crashes with FFI\CData #12926

Closed
bukka wants to merge 1 commit intophp:PHP-8.2from
bukka:stream_user_wrapper_ffi_cdata_crash
Closed

Fix GH-9698: stream_wrapper_register crashes with FFI\CData #12926
bukka wants to merge 1 commit intophp:PHP-8.2from
bukka:stream_user_wrapper_ffi_cdata_crash

Conversation

@bukka
Copy link
Member

@bukka bukka commented Dec 10, 2023

The context is provided before constructor is called so there needs to be a bit more checking in FFI and stream user wrapper.

@bukka bukka requested a review from dstogov as a code owner December 10, 2023 22:31
@bukka bukka changed the base branch from master to PHP-8.2 December 10, 2023 22:31
@bukka bukka force-pushed the stream_user_wrapper_ffi_cdata_crash branch from 3d783e8 to 8d8bef3 Compare December 10, 2023 22:32
dstogov
dstogov approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

The fix is right, but FFI\CData is not an unique class that disables instantiation through a fake constructor. It seems like all of them may be used in unexpected ways. (e.g. you fix only property update, but some code may try to read property first).

It would be great to find a common solution to disable instantiation of some classes (this is probably for master branch only).

@mvorisek mvorisek mentioned this pull request Dec 11, 2023
Closed
@bukka
Copy link
Member Author

bukka commented Dec 15, 2023

So stream wrapper actually calls constructor but after adding the context property so we would probably need some sort of checks in functions that call write_property (add_property_zval_ex), read_property and so on. It would probably help but extension could still call the handler directly so the question is whether it is not better to be addressed in each class - it means in this case FFI\CData should always check if it is initialized when writing, reading or unsetting props.

@bukka
Copy link
Member Author

bukka commented Dec 15, 2023

Anyway I will merge this as it fixes the actual problem and it is applicable as a bug fix.

@bukka bukka closed this in 40ccc8e Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dstogov dstogov dstogov approved these changes

Assignees

No one assigned

Labels

Category: Streams Extension: ffi

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bukka @dstogov