Fix: Checking if a stream is castable should not emit warnings for user defined streams #10435

Girgias · 2023-01-24T17:01:45Z

When calling php_stream_can_cast() the expectation is that errors about a stream not being castable are suppressed, but when a userland stream wrapper does not define the stream_cast() method it always warns.

This is a "similar" problem to #9583/#9638 in that for user defined implementation the function pointer is always defined even if the user implementation does not actually support the operation.

To solve this without unwrapping the stream wrapper and check if the method is defined, we overload the cast_as parameter by turning it into a bitmask which accepts the internal PHP_STREAM_FLAG_SUPPRESS_ERRORS flag. (I initially went for REPORT_ERRORS but the bits overlap between this constant and the CAST_AS constants)

However, we only do this if the stream_wrapper is a userland wrapper (as we control the implementation) as otherwise this is an API break for stream wrappers defined by extensions.

arnaud-lb

I'm not a fan of adding a special case for user-space streams.

php_stream_temp_cast can emit errors when php_stream_fopen_tmpfile fails. I think that php_openssl_sockop_cast may emit errors too, when calling php_stream_fill_read_buffer, but I'm not sure that can be suppressed.

arnaud-lb · 2023-01-27T18:11:33Z

main/streams/cast.c

+	return (strlen(stream->ops->label) == strlen("user-space"))
+		&& strncmp(stream->ops->label, "user-space", strlen("user-space")) == 0;


An other way to do that would be to compare stream->ops to user_stream_wops. We do something like this in

php-src/main/streams/xp_socket.c

Line 655 in ebf26af

if (stream->ops == &php_stream_unix_socket_ops || stream->ops == &php_stream_unixdg_socket_ops) {

arnaud-lb · 2023-01-27T18:25:05Z

main/streams/userspace.c

@@ -1382,6 +1384,8 @@ static int php_userstreamop_cast(php_stream *stream, int castas, void **retptr)
 	php_stream * intstream = NULL;
 	int call_result;
 	int ret = FAILURE;
+	bool report_errors = !(castas & PHP_STREAM_FLAG_SUPPRESS_ERRORS);
+	castas &= ~PHP_STREAM_FLAG_SUPPRESS_ERRORS;


We could use PHP_STREAM_CAST_MASK here like in

php-src/main/streams/cast.c

Lines 193 to 194 in ebf26af

int flags = castas & PHP_STREAM_CAST_MASK;

castas &= ~PHP_STREAM_CAST_MASK;

Okay this doesn't work as PHP_STREAM_CAST_MASK does not contain the PHP_STREAM_FLAG_SUPPRESS_ERRORS flag.

#define PHP_STREAM_CAST_MASK (PHP_STREAM_CAST_TRY_HARD | PHP_STREAM_CAST_RELEASE | PHP_STREAM_CAST_INTERNAL)

I could reuse the PHP_STREAM_CAST_INTERNAL flag but that seems kinda terrible. Not sure what to do here.

As I mentioned a new cast flag would be better here.

arnaud-lb · 2023-01-27T18:31:25Z

Since this targets master, we could add a flags parameter on the cast operation?

Girgias · 2023-01-27T20:13:50Z

I'm not a fan of adding a special case for user-space streams.

php_stream_temp_cast can emit errors when php_stream_fopen_tmpfile fails. I think that php_openssl_sockop_cast may emit errors too, when calling php_stream_fill_read_buffer, but I'm not sure that can be suppressed.

Do we already have tests for those, if yes do you know which ones? Otherwise would be good to add some.

Since this targets master, we could add a flags parameter on the cast operation?

This was meant as a bugfix for 8.1 initally

I thought about adding a flag parameter, but that's an API break and wasn't sure if it was worth to do it.

bukka

I have been thinking about this and I don't this this is the right approach. We should not be doing special exceptions for user in cast code.

There seems to be quite common pattern when we don't want to emit some warnings during casting check. As mentioned this does not need to be specific to the user stream. I think we should introduce a new flag (castas param) for that purpose (e.g. PHP_STREAM_CAST_CHECK) and disable selected warnings in such case. That would be more generic solution IMHO.

bukka · 2023-07-07T11:18:05Z

main/streams/cast.c

+		/* Do special handling for user streams as they may not implement the cast method */
+		if (!show_err && is_stream_user_stream(stream)) {
+			castas |= PHP_STREAM_FLAG_SUPPRESS_ERRORS;
+		}


Why is this done here? This checks ret which means it won't be executed during casting check as ret is null. This should be only executed during the actual casting so why did you want to suppress it here as well?

Doesn't it make more sense to use that fact to check if an error should be emitted or not rather than a flag then?

bukka · 2023-07-07T11:20:06Z

main/streams/cast.c

@@ -268,7 +282,7 @@ PHPAPI int _php_stream_cast(php_stream *stream, int castas, void **ret, int show
 		} else if (flags & PHP_STREAM_CAST_TRY_HARD) {
 			php_stream *newstream;

-			newstream = php_stream_fopen_tmpfile();
+			newstream = php_stream_fopen_tmpfile_ex(/* emit errors */ false);


From what I see PHP_STREAM_CAST_TRY_HARD is used only during the actual casting so why do you suppress errors here?

That is not how I see it. We do not control what flag is being passed in what context, and if someone wants to check that a stream is castable by trying hard before attempting to do so they are currently allowed to do as such.

bukka

The logic seems ok. Just needs a little bit of clean up.

main/streams/cast.c

main/streams/php_stream_plain_wrapper.h

main/streams/userspace.c

bukka · 2023-08-26T19:51:10Z

main/streams/userspace.c

@@ -1398,22 +1400,28 @@ static int php_userstreamop_cast(php_stream *stream, int castas, void **retptr)

 	do {
 		if (call_result == FAILURE) {
-			php_error_docref(NULL, E_WARNING, "%s::" USERSTREAM_CAST " is not implemented!",
+			if (report_errors) {
+				php_error_docref(NULL, E_WARNING, "%s::" USERSTREAM_CAST " is not implemented!",
 					ZSTR_VAL(us->wrapper->ce->name));


NIT: it looks to me that stream convention is to use two tabs indent for continous indent so it would be nice to keep it that way.

bukka · 2023-08-26T20:08:40Z

main/streams/userspace.c

@@ -1382,6 +1382,8 @@ static int php_userstreamop_cast(php_stream *stream, int castas, void **retptr)
 	php_stream * intstream = NULL;
 	int call_result;
 	int ret = FAILURE;
+	/* If we a checking if the stream can cast, no return pointer is provided, so do not emit errors */
+	bool report_errors = retptr;


Seems a bit hacky but I guess it's still better that hold call state in the stream struct so I'm cool with this. Not sure if we need temp var but not a big issue with that either...

…r a cast

PHP_STREAM_CAST_MASK does not contain the REPORT_ERROR bit value This reverts commit 9617c45.

…r is NULL

Girgias · 2023-08-30T20:07:47Z

Should I backport this to 8.1 @bukka ?

bukka · 2023-09-07T13:58:30Z

@Girgias I would target only 8.3 as removal of warnings might potentially break some code even though it's unlikely. But considering it's been there for ages, only 8.3 should be fine here.

Girgias added Bug Category: Streams labels Jan 24, 2023

Girgias requested review from bukka and arnaud-lb January 24, 2023 17:01

github-actions bot added the Extension: standard label Jan 24, 2023

arnaud-lb reviewed Jan 27, 2023

View reviewed changes

Girgias force-pushed the non-cast-userstream-warns branch from a2a9873 to c617e15 Compare January 27, 2023 20:43

Girgias force-pushed the non-cast-userstream-warns branch from c617e15 to c5e04e2 Compare February 5, 2023 17:30

bukka requested changes Jul 7, 2023

View reviewed changes

bukka reviewed Jul 7, 2023

View reviewed changes

Girgias force-pushed the non-cast-userstream-warns branch from c5e04e2 to fbe2070 Compare July 17, 2023 05:55

github-actions bot removed the Extension: standard label Jul 17, 2023

Girgias requested a review from bukka July 25, 2023 09:56

bukka requested changes Aug 26, 2023

View reviewed changes

Girgias added 9 commits August 30, 2023 21:00

Add test about attempting to cast an uncastable userstream

f1e2eea

Do not warn that user stream does not implement cast when checking fo…

a6d5547

…r a cast

Compare php_stream_ops pointer instead of label

de52f73

Add silent version of php_stream_fopen_tmpfile()

7dd6bb9

Use PHP_STREAM_CAST_MASK to mask flags in castas param

50e4b7f

Revert "Use PHP_STREAM_CAST_MASK to mask flags in castas param"

615a367

PHP_STREAM_CAST_MASK does not contain the REPORT_ERROR bit value This reverts commit 9617c45.

Use fact that if checking that a stream can be cast the return pointe…

9098b1a

…r is NULL

Drop remains of a now useless change

2f343c1

Drop remains of a now useless change

0f7149e

Girgias changed the base branch from master to PHP-8.3 August 30, 2023 20:06

Code review comments

4dd3caf

Girgias force-pushed the non-cast-userstream-warns branch from fbe2070 to 4dd3caf Compare August 30, 2023 20:07

Girgias closed this in d68073c Sep 8, 2023

Girgias deleted the non-cast-userstream-warns branch September 8, 2023 12:23

		return (strlen(stream->ops->label) == strlen("user-space"))
		&& strncmp(stream->ops->label, "user-space", strlen("user-space")) == 0;

	int flags = castas & PHP_STREAM_CAST_MASK;
	castas &= ~PHP_STREAM_CAST_MASK;

Fix: Checking if a stream is castable should not emit warnings for user defined streams #10435

Fix: Checking if a stream is castable should not emit warnings for user defined streams #10435

Uh oh!

Conversation

Girgias commented Jan 24, 2023

Uh oh!

arnaud-lb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaud-lb commented Jan 27, 2023

Uh oh!

Girgias commented Jan 27, 2023

Uh oh!

bukka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias commented Aug 30, 2023

Uh oh!

bukka commented Sep 7, 2023

Uh oh!

Uh oh!