stream_is_local(): unwrap zlib and bzip2 wrappers if needed - #23714
DanielEScherzer wants to merge 4 commits into
Conversation
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
As Nora said, this is a hack. It needs to get info from the wrapper and not doing checks by name.
7087c15 to
4b31d57
Compare
Yeah, I was trying to avoid doing large cross-cutting changes so close to the 8.6 branch, but that makes sense - done |
github-actions
Bot
added
Extension: openssl
Extension: phar
Extension: zip
ABI break
labels
iliaal
left a comment
iliaal
left a comment
There was a problem hiding this comment.
Two crashes reachable from the new callbacks, details inline.
4b31d57 to
9bd21a1
Compare
Add tests to demonstrate that the `zlib:`, `compress.zlib://`, and `compress.bzip2://` wrappers also result in `stream_is_local()` returning `true`. This behavior will be changed in a subsequent commit.
9bd21a1 to
ed42c52
Compare
Switch `php_stream_wrapper.is_url` from a binary flag (stored as an `int`) to an instance of the new enum `php_stream_wrapper_is_url`, which has the cases `STREAM_IS_URL_NEVER`, `STREAM_IS_URL_ALWAYS`, and `STREAM_IS_URL_SOMETIMES`. Add a new optional stream wrapper operation, `php_stream_wrapper_ops.stream_is_url`, that checks if the given path should be considered a URL or not. This callback is required when a stream wrapper is marked as `STREAM_IS_URL_SOMETIMES`. Update existing stream wrappers: - most of those are marked as not being URLs (`is_url` was 0) just updated that field to be `STREAM_IS_URL_NEVER` and added a new NULL field to their operations. - all of those marked as being URLs (`is_url` was 1) just updated that field to be `STREAM_IS_URL_ALWAYS` and added a new NULL field to their operations. - for the zlib and bz2 wrappers, which were previously marked as never being URLs (`is_url` was 0), set the field to `STREAM_IS_URL_SOMETIMES` and implement the new operation to strip the `compress.zlib://` and `compress.bzip2://` prefixes respectively and then check the underlying wrapped stream.
ed42c52 to
d5bbc29
Compare
| struct _php_stream_wrapper { | ||
| const php_stream_wrapper_ops *wops; /* operations the wrapper can perform */ | ||
| void *abstract; /* context for the wrapper */ | ||
| int is_url; /* so that PG(allow_url_fopen) can be respected */ | ||
| php_stream_wrapper_is_url is_url; /* so that PG(allow_url_fopen) can be respected */ |
There was a problem hiding this comment.
I'd rather we remove this field all together and just have the wrapper ops that indicates if the stream is a URI.
Could be that if the wrapper op is NULL then assume it is an URI to be on the safe side.
This would also reduce the size from 24 to 16 due to the alignment requirements, effectively trading 8 bytes in this struct for 8 bytes in the wrapper ops struct with the new function pointer.
There was a problem hiding this comment.
I would rather change it to flags and make it more generic so we can include more info there. I had some use cases before so if we are breaking ABI, it would be better to do it in this way.
| struct _php_stream_wrapper { | ||
| const php_stream_wrapper_ops *wops; /* operations the wrapper can perform */ | ||
| void *abstract; /* context for the wrapper */ | ||
| int is_url; /* so that PG(allow_url_fopen) can be respected */ | ||
| php_stream_wrapper_is_url is_url; /* so that PG(allow_url_fopen) can be respected */ |
There was a problem hiding this comment.
I would rather change it to flags and make it more generic so we can include more info there. I had some use cases before so if we are breaking ABI, it would be better to do it in this way.
| @@ -155,12 +155,20 @@ typedef struct _php_stream_wrapper_ops { | |||
| int (*stream_rmdir)(php_stream_wrapper *wrapper, const char *url, int options, php_stream_context *context); | |||
| /* Metadata handling */ | |||
| int (*stream_metadata)(php_stream_wrapper *wrapper, const char *url, int options, void *value, php_stream_context *context); | |||
| /* Required if the wrapper is_url is STREAM_IS_URL_SOMETIMES, otherwise ignored */ | |||
| bool (*stream_is_url)(php_stream_wrapper *wrapper, const char *url, php_stream_context *context); | |||
There was a problem hiding this comment.
I'm not sure we should have ops just for that. Again I would prefer something more generic. I was considering to put it to metadata but it doesn't fit but need to think about other use cases as there were some IIRC
| } php_stream_wrapper_ops; | ||
|
|
||
| C23_ENUM(php_stream_wrapper_is_url, uint8_t) { |
5 participants
When a stream is wrapped with
compress.zlib://, orcompress.bzip2://, consult the appropriate stream wrapper operations to determine if a stream is local or not.