Skip to content

ext/sockets: Additional refactorings to socket_set_option() #17186

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

Merged
merged 9 commits into from
Dec 29, 2024
Merged

ext/sockets: Additional refactorings to socket_set_option() #17186

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
eb86bb4
ext/socket: Reduce scope of variables
Girgias Dec 16, 2024
64f819f
ext/socket: Throw TypeErrors when not passing a string for options re…
Girgias Dec 17, 2024
c00fc6f
ext/sockets: Throw ValueError when string has null bytes for options …
Girgias Dec 17, 2024
7a904bb
ext/sockets: Throw ValueError when string is too long
Girgias Dec 17, 2024
476d01e
fixes and comment
Girgias Dec 17, 2024
e13d106
fixes
Girgias Dec 17, 2024
9eaac69
Fix considition
Girgias Dec 28, 2024
d1b19e7
Use SOL_TCP as this is how the extension exposes the const to userland
Girgias Dec 28, 2024
3e08a78
fix condition
Girgias Dec 29, 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
60 changes: 38 additions & 22 deletions ext/sockets/sockets.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1881,8 +1881,6 @@ PHP_FUNCTION(socket_set_option)
zend_long level, optname;
void *opt_ptr;
HashTable *opt_ht;
zval *l_onoff, *l_linger;
zval *sec, *usec;

ZEND_PARSE_PARAMETERS_START(4, 4)
Z_PARAM_OBJECT_OF_CLASS(arg1, socket_ce)
Expand Down Expand Up @@ -1923,13 +1921,12 @@ PHP_FUNCTION(socket_set_option)
switch (optname) {
#ifdef TCP_CONGESTION
case TCP_CONGESTION: {
if (Z_TYPE_P(arg4) == IS_STRING) {
opt_ptr = Z_STRVAL_P(arg4);
optlen = Z_STRLEN_P(arg4);
} else {
opt_ptr = "";
optlen = 0;
if (Z_TYPE_P(arg4) != IS_STRING) {
zend_argument_type_error(4, "must be of type string when argument #3 ($option) is TCP_CONGESTION, %s given", zend_zval_value_name(arg4));
RETURN_THROWS();
}
opt_ptr = Z_STRVAL_P(arg4);
optlen = Z_STRLEN_P(arg4);
if (setsockopt(php_sock->bsd_socket, level, optname, opt_ptr, optlen) != 0) {
PHP_SOCKET_ERROR(php_sock, "Unable to set socket option", errno);
RETURN_FALSE;
Expand All @@ -1942,11 +1939,20 @@ PHP_FUNCTION(socket_set_option)
#ifdef TCP_FUNCTION_BLK
case TCP_FUNCTION_BLK: {
if (Z_TYPE_P(arg4) != IS_STRING) {
php_error_docref(NULL, E_WARNING, "Invalid tcp stack name argument type");
RETURN_FALSE;
zend_argument_type_error(4, "must be of type string when argument #3 ($option) is TCP_FUNCTION_BLK, %s given", zend_zval_value_name(arg4));
RETURN_THROWS();
}
if (zend_str_has_nul_byte(Z_STR_P(arg4))) {
zend_argument_value_error(4, "must not contain null bytes when argument #3 ($option) is TCP_FUNCTION_BLK");
RETURN_THROWS();
}
if (Z_STRLEN_P(arg4) > TCP_FUNCTION_NAME_LEN_MAX) {
zend_argument_value_error(4, "must be less than %zu bytes when argument #3 ($option) is TCP_FUNCTION_BLK", TCP_FUNCTION_NAME_LEN_MAX);
RETURN_THROWS();
}
struct tcp_function_set tfs = {0};
strlcpy(tfs.function_set_name, Z_STRVAL_P(arg4), TCP_FUNCTION_NAME_LEN_MAX);
/* Copy the trailing nul byte */
memcpy(tfs.function_set_name, Z_STRVAL_P(arg4), Z_STRLEN_P(arg4)+1);

optlen = sizeof(tfs);
opt_ptr = &tfs;
Expand All @@ -1969,6 +1975,7 @@ PHP_FUNCTION(socket_set_option)
case SO_LINGER: {
const char l_onoff_key[] = "l_onoff";
const char l_linger_key[] = "l_linger";
zval *l_onoff, *l_linger;

if (Z_TYPE_P(arg4) != IS_ARRAY) {
if (UNEXPECTED(Z_TYPE_P(arg4) != IS_OBJECT)) {
Expand Down Expand Up @@ -2015,6 +2022,7 @@ PHP_FUNCTION(socket_set_option)
case SO_SNDTIMEO: {
const char sec_key[] = "sec";
const char usec_key[] = "usec";
zval *sec, *usec;

if (Z_TYPE_P(arg4) != IS_ARRAY) {
if (UNEXPECTED(Z_TYPE_P(arg4) != IS_OBJECT)) {
Expand Down Expand Up @@ -2054,25 +2062,33 @@ PHP_FUNCTION(socket_set_option)
}
#ifdef SO_BINDTODEVICE
case SO_BINDTODEVICE: {
if (Z_TYPE_P(arg4) == IS_STRING) {
opt_ptr = Z_STRVAL_P(arg4);
optlen = Z_STRLEN_P(arg4);
} else {
opt_ptr = "";
optlen = 0;
if (Z_TYPE_P(arg4) != IS_STRING) {
zend_argument_type_error(4, "must be of type string when argument #3 ($option) is SO_BINDTODEVICE, %s given", zend_zval_value_name(arg4));
RETURN_THROWS();
}
opt_ptr = Z_STRVAL_P(arg4);
optlen = Z_STRLEN_P(arg4);
break;
}
#endif

#ifdef SO_ACCEPTFILTER
case SO_ACCEPTFILTER: {
if (Z_TYPE_P(arg4) != IS_STRING) {
php_error_docref(NULL, E_WARNING, "Invalid filter argument type");
RETURN_FALSE;
zend_argument_type_error(4, "must be of type string when argument #3 ($option) is SO_ACCEPTFILTER, %s given", zend_zval_value_name(arg4));
RETURN_THROWS();
}
if (zend_str_has_nul_byte(Z_STR_P(arg4))) {
zend_argument_value_error(4, "must not contain null bytes when argument #3 ($option) is SO_ACCEPTFILTER");
RETURN_THROWS();
}
if (Z_STRLEN_P(arg4) > sizeof(af.af_name)) {
Copy link
Member

Choose a reason for hiding this comment

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

Should that be Z_STRLEN_P(arg4) >= sizeof(af.af_name) (or Z_STRLEN_P(arg4)+1 > sizeof(af.af_name)) to account for the null byte?

Copy link
Member

@devnexen devnexen Dec 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Like here, it would be nice to have more explanations of why you feel we need to replace strlcpy with memcpy.
1/ It is easier to fall into overflows with memcpy as per Arnaud/my comments.
2/ If you want to check truncations, strlcpy return value can help in this.
3/ If this is performance gain and since those changes are mainly about FreeBSD parts, let's say we create a bunch of sockets and we set bbr tcp stack for each so how much of a difference it makes in practice (related buffers are respectively 32 and 16 larges) ? Albeit freebsd's memcpy finally caught up a lot in term of optimisations (highlighting the case for size <= 32 bytes here) since the 14.x release serie. In comparison our strlcpy version is very straightforward but today's compilers are smarter.

Whether the changes are justified, I do not think it is very urgent we can see after Xmas days no rush :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Because strlcpy() is bad as I just replied to that other thread, we also need to do extra work to instrumentalize this on top of using a non standard C function.

I don't mind adding a helper function if that's the concern.

Copy link
Member

@devnexen devnexen Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see ... in this case you still need to address arnaud and my comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll get round to it after the Xmas break :)

zend_argument_value_error(4, "must be less than %zu bytes when argument #3 ($option) is SO_ACCEPTFILTER", sizeof(af.af_name));
RETURN_THROWS();
}
struct accept_filter_arg af = {0};
strlcpy(af.af_name, Z_STRVAL_P(arg4), sizeof(af.af_name));
/* Copy the trailing nul byte */
memcpy(af.af_name, Z_STRVAL_P(arg4), Z_STRLEN_P(arg4)+1);
opt_ptr = &af;
optlen = sizeof(af);
break;
Expand All @@ -2087,8 +2103,8 @@ PHP_FUNCTION(socket_set_option)
RETURN_FALSE;
}
if (Z_TYPE_P(arg4) != IS_STRING) {
php_error_docref(NULL, E_WARNING, "Invalid filter argument type");
RETURN_FALSE;
zend_argument_type_error(4, "must be of type string when argument #3 ($option) is FIL_ATTACH or FIL_DETACH, %s given", zend_zval_value_name(arg4));
RETURN_THROWS();
}
opt_ptr = Z_STRVAL_P(arg4);
optlen = Z_STRLEN_P(arg4);
Expand Down
33 changes: 33 additions & 0 deletions ext/sockets/tests/socket_set_option_invalid_value_for_IPPROTO_TCP_TCP_CONGESTION.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
--TEST--
socket_set_option($socket, IPPROTO_TCP, TCP_CONGESTION, INVALID_TYPE_FOR_OPTION)
--EXTENSIONS--
sockets
--SKIPIF--
<?php

if (!defined('IPPROTO_TCP')) {
Copy link
Member

@devnexen devnexen Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IPPROTO_TCP had been translated into SOL_TCP, also no need to check its existence it s unconditionally set looking at the stub.

Copy link
Member

Choose a reason for hiding this comment

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

See my run of your new tests

image

die('skip IPPROTO_TCP not available.');
}
if (!defined('TCP_CONGESTION')) {
die('skip TCP_CONGESTION not available.');
}

?>
--FILE--
<?php
$socket = socket_create(AF_INET, SOCK_STREAM, SOL_TCP);
if (!$socket) {
die('Unable to create AF_INET socket [socket]');
}

try {
$ret = socket_set_option($socket, IPPROTO_TCP, TCP_CONGESTION, new stdClass());
var_dump($ret);
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}

socket_close($socket);
?>
--EXPECT--
TypeError: socket_set_option(): Argument #4 ($value) must be of type string when argument #3 ($option) is TCP_CONGESTION, stdClass given
47 changes: 47 additions & 0 deletions ext/sockets/tests/socket_set_option_invalid_value_for_IPPROTO_TCP_TCP_FUNCTION_BLK.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
--TEST--
socket_set_option($socket, IPPROTO_TCP, TCP_FUNCTION_BLK, INVALID_TYPE_FOR_OPTION)
--EXTENSIONS--
sockets
--SKIPIF--
<?php

if (!defined('IPPROTO_TCP')) {
die('skip IPPROTO_TCP not available.');
}
if (!defined('TCP_FUNCTION_BLK')) {
die('skip TCP_FUNCTION_BLK not available.');
}

?>
--FILE--
<?php
$socket = socket_create(AF_INET, SOCK_STREAM, SOL_TCP);
if (!$socket) {
die('Unable to create AF_INET socket [socket]');
}

try {
$ret = socket_set_option($socket, IPPROTO_TCP, TCP_FUNCTION_BLK, new stdClass());
var_dump($ret);
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}
try {
$ret = socket_set_option($socket, IPPROTO_TCP, TCP_FUNCTION_BLK, "string\0with\0null\0bytes");
var_dump($ret);
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}
try {
$ret = socket_set_option($socket, IPPROTO_TCP, TCP_FUNCTION_BLK, str_repeat("a", 2048);
var_dump($ret);
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}

socket_close($socket);
?>
--EXPECTF--
TypeError: socket_set_option(): Argument #4 ($value) must be of type string when argument #3 ($option) is TCP_FUNCTION_BLK, stdClass given
ValueError: socket_set_option(): Argument #4 ($value) must not contain null bytes when argument #3 ($option) is TCP_FUNCTION_BLK
ValueError: socket_set_option(): Argument #4 ($value) must be less than %d bytes when argument #3 ($option) is TCP_FUNCTION_BLK
44 changes: 44 additions & 0 deletions ext/sockets/tests/socket_set_option_invalid_value_for_SOL_FILTER_SO_ATTACH.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
--TEST--
socket_set_option($socket, SOL_SOCKET, SO_BINDTODEVICE, INVALID_TYPE_FOR_OPTION)
--EXTENSIONS--
sockets
--SKIPIF--
<?php

if (!defined('SOL_FILTER')) {
die('skip SOL_FILTER not available.');
}
if (!defined('FIL_ATTACH')) {
die('skip FIL_ATTACH not available.');
}
if (!defined('FIL_DETACH')) {
die('skip FIL_DETACH not available.');
}

?>
--FILE--
<?php
$socket = socket_create(AF_INET, SOCK_STREAM, SOL_TCP);
if (!$socket) {
die('Unable to create AF_INET socket [socket]');
}

// TODO Warning when using FIL_ATTACH/FIL_DETACH option when level is not SOL_FILTER
try {
$ret = socket_set_option($socket, SOL_FILTER, FIL_ATTACH, new stdClass());
var_dump($ret);
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}
try {
$ret = socket_set_option($socket, SOL_FILTER, FIL_DETACH, new stdClass());
var_dump($ret);
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}

socket_close($socket);
?>
--EXPECT--
TypeError: socket_set_option(): Argument #4 ($value) must be of type string when argument #3 ($option) is FIL_ATTACH or FIL_DETACH, stdClass given
TypeError: socket_set_option(): Argument #4 ($value) must be of type string when argument #3 ($option) is FIL_ATTACH or FIL_DETACH, stdClass given
47 changes: 47 additions & 0 deletions ext/sockets/tests/socket_set_option_invalid_value_for_SOL_SOCKET_SO_ACCEPTFILTER.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
--TEST--
socket_set_option($socket, SOL_SOCKET, SO_ACCEPTFILTER, INVALID_TYPE_FOR_OPTION)
--EXTENSIONS--
sockets
--SKIPIF--
<?php

if (!defined('SOL_SOCKET')) {
die('skip SOL_SOCKET not available.');
}
if (!defined('SO_ACCEPTFILTER')) {
die('skip SO_ACCEPTFILTER not available.');
}

?>
--FILE--
<?php
$socket = socket_create(AF_INET, SOCK_STREAM, SOL_TCP);
if (!$socket) {
die('Unable to create AF_INET socket [socket]');
}

try {
$ret = socket_set_option($socket, SOL_SOCKET, SO_ACCEPTFILTER, new stdClass());
var_dump($ret);
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}
try {
$ret = socket_set_option($socket, SOL_SOCKET, SO_ACCEPTFILTER, "string\0with\0null\0bytes");
var_dump($ret);
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}
try {
$ret = socket_set_option($socket, SOL_SOCKET, SO_ACCEPTFILTER, str_repeat("a", 2048);
var_dump($ret);
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}

socket_close($socket);
?>
--EXPECTF--
TypeError: socket_set_option(): Argument #4 ($value) must be of type string when argument #3 ($option) is SO_ACCEPTFILTER, stdClass given
ValueError: socket_set_option(): Argument #4 ($value) must not contain null bytes when argument #3 ($option) is SO_ACCEPTFILTER
ValueError: socket_set_option(): Argument #4 ($value) must be less than %d bytes when argument #3 ($option) is SO_ACCEPTFILTER
33 changes: 33 additions & 0 deletions ext/sockets/tests/socket_set_option_invalid_value_for_SOL_SOCKET_SO_BINDTODEVICE.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
--TEST--
socket_set_option($socket, SOL_SOCKET, SO_BINDTODEVICE, INVALID_TYPE_FOR_OPTION)
--EXTENSIONS--
sockets
--SKIPIF--
<?php

if (!defined('SOL_SOCKET')) {
die('skip SOL_SOCKET not available.');
}
if (!defined('SO_BINDTODEVICE')) {
die('skip SO_BINDTODEVICE not available.');
}

?>
--FILE--
<?php
$socket = socket_create(AF_INET, SOCK_STREAM, SOL_TCP);
if (!$socket) {
die('Unable to create AF_INET socket [socket]');
}

try {
$ret = socket_set_option($socket, SOL_SOCKET, SO_BINDTODEVICE, new stdClass());
var_dump($ret);
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}

socket_close($socket);
?>
--EXPECT--
TypeError: socket_set_option(): Argument #4 ($value) must be of type string when argument #3 ($option) is SO_BINDTODEVICE, stdClass given
Loading