Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

dstogov
Copy link
Member

@dstogov dstogov commented Oct 11, 2022

Closes GH-9697

Co-authored-by: Christoph M. Becker <cmbecker69@gmx.de>
Copy link
Contributor

@TysonAndre TysonAndre left a 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;
Copy link
Contributor

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

Copy link
Member Author

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``

@TysonAndre
Copy link
Contributor

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)

https://github.com/TysonAndre/pecl-teds/blob/1.2.6/teds_emptycollection.c#L422
https://github.com/TysonAndre/pecl-teds/blob/1.2.6/teds_util.c#L3-L8

E.g. for EmptySequence/EmptySet/EmptyMap in the teds pecl - https://github.com/TysonAndre/pecl-teds/tree/1.2.6#overview , I overrid get_properties_for to return null, and didn't override get_properties at all, since typical code paths in php 8.3 wouldn't really use get_properties that often with the various handler overrides, outside of rarely used or deprecated functions. Tests of expected behavior for empty collections pass

@dstogov dstogov closed this Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants