Skip to content

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

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 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e4eeda
ext/sockets: socket_set_option switch from convert_to_long to zval_tr…
devnexen Dec 12, 2024
45270c6
add test
devnexen Dec 12, 2024
695861e
changes from review
devnexen Dec 13, 2024
6c5f436
add test case
devnexen Dec 13, 2024
1a14443
changes from review also attempt to prevent UB with timeout on windows.
devnexen Dec 13, 2024
e18fbb3
remove UB check and using zval_get_long instead
devnexen Dec 13, 2024
d6e64e6
other changes from review
devnexen Dec 15, 2024
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
50 changes: 34 additions & 16 deletions ext/sockets/sockets.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

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

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.

RETURN_THROWS();
}

opt_ht = Z_ARRVAL_P(arg4);

if ((l_onoff = zend_hash_str_find(opt_ht, l_onoff_key, sizeof(l_onoff_key) - 1)) == NULL) {
Expand All @@ -1883,11 +1887,21 @@ PHP_FUNCTION(socket_set_option)
RETURN_THROWS();
}

convert_to_long(l_onoff);
convert_to_long(l_linger);
zend_long vall_lonoff = zval_get_long(l_onoff);
zend_long vall_linger = zval_get_long(l_linger);
Copy link
Member

Choose a reason for hiding this comment

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

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);


lv.l_onoff = (unsigned short)Z_LVAL_P(l_onoff);
lv.l_linger = (unsigned short)Z_LVAL_P(l_linger);
if (vall_lonoff < 0 || vall_lonoff > USHRT_MAX) {
zend_argument_value_error(4, "\"%s\" must be between 0 and %u", l_onoff_key, USHRT_MAX);
RETURN_THROWS();
}

if (vall_linger < 0 || vall_linger > USHRT_MAX) {
zend_argument_value_error(4, "\"%s\" must be between 0 and %d", l_linger, USHRT_MAX);
RETURN_THROWS();
}

lv.l_onoff = (unsigned short)vall_lonoff;
lv.l_linger = (unsigned short)vall_linger;

optlen = sizeof(lv);
opt_ptr = &lv;
Expand All @@ -1899,7 +1913,11 @@ PHP_FUNCTION(socket_set_option)
const char sec_key[] = "sec";
const char usec_key[] = "usec";

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));
RETURN_THROWS();
}

opt_ht = Z_ARRVAL_P(arg4);

if ((sec = zend_hash_str_find(opt_ht, sec_key, sizeof(sec_key) - 1)) == NULL) {
Expand All @@ -1911,15 +1929,16 @@ PHP_FUNCTION(socket_set_option)
RETURN_THROWS();
}

convert_to_long(sec);
convert_to_long(usec);
zend_long valsec = zval_get_long(sec);
zend_long valusec = zval_get_long(usec);
#ifndef PHP_WIN32
tv.tv_sec = Z_LVAL_P(sec);
tv.tv_usec = Z_LVAL_P(usec);
tv.tv_sec = valsec;
tv.tv_usec = valusec;
optlen = sizeof(tv);
opt_ptr = &tv;
#else
timeout = Z_LVAL_P(sec) * 1000 + Z_LVAL_P(usec) / 1000;
timeout = valsec * 1000 + valusec / 1000;

optlen = sizeof(int);
opt_ptr = &timeout;
#endif
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

fval ? what does the f stand for?

Copy link
Member Author

Choose a reason for hiding this comment

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

f for function (for CBPF) val.

Copy link
Member

Choose a reason for hiding this comment

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

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


if (!Z_LVAL_P(arg4)) {
if (!fval) {
ov = 1;
optlen = sizeof(ov);
opt_ptr = &ov;
optname = SO_DETACH_BPF;
} else {
uint32_t k = (uint32_t)Z_LVAL_P(arg4);
uint32_t k = (uint32_t)fval;
static struct sock_filter cbpf[8] = {0};
static struct sock_fprog bpfprog;

Expand All @@ -2006,8 +2025,7 @@ PHP_FUNCTION(socket_set_option)

default:
default_case:
convert_to_long(arg4);
ov = Z_LVAL_P(arg4);
ov = zval_get_long(arg4);

optlen = sizeof(ov);
opt_ptr = &ov;
Expand Down
9 changes: 8 additions & 1 deletion ext/sockets/tests/socket_reuseport_cbpf.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,20 @@ if (!$socket) {
}
var_dump(socket_set_option( $socket, SOL_SOCKET, SO_REUSEADDR, true));
var_dump(socket_set_option( $socket, SOL_SOCKET, SO_REUSEPORT, true));
try {
socket_set_option( $socket, SOL_SOCKET, SO_ATTACH_REUSEPORT_CBPF, array());
} catch (\TypeError $e) {
echo $e->getMessage() . PHP_EOL;
}
var_dump(socket_set_option( $socket, SOL_SOCKET, SO_ATTACH_REUSEPORT_CBPF, SKF_AD_CPU));
var_dump(socket_bind($socket, '0.0.0.0'));
socket_listen($socket);
socket_close($socket);
?>
--EXPECT--
--EXPECTF--
bool(true)
bool(true)

Warning: socket_set_option(): Unable to set socket option [2]: No such file or directory in %s on line %d
bool(true)
bool(true)
60 changes: 60 additions & 0 deletions ext/sockets/tests/socket_set_option_timeo_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
--TEST--
socket_set_option() with SO_RCVTIMEO/SO_SNDTIMEO/SO_LINGER
--EXTENSIONS--
sockets
--FILE--
<?php
$socket = socket_create(AF_INET, SOCK_STREAM, SOL_TCP);
if (!$socket) {
die('Unable to create AF_INET socket [socket]');
}
$options_1 = array("sec" => 1, "usec" => "aaaaa");
$options_2 = array("sec" => new stdClass(), "usec" => "1");
$options_3 = array("l_onoff" => "aaaa", "l_linger" => "1");
$options_4 = array("l_onoff" => "1", "l_linger" => []);
$options_5 = array("l_onoff" => PHP_INT_MAX, "l_linger" => "1");

try {
socket_set_option( $socket, SOL_SOCKET, SO_RCVTIMEO, new stdClass);
} catch (\TypeError $e) {
echo $e->getMessage() . PHP_EOL;
}

try {
socket_set_option( $socket, SOL_SOCKET, SO_RCVTIMEO, $options_1);
} catch (\TypeError $e) {
echo $e->getMessage() . PHP_EOL;
}

try {
socket_set_option( $socket, SOL_SOCKET, SO_SNDTIMEO, $options_2);
} catch (\TypeError $e) {
echo $e->getMessage() . PHP_EOL;
}
try {
socket_set_option( $socket, SOL_SOCKET, SO_LINGER, "not good");
} catch (\TypeError $e) {
echo $e->getMessage() . PHP_EOL;
}
try {
socket_set_option( $socket, SOL_SOCKET, SO_LINGER, $options_3);
} catch (\TypeError $e) {
echo $e->getMessage() . PHP_EOL;
}
try {
socket_set_option( $socket, SOL_SOCKET, SO_LINGER, $options_4);
} catch (\TypeError $e) {
echo $e->getMessage() . PHP_EOL;
}
try {
socket_set_option( $socket, SOL_SOCKET, SO_LINGER, $options_5);
} catch (\ValueError $e) {
echo $e->getMessage() . PHP_EOL;
}
?>
--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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 */ } }

Copy link
Member

Choose a reason for hiding this comment

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

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

socket_set_option(): Argument #4 ($value) must be of type array, string given
socket_set_option(): Argument #4 ($value) "l_onoff" must be between 0 and %d
Loading