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 2 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
53 changes: 39 additions & 14 deletions ext/sockets/sockets.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1811,6 +1811,7 @@ PHP_FUNCTION(socket_set_option)
HashTable *opt_ht;
zval *l_onoff, *l_linger;
zval *sec, *usec;
bool failed;
Copy link
Member

Choose a reason for hiding this comment

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

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


if (zend_parse_parameters(ZEND_NUM_ARGS(), "Ollz", &arg1, socket_ce, &level, &optname, &arg4) == FAILURE) {
RETURN_THROWS();
Expand Down Expand Up @@ -1883,11 +1884,19 @@ 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);
Copy link
Member

Choose a reason for hiding this comment

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

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

if (failed) {
zend_argument_type_error(4, "\"%s\" must be an int, %s given", l_onoff_key, zend_zval_value_name(l_onoff));
Copy link
Member

Choose a reason for hiding this comment

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

Wording

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

Choose a reason for hiding this comment

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

Wording

RETURN_THROWS();
}

lv.l_onoff = (unsigned short)Z_LVAL_P(l_onoff);
lv.l_linger = (unsigned short)Z_LVAL_P(l_linger);
lv.l_onoff = (unsigned short)zl_onoff;
lv.l_linger = (unsigned short)zl_linger;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Already happens in the lines above.


optlen = sizeof(lv);
opt_ptr = &lv;
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

This convert_to_array() appears to be superfluous.

opt_ht = Z_ARRVAL_P(arg4);
Expand All @@ -1911,15 +1921,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);
Copy link
Member

Choose a reason for hiding this comment

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

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

if (failed) {
zend_argument_type_error(4, "\"%s\" must be an int, %s given", sec_key, zend_zval_value_name(sec));
Copy link
Member

Choose a reason for hiding this comment

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

Wording

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

Choose a reason for hiding this comment

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

Ditto

RETURN_THROWS();
}
#ifndef PHP_WIN32
tv.tv_sec = Z_LVAL_P(sec);
tv.tv_usec = Z_LVAL_P(usec);
tv.tv_sec = zsec;
tv.tv_usec = zusec;
optlen = sizeof(tv);
opt_ptr = &tv;
#else
timeout = Z_LVAL_P(sec) * 1000 + Z_LVAL_P(usec) / 1000;
timeout = zsec * 1000 + zusec / 1000;
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'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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

optlen = sizeof(int);
opt_ptr = &timeout;
#endif
Expand Down Expand Up @@ -1971,15 +1989,19 @@ PHP_FUNCTION(socket_set_option)

#ifdef SO_ATTACH_REUSEPORT_CBPF
case SO_ATTACH_REUSEPORT_CBPF: {
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));
Copy link
Member

Choose a reason for hiding this comment

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

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

RETURN_THROWS();
}

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 +2028,11 @@ PHP_FUNCTION(socket_set_option)

default:
default_case:
convert_to_long(arg4);
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));
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

RETURN_THROWS();
}

optlen = sizeof(ov);
opt_ptr = &ov;
Expand Down
6 changes: 6 additions & 0 deletions 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,6 +18,11 @@ 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);
Expand All @@ -26,5 +31,6 @@ socket_close($socket);
--EXPECT--
bool(true)
bool(true)
socket_set_option(): Argument #4 ($value) must be an int, array given
bool(true)
bool(true)
42 changes: 42 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,42 @@
--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" => []);

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, $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;
}
?>
--EXPECT--
socket_set_option(): Argument #4 ($value) "usec" must be an int, string given
socket_set_option(): Argument #4 ($value) "sec" must be an int, stdClass given
socket_set_option(): Argument #4 ($value) "l_onoff" must be an int, string given
socket_set_option(): Argument #4 ($value) "l_linger" must be an int, array given
Loading