Fix GH-20601: ftp_connect() timeout argument overflow. #20603

devnexen · 2025-11-27T19:37:43Z

No description provided.

bukka · 2025-11-29T10:33:09Z

main/network.c

 		struct timeval *timeout)
 {
 	gettimeofday(limit_time, NULL);
+	const double timeoutmax = (double) PHP_TIMEOUT_ULL_MAX / 1000000.0;


I'm just checking the whole logic here and I wonder why PHP_TIMEOUT_ULL_MAX is unsigned int 64? The time_t is signed int 64 (or 32 on 32bit arch) if I'm not mistakend. And in this case this is later converted to int (see php_pollfd_for and mainly php_tvtoto). So what am I missing?

Yes it s unsigned but my sense since it s that way because this constant is never used directly but converted from microseconds value to seconds (so yes unsigned int64 > double > time_t or int).

But how is this covering the lower type overflows? I mean if it gets converted in php_tvtoto, then it will still overflow on conversion to int, right?

bukka · 2025-11-29T10:34:51Z

main/network.c

 	gettimeofday(limit_time, NULL);
+	const double timeoutmax = (double) PHP_TIMEOUT_ULL_MAX / 1000000.0;
+
+	if (limit_time->tv_sec > (timeoutmax - timeout->tv_sec)) {


Shouldn't that be be >=?

yes it should

bukka · 2025-11-29T10:43:24Z

main/network.c

-		php_network_set_limit_time(&limit_time, &working_timeout);
+		if (UNEXPECTED(php_network_set_limit_time(&limit_time, &working_timeout) == FAILURE)) {
+			php_network_freeaddresses(psal);
+			zend_value_error("timeout value overflow");


I'm not sure about throwing error here even though it already triggers normal errors in some cases.

Why is this even checked here and not in the actual function as it has been case before?

Girgias · 2025-11-29T15:11:32Z

main/network.c

+	if (limit_time->tv_sec >= (timeoutmax - timeout->tv_sec)) {
+		zend_value_error("timeout value overflow");
+		return FAILURE;
+	}


I would rather have this be an assertion and make it the responsibility of the calling code to ensure this is respected. So move the check to ftp_connect(), especially as you get better error messages and you can pin point to the exact argument is causing an issue.

ok done, note that I purposely kept separated the check for <= 0 and the new one since it s stable branch.

Girgias · 2025-11-29T15:11:57Z

main/network.c

 #if HAVE_GETTIMEOFDAY
-		php_network_set_limit_time(&limit_time, &working_timeout);
+		if (UNEXPECTED(php_network_set_limit_time(&limit_time, &working_timeout) == FAILURE)) {
+			php_network_freeaddresses(psal);
+			return -1;
+		}
 #endif


Girgias · 2025-11-29T15:12:06Z

main/network.c

 #if HAVE_GETTIMEOFDAY
-		php_network_set_limit_time(&limit_time, &working_timeout);
+		if (UNEXPECTED(php_network_set_limit_time(&limit_time, &working_timeout) == FAILURE)) {
+			error = ERANGE;
+			ret = -1;
+			goto ok;
+		}
 #endif


Girgias · 2025-11-29T17:41:26Z

ext/ftp/tests/gh20601.phpt

+--SKIPIF--
+<?php
+if (PHP_INT_SIZE != 8) die("skip: 64-bit only");
+if (PHP_OS_FAMILY === 'Windows') die("skip not for windows");
+?>


This an issue that cannot appear on 32 bit nor Windows?

yes this test "failed" for both earlier.

iluuu1994 · 2025-12-02T16:11:29Z

@devnexen Can you please look at the FTP failure in https://github.com/php/php-src/actions/runs/19843698497/job/56857155631? It looks related to this change.

devnexen · 2025-12-02T16:13:50Z

ok will have a look tonight.

iluuu1994 · 2025-12-02T18:45:19Z

@devnexen @ndossche also reminded me that this needs to be cherry-picked for 8.5.1, given we've just tagged RC1. /cc @php/release-managers-85

bukka · 2025-12-02T19:04:32Z

8.3.29RC1 also tagged and win32 NTS is failing there as well so we will likely need another RC (ideally next week as @edorian does it already for 8.5.1 due to another fix) there as well. CC @ERICMAN

There's still time for 8.4 as Saki is delaying tag till tomorrow.

the timeout needed to be unsigned.

the timeout needed to be unsigned. close GH-20634

* PHP-8.3: Fix GH-20603 issue on windows 32 bits.

* PHP-8.4: Fix GH-20603 issue on windows 32 bits.

* PHP-8.5: Fix GH-20603 issue on windows 32 bits.

devnexen · 2025-12-03T20:24:14Z

@devnexen @ndossche also reminded me that this needs to be cherry-picked for 8.5.1, given we've just tagged RC1. /cc @php/release-managers-85

Ok fix committed. Cheers.

the timeout needed to be unsigned. close GH-20634

the timeout needed to be unsigned. close GH-20634 (cherry picked from commit ff51ac1)

tml · 2026-01-02T21:38:02Z

ext/ftp/php_ftp.c

 		RETURN_THROWS();
 	}

+	const zend_long timeoutmax = (zend_long)((double) PHP_TIMEOUT_ULL_MAX / 1000000.0);


This is causing an integer overflow on 32-bit PHP, resulting in

Fatal error: Uncaught ValueError: ftp_connect(): Argument #3 ($timeout) must be less than -2147483648

been fixed for the next release.

iluuu1994 · 2026-01-02T22:10:02Z

Shouldn't this have been cherry-picked for each release?

ndossche · 2026-01-02T22:25:03Z

Since these are integer UB overflow fixes, the real world impact of these bugs are very small. So wouldn't it make more sense to only do these in master in the future to avoid risking regressions? If necessary, they can always be cherry-picked later.

iluuu1994 · 2026-01-02T22:49:47Z

It looks like multiple things went wrong here.

As @ndossche mentioned, we might want to be more cautious when pushing fixes for fuzzing issues (i.e. push them to master only). They are frequently cosmetic and not worth the risk of causing real-world issues though a faulty fix.
The issue was found and fixed in time for the release, but not actually deployed. @bukka pinged some people, but only mentioned @SakiTakamachi by-name, and misspelled @ericmann's usename. Optimally, all relevant RM teams should be pinged through the team's handle.
Even the RM team that was pinged (@php/release-managers-85) failed to cherry-pick the fix. Nobody even acknowledged the ping.
Ultimately, the author of the faulty commit (@devnexen in this case) should ensure the fix actually lands.

My intention is not to blame anybody here (nobody is exempt from causing bugs). But maybe we can put more guards in place to avoid this in the future. The RC-period is quite pointless if issues that are found aren't actually fixed.

Another issue is that deploying such a fix requires the intervention of 3 RM teams. That doesn't exactly increase the chance of everything going smoothly. Maybe it's acceptable for one RM team to cherry-pick the fix for every release? I'm not an RM and don't plan to be, so feel free to suggest something more sensible.

github-actions bot added the Extension: ftp label Nov 27, 2025

Fix phpGH-20601: ftp_connect() timeout argument overflow.

05342f4

devnexen force-pushed the gh20601 branch from 2954678 to 05342f4 Compare November 27, 2025 22:24

devnexen marked this pull request as ready for review November 28, 2025 20:26

devnexen requested a review from bukka as a code owner November 28, 2025 20:26

devnexen linked an issue Nov 28, 2025 that may be closed by this pull request

integer overflow in network.c #20601

Closed

bukka reviewed Nov 29, 2025

View reviewed changes

bukka added the Category: Network label Nov 29, 2025

feedback

37dd21c

Girgias requested changes Nov 29, 2025

View reviewed changes

feedback 2

b1090b4

Girgias reviewed Nov 29, 2025

View reviewed changes

Girgias approved these changes Nov 29, 2025

View reviewed changes

devnexen closed this in 4312a44 Nov 29, 2025

devnexen added a commit to devnexen/php-src that referenced this pull request Dec 2, 2025

Fix phpGH-20603 issue on windows 32 bits.

472a774

the timeout needed to be unsigned.

devnexen added a commit that referenced this pull request Dec 3, 2025

Fix GH-20603 issue on windows 32 bits.

ff51ac1

the timeout needed to be unsigned. close GH-20634

devnexen added a commit that referenced this pull request Dec 3, 2025

Merge branch 'PHP-8.3' into PHP-8.4

51f4450

* PHP-8.3: Fix GH-20603 issue on windows 32 bits.

devnexen added a commit that referenced this pull request Dec 3, 2025

Merge branch 'PHP-8.4' into PHP-8.5

f90d1f7

* PHP-8.4: Fix GH-20603 issue on windows 32 bits.

devnexen added a commit that referenced this pull request Dec 3, 2025

Merge branch 'PHP-8.5'

50abef9

* PHP-8.5: Fix GH-20603 issue on windows 32 bits.

edorian pushed a commit that referenced this pull request Dec 9, 2025

Fix GH-20603 issue on windows 32 bits.

671f95e

the timeout needed to be unsigned. close GH-20634

bukka pushed a commit that referenced this pull request Dec 12, 2025

Fix GH-20603 issue on windows 32 bits.

08bd490

the timeout needed to be unsigned. close GH-20634 (cherry picked from commit ff51ac1)

bukka pushed a commit that referenced this pull request Dec 12, 2025

Fix GH-20603 issue on windows 32 bits.

cbb854f

the timeout needed to be unsigned. close GH-20634 (cherry picked from commit ff51ac1)

tml reviewed Jan 2, 2026

View reviewed changes

iluuu1994 mentioned this pull request Jan 3, 2026

Integer overflow on 32bit PHP caused by GH-20601 #20823

Closed

Fix GH-20601: ftp_connect() timeout argument overflow. #20603

Fix GH-20601: ftp_connect() timeout argument overflow. #20603

Conversation

devnexen commented Nov 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka Nov 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devnexen Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iluuu1994 commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devnexen commented Dec 2, 2025

Uh oh!

iluuu1994 commented Dec 2, 2025

Uh oh!

bukka commented Dec 2, 2025

Uh oh!

devnexen commented Dec 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iluuu1994 commented Jan 2, 2026

Uh oh!

ndossche commented Jan 2, 2026

Uh oh!

iluuu1994 commented Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bukka Nov 30, 2025 •

edited

Loading

devnexen Nov 29, 2025 •

edited

Loading

iluuu1994 commented Dec 2, 2025 •

edited

Loading

iluuu1994 commented Jan 2, 2026 •

edited

Loading