ext/sockets: socket_set_option switch from convert_to_long to zval_tr… #17135

devnexen · 2024-12-12T22:12:10Z

…y_get_long.

to be explicit when the expected type is not met.

…y_get_long. to be explicit when the expected type is not met.

Girgias · 2024-12-13T01:37:08Z

ext/sockets/sockets.c

@@ -1811,6 +1811,7 @@ PHP_FUNCTION(socket_set_option)
 	HashTable		 		*opt_ht;
 	zval 					*l_onoff, *l_linger;
 	zval		 			*sec, *usec;
+	bool                                    failed;


Bleh the indent doesn't match here. I would say to not bother, or remove the param name alignment.

Girgias · 2024-12-13T01:38:31Z

ext/sockets/sockets.c

+			lv.l_onoff = (unsigned short)zl_onoff;
+			lv.l_linger = (unsigned short)zl_linger;


Probably the zend_long should be checked to see if it fits in an unsigned short before.

Already happens in the lines above.

Girgias · 2024-12-13T01:39:22Z

ext/sockets/sockets.c

@@ -1898,6 +1907,7 @@ PHP_FUNCTION(socket_set_option)
 		case SO_SNDTIMEO: {
 			const char sec_key[] = "sec";
 			const char usec_key[] = "usec";
+			bool failed;

 			convert_to_array(arg4);


This could also be updated to check that the value is an array, as if it is not the zend_hash_str_find calls below will fail. Similarly for the other function.

This convert_to_array() appears to be superfluous.

Girgias · 2024-12-13T01:40:18Z

ext/sockets/sockets.c

-			convert_to_long(arg4);
+			zend_long fval = zval_try_get_long(arg4, &failed);
+			if (failed) {
+				zend_argument_type_error(4, "must be an int, %s given", zend_zval_value_name(arg4));


Suggested change

zend_argument_type_error(4, "must be an int, %s given", zend_zval_value_name(arg4));

zend_argument_type_error(4, "must be of type int, %s given", zend_zval_value_name(arg4));

Girgias · 2024-12-13T01:40:24Z

ext/sockets/sockets.c

-			ov = Z_LVAL_P(arg4);
+			ov = zval_try_get_long(arg4, &failed);
+			if (failed) {
+				zend_argument_type_error(4, "must be an int, %s given", zend_zval_value_name(arg4));


Girgias · 2024-12-13T01:40:31Z

ext/sockets/sockets.c

-			convert_to_long(usec);
+			zend_long zsec = zval_try_get_long(sec, &failed);
+			if (failed) {
+				zend_argument_type_error(4, "\"%s\" must be an int, %s given", sec_key, zend_zval_value_name(sec));


Girgias · 2024-12-13T01:40:35Z

ext/sockets/sockets.c

+			}
+			zend_long zusec = zval_try_get_long(usec, &failed);
+			if (failed) {
+				zend_argument_type_error(4, "\"%s\" must be an int, %s given", usec_key, zend_zval_value_name(usec));


Girgias · 2024-12-13T01:40:43Z

ext/sockets/sockets.c

+			}
+			zend_long zl_linger = zval_try_get_long(l_linger, &failed);
+			if (failed) {
+				zend_argument_type_error(4, "\"%s\" must be an int, %s given", l_linger_key, zend_zval_value_name(l_linger));


Girgias · 2024-12-13T01:40:47Z

ext/sockets/sockets.c

-			convert_to_long(l_linger);
+			zend_long zl_onoff = zval_try_get_long(l_onoff, &failed);
+			if (failed) {
+				zend_argument_type_error(4, "\"%s\" must be an int, %s given", l_onoff_key, zend_zval_value_name(l_onoff));


cmb69

Thank you for taking a stab at this!

cmb69 · 2024-12-13T18:19:05Z

ext/sockets/sockets.c

+				zend_argument_type_error(4, "must be of type array, %s given", zend_zval_value_name(arg4));
+				RETURN_THROWS();
+			}
+
 			convert_to_array(arg4);


This appears to be superfluous, since we just checked that it is an array (and otherwise bailed out).

cmb69 · 2024-12-13T18:21:45Z

ext/sockets/sockets.c

@@ -1883,11 +1889,29 @@ PHP_FUNCTION(socket_set_option)
 				RETURN_THROWS();
 			}

-			convert_to_long(l_onoff);
-			convert_to_long(l_linger);
+			zend_long zl_onoff = zval_try_get_long(l_onoff, &failed);


Might also use zval_get_long() for better BC, but I don't have a strong opinion on this.

cmb69 · 2024-12-13T18:23:02Z

ext/sockets/sockets.c

+			lv.l_onoff = (unsigned short)zl_onoff;
+			lv.l_linger = (unsigned short)zl_linger;


Already happens in the lines above.

cmb69 · 2024-12-13T18:23:32Z

ext/sockets/sockets.c

@@ -1898,6 +1907,7 @@ PHP_FUNCTION(socket_set_option)
 		case SO_SNDTIMEO: {
 			const char sec_key[] = "sec";
 			const char usec_key[] = "usec";
+			bool failed;

 			convert_to_array(arg4);


This convert_to_array() appears to be superfluous.

cmb69 · 2024-12-13T18:33:18Z

ext/sockets/sockets.c

 			optlen = sizeof(tv);
 			opt_ptr = &tv;
 #else
-			timeout = Z_LVAL_P(sec) * 1000 + Z_LVAL_P(usec) / 1000;
+			timeout = zsec * 1000 + zusec / 1000;


I think I've missed that in the other PR: timeout is DWORD (that is 32bit unsigned), so the assignment might cause wrap-around. I think we should catch that.

I think I've missed that in the other PR: timeout is DWORD (that is 32bit unsigned), so the assignment might cause wrap-around. I think we should catch that.

DWORD on master tough here still int but issue can definitively occur very much.

Oops, I've mixed that up! If this targets PHP-8.3, we cannot change convert_to_long() to use zval_try_get_long() (too much BC break). Should better use zval_get_long() instead.

cmb69 · 2024-12-13T18:34:31Z

ext/sockets/sockets.c

@@ -1911,15 +1940,23 @@ PHP_FUNCTION(socket_set_option)
 				RETURN_THROWS();
 			}

-			convert_to_long(sec);
-			convert_to_long(usec);
+			zend_long zsec = zval_try_get_long(sec, &failed);


Naming is confusing here. zsec would appear to be a zval, and sec its corresponding long, but it's the other way round.

cmb69 · 2024-12-13T19:15:59Z

ext/sockets/sockets.c

+			if (valsec < 0 || valsec > INT_MAX / 1000) {
+				zend_argument_value_error(4, "\"%s\" must be between 0 and %d", sec_key, INT_MAX / 1000);
+				RETURN_THROWS();
+			}
+
+			timeout = valsec * 1000;
+
+			if (valusec < 0 || timeout > INT_MAX - (valusec / 1000)) {
+				zend_argument_value_error(4, "\"%s\" must be between 0 and %u", usec_key, (DWORD)(INT_MAX - (valusec / 1000)));
+				RETURN_THROWS();
+			}


I don't think this works for very large usec. We may also need to take BC into account (currently, negative usec are likely allowed for Windows). Maybe leave the overflow checking for master; while technically UB can happen, I think wrap-around is happening in practice (and nothing really bad).

Girgias · 2024-12-13T22:26:27Z

ext/sockets/sockets.c

@@ -1971,15 +1990,15 @@ PHP_FUNCTION(socket_set_option)

 #ifdef SO_ATTACH_REUSEPORT_CBPF
 		case SO_ATTACH_REUSEPORT_CBPF: {
-			convert_to_long(arg4);
+			zend_long fval = zval_get_long(arg4);


fval ? what does the f stand for?

f for function (for CBPF) val.

I think I would prefer cbpf_val then, as when I see fval I'm thinking "float val".

Girgias · 2024-12-13T22:27:38Z

ext/sockets/sockets.c

+			zend_long vall_lonoff = zval_get_long(l_onoff);
+			zend_long vall_linger = zval_get_long(l_linger);


Not sure why it is vall?

Suggested change

zend_long vall_lonoff = zval_get_long(l_onoff);

zend_long vall_linger = zval_get_long(l_linger);

zend_long val_lonoff = zval_get_long(l_onoff);

zend_long val_linger = zval_get_long(l_linger);

cmb69 · 2024-12-13T22:32:07Z

ext/sockets/tests/socket_set_option_timeo_error.phpt

+--EXPECTF--
+socket_set_option(): Argument #4 ($value) must be of type array, stdClass given
+
+Warning: Object of class stdClass could not be converted to int in %s on line %d


This is the only "detail" I'm not sure about regarding BC: is it okay to drop support for stdClass, ArrayObject and purpose built "data classes" (and maybe even objects in general)?

Maybe wait what @Girgias (or others) have to say about this.

If you are talking about the warning, I don't see how this is a BC breaks.
All the cases you mention already trigger this warning.

If you are talking about not supporting casting random objects to arrays, that is a recurring "problem" with PHP objects being structs, reference pointers, and everything in between. I don't think it is a problem is properly documented in UPGRADING for master, as most functions do not allow objects in those cases. Nor does ZPP unless one uses the special Z_PARAM_ARRAY_OR_OBJECT(_*) specifier (or A/H for non-fast ZPP).

Moreover, this causes issues when the value in question possibly needs to be interpreted like a string.

However, as this is a bugfix, I would do if (zval != IS_ARRAY) { /* slow path */ if (zval != IS_OBJECT) { /* throw */ } else { /* get HT from props */ } }

I was referring to the BC if we apply to stable branches (it's okay to be more strict for master).

Girgias · 2024-12-15T22:38:40Z

ext/sockets/sockets.c

@@ -1971,15 +1990,15 @@ PHP_FUNCTION(socket_set_option)

 #ifdef SO_ATTACH_REUSEPORT_CBPF
 		case SO_ATTACH_REUSEPORT_CBPF: {
-			convert_to_long(arg4);
+			zend_long fval = zval_get_long(arg4);


I think I would prefer cbpf_val then, as when I see fval I'm thinking "float val".

Girgias · 2024-12-15T22:41:48Z

ext/sockets/tests/socket_set_option_timeo_error.phpt

+--EXPECTF--
+socket_set_option(): Argument #4 ($value) must be of type array, stdClass given
+
+Warning: Object of class stdClass could not be converted to int in %s on line %d


If you are talking about the warning, I don't see how this is a BC breaks.
All the cases you mention already trigger this warning.

If you are talking about not supporting casting random objects to arrays, that is a recurring "problem" with PHP objects being structs, reference pointers, and everything in between. I don't think it is a problem is properly documented in UPGRADING for master, as most functions do not allow objects in those cases. Nor does ZPP unless one uses the special Z_PARAM_ARRAY_OR_OBJECT(_*) specifier (or A/H for non-fast ZPP).

Moreover, this causes issues when the value in question possibly needs to be interpreted like a string.

However, as this is a bugfix, I would do if (zval != IS_ARRAY) { /* slow path */ if (zval != IS_OBJECT) { /* throw */ } else { /* get HT from props */ } }

Girgias · 2024-12-15T22:45:39Z

ext/sockets/sockets.c

@@ -1871,7 +1871,11 @@ PHP_FUNCTION(socket_set_option)
 			const char l_onoff_key[] = "l_onoff";
 			const char l_linger_key[] = "l_linger";

-			convert_to_array(arg4);
+			if (Z_TYPE_P(arg4) != IS_ARRAY) {
+				zend_argument_type_error(4, "must be of type array, %s given", zend_zval_value_name(arg4));


I think adding what option we are talking about would help the error message.

Suggested change

zend_argument_type_error(4, "must be of type array, %s given", zend_zval_value_name(arg4));

zend_argument_type_error(4, "must be of type array when argument #3 ($option) is SO_LINGER, %s given", zend_zval_value_name(arg4));

And ditto for the other error cases.

Girgias

I think this looks good now, @CMB?

cmb69 · 2024-12-16T15:22:59Z

If ::get_properties() instead of ::get_properties_for(ZEND_PROP_PURPOSE_ARRAY_CAST) is good enough (what it likely is), I'm fine with the PR.

Note that I'm @cmb69, though. :)

…t_long. to be explicit when the expected type is not met. Check SO_LINGER values for possible overflow. close phpGH-17135

ext/sockets: socket_set_option switch from convert_to_long to zval_tr…

0e4eeda

…y_get_long. to be explicit when the expected type is not met.

github-actions bot added the Extension: sockets label Dec 12, 2024

add test

45270c6

devnexen marked this pull request as ready for review December 13, 2024 00:07

devnexen requested a review from Girgias December 13, 2024 00:07

Girgias requested changes Dec 13, 2024

View reviewed changes

devnexen added 2 commits December 13, 2024 06:50

changes from review

695861e

add test case

6c5f436

devnexen force-pushed the socket_set_option_try_to_long branch from 8d4b825 to 6c5f436 Compare December 13, 2024 07:01

cmb69 reviewed Dec 13, 2024

View reviewed changes

devnexen force-pushed the socket_set_option_try_to_long branch from 5c7768b to 1e1b1be Compare December 13, 2024 19:01

changes from review also attempt to prevent UB with timeout on windows.

1a14443

devnexen force-pushed the socket_set_option_try_to_long branch from 1e1b1be to 1a14443 Compare December 13, 2024 19:12

cmb69 reviewed Dec 13, 2024

View reviewed changes

remove UB check and using zval_get_long instead

e18fbb3

Girgias reviewed Dec 13, 2024

View reviewed changes

cmb69 reviewed Dec 13, 2024

View reviewed changes

Girgias reviewed Dec 15, 2024

View reviewed changes

other changes from review

d6e64e6

Girgias approved these changes Dec 16, 2024

View reviewed changes

devnexen closed this in 8a649a8 Dec 16, 2024

		lv.l_onoff = (unsigned short)zl_onoff;
		lv.l_linger = (unsigned short)zl_linger;

	zend_argument_type_error(4, "must be an int, %s given", zend_zval_value_name(arg4));
	zend_argument_type_error(4, "must be of type int, %s given", zend_zval_value_name(arg4));

		zend_long vall_lonoff = zval_get_long(l_onoff);
		zend_long vall_linger = zval_get_long(l_linger);

ext/sockets: socket_set_option switch from convert_to_long to zval_tr… #17135

ext/sockets: socket_set_option switch from convert_to_long to zval_tr… #17135

Uh oh!

Conversation

devnexen commented Dec 12, 2024

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmb69 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!

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!

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!

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!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

cmb69 commented Dec 16, 2024

Uh oh!

Uh oh!