-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Avoid crash for reset/end/next/prev() on ffi classes #9716
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
Conversation
Co-authored-by: Christoph M. Becker <cmbecker69@gmx.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what's the motivation/context for overriding get_properties - if it's just reducing memory for typical workloads, it should be safe to override get_properties_for (which would keep memory low after var_export/debug_zval_dump/var_dump/json_encode)
Is there anything about FFI specifically that is a reason to avoid populating obj->properties
@@ -188,6 +188,10 @@ typedef struct _zend_ffi_ctype { | |||
zend_ffi_type *type; | |||
} zend_ffi_ctype; | |||
|
|||
/* This is a "mutable" copy of zend_empty_array that prevents asseerts in attempts of iteraton | |||
* (see https://github.com/php/php-src/issues/9697) */ | |||
static HashTable _empty_array; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about ZTS builds? Technically, having multiple parallel threads calling reset() would be writing to this static array simultaneously.
That'd probably be writing the exact same value concurrently if multiple threads concurrently called reset(), which would be safe on most architectures, but may be unsafe in obscure architectures.
A bigger issue would be whether bugs in opcache, PECLs, the engine, or internal functions could actually write properties to this empty array - those would affect not only the given request, but all subsequent requests.
Doing something like ext/session and putting this in the request globals in rinit would limit the effect of bugs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. You are right. Lets commit your solution.
Also, what's the motivation/context for overriding get_properties - if it's just reducing memory for typical workloads, it should be safe to override get_properties_for (which would keep memory low after var_export/debug_zval_dump/var_dump/json_encode)
Is there anything about FFI specifically that is a reason to avoid populating obj->properties
I think, FFI was developed before get_properties_for
was added.
obj->properties for FFI objects just don't make sense.
If you like, you may refactor ext/ffi to use `get_properties_for``
https://github.com/TysonAndre/pecl-teds/blob/1.2.6/teds_emptycollection.c#L422 E.g. for EmptySequence/EmptySet/EmptyMap in the |
Closes GH-9697