Skip to content

streams: Fix php_stream_from_{res|zval}_no_verify() APIs actually throwing TypeErrors #18079

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.

Sign up for GitHub

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
Closed

streams: Fix php_stream_from_{res|zval}_no_verify() APIs actually throwing TypeErrors #18079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bc4aa4
streams: Fix php_stream_from_{res|zval}_no_verify() APIs actually thr…
Girgias Mar 15, 2025
e992ebb
Fix now manual stream checks
Girgias Mar 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions ext/libxml/libxml.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,11 +771,17 @@ static xmlParserInputPtr php_libxml_external_entity_loader(const char *URL,
} else if (Z_TYPE(retval) == IS_RESOURCE) {
php_stream *stream;
php_stream_from_zval_no_verify(stream, &retval);
if (stream == NULL) {
if (UNEXPECTED(stream == NULL)) {
Copy link
Member

Choose a reason for hiding this comment

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

This code fragment just looks like a completely different fix (for stable branches as well)?

zval callable;
zend_get_callable_zval_from_fcc(&LIBXML(entity_loader_callback), &callable);

zend_string *callable_name = zend_get_callable_name(&callable);
php_libxml_ctx_error(context,
"The user entity loader callback '%s' has returned a "
"The user entity loader callback \"%s\" has returned a "
"resource, but it is not a stream",
ZSTR_VAL(LIBXML(entity_loader_callback).function_handler->common.function_name));
ZSTR_VAL(callable_name));
zend_string_release(callable_name);
zval_dtor(&callable);
} else {
/* TODO: allow storing the encoding in the stream context? */
xmlCharEncoding enc = XML_CHAR_ENCODING_NONE;
Expand Down
7 changes: 5 additions & 2 deletions ext/libxml/tests/libxml_get_external_entity_loader_error_callback_name.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,8 @@ try {
}

?>
--EXPECT--
string(73) "DOMDocument::validate(): supplied resource is not a valid stream resource"
--EXPECTF--
Warning: DOMDocument::validate(): The user entity loader callback "Handler::handle" has returned a resource, but it is not a streamFailed to load external entity "-//FOO/BAR" in %s on line %d

Warning: DOMDocument::validate(): Could not load the external subset "http://example.com/foobar" in %s on line %d
bool(false)
3 changes: 2 additions & 1 deletion ext/posix/posix.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,8 @@ static zend_result php_posix_stream_get_fd(zval *zfp, zend_long *ret) /* {{{ */

php_stream_from_zval_no_verify(stream, zfp);

if (stream == NULL) {
if (UNEXPECTED(stream == NULL)) {
zend_argument_type_error(1, "must be an open stream resource");
return FAILURE;
}

Expand Down
2 changes: 1 addition & 1 deletion ext/posix/tests/posix_ttyname_error_wrongparams.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,4 @@ try {
?>
--EXPECT--
bool(false)
posix_ttyname(): supplied resource is not a valid stream resource
posix_ttyname(): Argument #1 ($file_descriptor) must be an open stream resource
4 changes: 2 additions & 2 deletions main/php_streams.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,8 @@ END_EXTERN_C()
return; \
} \
} while (0)
#define php_stream_from_res_no_verify(xstr, pzval) (xstr) = (php_stream*)zend_fetch_resource2((res), "stream", php_file_le_stream(), php_file_le_pstream())
#define php_stream_from_zval_no_verify(xstr, pzval) (xstr) = (php_stream*)zend_fetch_resource2_ex((pzval), "stream", php_file_le_stream(), php_file_le_pstream())
#define php_stream_from_res_no_verify(xstr, pzval) (xstr) = (php_stream*)zend_fetch_resource2((res), NULL, php_file_le_stream(), php_file_le_pstream())
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the motivation, the no_verify just means that the macro won't return, so it's just an unclear name.
Also this behaviour change does not seem worth it for extensions?

Copy link
Member Author

Choose a reason for hiding this comment

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

The motivation is to have an API that extension can rely on to get a php_stream (mainly from a zval) to ease the migration from resource to objects. And we have a bunch of case which use zend_fetch_resource2 with NULL so they can handle the errors themselves.

I guess I'll just add a new macro/inline function

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not doing those incremental changes. Once there is such migration, users will need to handle it anyway.

#define php_stream_from_zval_no_verify(xstr, pzval) (xstr) = (php_stream*)zend_fetch_resource2_ex((pzval), NULL, php_file_le_stream(), php_file_le_pstream())

BEGIN_EXTERN_C()

Expand Down
Loading