Skip to content

ext/ftp: apply the connect timeout ceiling to the remaining entry points#22767

Closed
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:ftp-ss001-ssl-timeout-cap
Closed

ext/ftp: apply the connect timeout ceiling to the remaining entry points#22767
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:ftp-ss001-ssl-timeout-cap

Conversation

@iliaal

@iliaal iliaal commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

GH-20601 capped the timeout in ftp_connect() so a value that overflows the timeval conversion cannot reach php_network_set_limit_time(). ftp_ssl_connect() and ftp_set_option(FTP_TIMEOUT_SEC) kept rejecting only non-positive values, so PHP_INT_MAX still reached ftp_open() and my_poll() through them. Both now apply the same bound.

The three entry points before this change:

ftp_connect('127.0.0.1', 1024, PHP_INT_MAX);              // ValueError
ftp_ssl_connect('127.0.0.1', 1024, PHP_INT_MAX);          // hangs
ftp_set_option($c, FTP_TIMEOUT_SEC, PHP_INT_MAX);         // bool(true)

Floor PHP-8.4, matching where GH-20601 landed.

iliaal added a commit to iliaal/php-src that referenced this pull request Jul 16, 2026
phpGH-20601 capped the timeout in ftp_connect() so a value that overflows the
timeval conversion cannot reach php_network_set_limit_time(). ftp_ssl_connect()
and ftp_set_option(FTP_TIMEOUT_SEC) kept rejecting only non-positive values, so
PHP_INT_MAX still reached ftp_open() and my_poll() through them.

Apply the same bound to both. ftp_ssl_connect() with PHP_INT_MAX hung before
this rather than reporting the bad argument.

Closes phpGH-22767
@iliaal
iliaal force-pushed the ftp-ss001-ssl-timeout-cap branch from 274cbec to fc3018d Compare July 16, 2026 16:24
@iliaal
iliaal requested review from devnexen and ndossche July 16, 2026 16:25
@ndossche

Copy link
Copy Markdown
Member

Can you factor out the maximum constant to a define or something like that?

iliaal added a commit to iliaal/php-src that referenced this pull request Jul 16, 2026
phpGH-20601 capped the timeout in ftp_connect() so a value that overflows the
timeval conversion cannot reach php_network_set_limit_time(). ftp_ssl_connect()
and ftp_set_option(FTP_TIMEOUT_SEC) kept rejecting only non-positive values, so
PHP_INT_MAX still reached ftp_open() and my_poll() through them.

Apply the same bound to both, factored into a shared PHP_FTP_TIMEOUT_SEC_MAX
macro. ftp_ssl_connect() with PHP_INT_MAX hung before this rather than
reporting the bad argument.

Closes phpGH-22767
@iliaal
iliaal force-pushed the ftp-ss001-ssl-timeout-cap branch from fc3018d to 616205a Compare July 16, 2026 23:54
@iliaal

iliaal commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Factored into PHP_FTP_TIMEOUT_SEC_MAX, now used by all three entry points.

Comment thread ext/ftp/tests/gh20601_set_option.phpt Outdated
require 'server.inc';

$ftp = ftp_connect('127.0.0.1', $port);
ftp_login($ftp, 'user', 'pass');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: it seems you need to invert this line and the next ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, reordered.

phpGH-20601 capped the timeout in ftp_connect() so a value that overflows the
timeval conversion cannot reach php_network_set_limit_time(). ftp_ssl_connect()
and ftp_set_option(FTP_TIMEOUT_SEC) kept rejecting only non-positive values, so
PHP_INT_MAX still reached ftp_open() and my_poll() through them.

Apply the same bound to both, factored into a shared PHP_FTP_TIMEOUT_SEC_MAX
macro. ftp_ssl_connect() with PHP_INT_MAX hung before this rather than
reporting the bad argument.

Closes phpGH-22767
@iliaal
iliaal force-pushed the ftp-ss001-ssl-timeout-cap branch from 616205a to 3c88014 Compare July 17, 2026 18:45
@iliaal iliaal closed this in c1773a1 Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants