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 4 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
72 changes: 58 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;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "Ollz", &arg1, socket_ce, &level, &optname, &arg4) == FAILURE) {
RETURN_THROWS();
Expand Down Expand Up @@ -1871,6 +1872,11 @@ PHP_FUNCTION(socket_set_option)
const char l_onoff_key[] = "l_onoff";
const char l_linger_key[] = "l_linger";

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

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 appears to be superfluous, since we just checked that it is an array (and otherwise bailed out).

opt_ht = Z_ARRVAL_P(arg4);

Expand All @@ -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);
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 of type int, %s given", l_onoff_key, zend_zval_value_name(l_onoff));
RETURN_THROWS();
}
zend_long zl_linger = zval_try_get_long(l_linger, &failed);
if (failed) {
zend_argument_type_error(4, "\"%s\" must be of type int, %s given", l_linger_key, zend_zval_value_name(l_linger));
RETURN_THROWS();
}

if (zl_onoff < 0 || zl_onoff > USHRT_MAX) {
zend_argument_value_error(4, "\"%s\" must be between 0 and %u", l_onoff_key, USHRT_MAX);
RETURN_THROWS();
}

if (zl_linger < 0 || zl_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)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 @@ -1899,6 +1923,11 @@ PHP_FUNCTION(socket_set_option)
const char sec_key[] = "sec";
const char usec_key[] = "usec";

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

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 +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);
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 of type int, %s given", sec_key, zend_zval_value_name(sec));
RETURN_THROWS();
}
zend_long zusec = zval_try_get_long(usec, &failed);
if (failed) {
zend_argument_type_error(4, "\"%s\" must be of type int, %s given", usec_key, zend_zval_value_name(usec));
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 +2008,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 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 +2047,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 of type int, array given
bool(true)
bool(true)
62 changes: 62 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,62 @@
--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
socket_set_option(): Argument #4 ($value) "usec" must be of type int, string given
socket_set_option(): Argument #4 ($value) "sec" must be of type int, stdClass given
socket_set_option(): Argument #4 ($value) must be of type array, string given
socket_set_option(): Argument #4 ($value) "l_onoff" must be of type int, string given
socket_set_option(): Argument #4 ($value) "l_linger" must be of type int, array given
socket_set_option(): Argument #4 ($value) "l_onoff" must be between 0 and %d
Loading