Fix GH-14780: p(f)sockopen overflow on timeout argument. #14785

devnexen · 2024-07-03T11:41:28Z

No description provided.

nielsdos · 2024-07-21T20:29:45Z

One tricky thing here is Windows: time_t is 32-bit on our x86 builds and 64-bit on x64 builds. But LONG_MAX is always the signed 32-bit limit even on Windows x64.
Also this whole issue is giving me a déjà vu, here's how I fixed it last year in another part of the codebase: #11183

nielsdos · 2024-07-22T14:09:11Z

ext/standard/fsock.c

@@ -73,14 +73,27 @@ static void php_fsockopen_stream(INTERNAL_FUNCTION_PARAMETERS, int persistent)
 	}

 	/* prepare the timeout value for use */
+	if (timeout < 0.0 || timeout > (double) PHP_TIMEOUT_ULL_MAX / 1000000.0) {


This should still be the following to catch NAN and INF

Suggested change

if (timeout < 0.0 || timeout > (double) PHP_TIMEOUT_ULL_MAX / 1000000.0) {

if (!(timeout >= 0.0 && timeout <= (double) PHP_TIMEOUT_ULL_MAX / 1000000.0)) {

Sorry for the confusion, this is in fact something I missed in my original patch. So I opened a PR to fix that: #15061

However, I'm not sure this is the best approach here after all.
My original patch didn't use a range check because some people use -1 to mean a practically infinite timeout, and I think we should keep that behaviour here as well. WDYT?

nielsdos · 2024-07-22T14:09:35Z

ext/standard/tests/streams/gh14780.phpt

+	pfsockopen('udp://127.0.0.1', '63844', $code, $err, (PHP_INT_MIN/100000)-1);
+} catch (\ValueError $e) {
+	echo $e->getMessage();
+}


Please add a closing ?>

nielsdos · 2024-07-22T14:09:44Z

ext/standard/tests/streams/gh14780.phpt

+<?php
+$code = null;
+$err = null;
+try {


Please also test NAN and INF.

nielsdos · 2024-07-22T17:51:49Z

ext/standard/fsock.c

@@ -73,14 +73,27 @@ static void php_fsockopen_stream(INTERNAL_FUNCTION_PARAMETERS, int persistent)
 	}

 	/* prepare the timeout value for use */
+	if (timeout != -1.0 && (timeout >= 0.0 || timeout <= (double) PHP_TIMEOUT_ULL_MAX / 1000000.0)) {


Suggested change

if (timeout != -1.0 && (timeout >= 0.0 || timeout <= (double) PHP_TIMEOUT_ULL_MAX / 1000000.0)) {

if (timeout != -1.0 && !(timeout >= 0.0 && timeout <= (double) PHP_TIMEOUT_ULL_MAX / 1000000.0)) {

The error message also isn't technically correct anymore

Also, please check if -1 behaves correctly now.

I added a test

Sorry, I missed that in the test file

pierrejoye · 2024-07-22T18:00:25Z

On Mon, Jul 22, 2024, 3:30 AM Niels Dossche ***@***.***> wrote: One tricky thing here is Windows: time_t is 32-bit on our x86 builds and 64-bit on x64 builds. But LONG_MAX is always the signed 32-bit limit even on Windows x64. Also this whole issue is giving me a déjà vu, here's how I fixed it last year in another part of the codebase: #11183 <#11183> — Reply to this email directly, view it on GitHub <#14785 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACE6KGZMKWA6SLW2GBH6PLZNQK5LAVCNFSM6AAAAABKJMSN7SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBRG43DKMJWGM> . You are receiving this because you are subscribed to this thread.

if I am not mistaken, php does not support 32bit on windows anymore. Also we introduce c9x int declaration for portability purposes a while back. I am not sure why long max on windows (within php's build) remains 32bit :) Message ID: ***@***.***>

…

nielsdos · 2024-07-22T18:07:29Z

if I am not mistaken, php does not support 32bit on windows anymore.

I can see downloads for 32-bit x86 Windows builds on https://windows.php.net/download/, so it's still supported.

Also we introduce c9x int declaration for portability purposes a while back. I am not sure why long max on windows (within php's build) remains 32bit :) Message ID: @.***>

LONG_MAX is in limits.h, not defined by PHP. See this msvc snippet that shows it's 32 bits on x64 msvc compiler: https://godbolt.org/z/bvjrdT8TM

nielsdos · 2024-07-22T18:08:47Z

ext/standard/fsock.c

+			efree(hashkey);
+		}
+
+		zend_argument_value_error(6, "must be between -1 and " ZEND_ULONG_FMT, (PHP_TIMEOUT_ULL_MAX / 1000000.0));


This message still isn't technically right, this should be:

Suggested change

zend_argument_value_error(6, "must be between -1 and " ZEND_ULONG_FMT, (PHP_TIMEOUT_ULL_MAX / 1000000.0));

zend_argument_value_error(6, "must be -1 or between 0 and " ZEND_ULONG_FMT, (PHP_TIMEOUT_ULL_MAX / 1000000.0));

In my patch I made finite but out-of-range values equal to an infinite timeout to preserve BC, whether we need that here I don't know. Part of me thinks your approach makes sense and part of me thinks we should be consistent.

pierrejoye · 2024-07-22T18:56:02Z

On Tue, Jul 23, 2024, 1:07 AM Niels Dossche ***@***.***> wrote: if I am not mistaken, php does not support 32bit on windows anymore. I can see downloads for 32-bit x86 Windows builds on https://windows.php.net/download/, so it's still supported. Also we introduce c9x int declaration for portability purposes a while back. I am not sure why long max on windows (within php's build) remains 32bit :) Message ID: *@*.***> LONG_MAX is in limits.h, not defined by PHP. See this msvc snippet that shows it's 32 bits on x64 msvc compiler: https://godbolt.org/z/bvjrdT8TM

right, however we did ensure that int64_t (and other c99 types) are always available, in all platform. It is defined in the CS, if that helps ( https://github.com/php/php-src/blob/b537f013537da981804282d1fe50f96a0cc325ee/CODING_STANDARDS.md?plain=1#L14 ). I checked the define and co, we still have too many places in standard code relying on custom checks and define :-/. cmb or Weltling know better and may provide more updated insights about this. I am also surprised we still provide 32bits build given the limitation, and more importantly not being supported anymore by msft (windows releases, 32bit mode still works). But that's off topic :)

…

— Reply to this email directly, view it on GitHub <#14785 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACE6KBTTIWZKJ7NDTQYDO3ZNVC7RAVCNFSM6AAAAABKJMSN7SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBTGUZDQNBQGI> . You are receiving this because you commented.Message ID: ***@***.***>

nielsdos

Technically this seems fine and fixes the UB, whether it's better to allow out of range values but treat them as infinite, I'm not sure. In any case this is an improvement as-is.

nielsdos · 2024-07-22T21:44:56Z

I'm a bit concerned that some people may be passing PHP_INT_MAX to timeout without thinking though.

close GH-15071

github-actions bot added the Extension: standard label Jul 3, 2024

devnexen force-pushed the gh14780 branch from 77acf3f to 6f51b1e Compare July 3, 2024 11:54

devnexen marked this pull request as ready for review July 3, 2024 17:50

devnexen requested a review from bukka as a code owner July 3, 2024 17:50

devnexen force-pushed the gh14780 branch from 6f51b1e to bc4f2b2 Compare July 6, 2024 17:02

devnexen force-pushed the gh14780 branch 2 times, most recently from a72cfed to a0a97a7 Compare July 22, 2024 13:56

nielsdos reviewed Jul 22, 2024

View reviewed changes

devnexen force-pushed the gh14780 branch from a0a97a7 to bb523c0 Compare July 22, 2024 17:34

nielsdos reviewed Jul 22, 2024

View reviewed changes

devnexen force-pushed the gh14780 branch from bb523c0 to 78db024 Compare July 22, 2024 18:00

nielsdos reviewed Jul 22, 2024

View reviewed changes

Fix phpGH-14780: p(f)sockopen overflow on timeout argument.

f4ae575

devnexen force-pushed the gh14780 branch from 78db024 to f4ae575 Compare July 22, 2024 20:28

nielsdos approved these changes Jul 22, 2024

View reviewed changes

devnexen closed this in ba909d7 Jul 22, 2024

devnexen added a commit to devnexen/php-src that referenced this pull request Jul 22, 2024

fix phpGH-14785 pedantic error

e34f543

devnexen added a commit that referenced this pull request Jul 22, 2024

fix GH-14785 pedantic error

350c10d

close GH-15071

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GH-14780: p(f)sockopen overflow on timeout argument. #14785

Fix GH-14780: p(f)sockopen overflow on timeout argument. #14785

devnexen commented Jul 3, 2024

nielsdos commented Jul 21, 2024

nielsdos Jul 22, 2024

nielsdos Jul 22, 2024

nielsdos Jul 22, 2024

nielsdos Jul 22, 2024

nielsdos Jul 22, 2024

devnexen Jul 22, 2024

nielsdos Jul 22, 2024

pierrejoye commented Jul 22, 2024 via email

nielsdos commented Jul 22, 2024

nielsdos Jul 22, 2024

nielsdos Jul 22, 2024

pierrejoye commented Jul 22, 2024 via email

nielsdos left a comment

nielsdos commented Jul 22, 2024

	if (timeout < 0.0 \|\| timeout > (double) PHP_TIMEOUT_ULL_MAX / 1000000.0) {
	if (!(timeout >= 0.0 && timeout <= (double) PHP_TIMEOUT_ULL_MAX / 1000000.0)) {

	if (timeout != -1.0 && (timeout >= 0.0 \|\| timeout <= (double) PHP_TIMEOUT_ULL_MAX / 1000000.0)) {
	if (timeout != -1.0 && !(timeout >= 0.0 && timeout <= (double) PHP_TIMEOUT_ULL_MAX / 1000000.0)) {

	zend_argument_value_error(6, "must be between -1 and " ZEND_ULONG_FMT, (PHP_TIMEOUT_ULL_MAX / 1000000.0));
	zend_argument_value_error(6, "must be -1 or between 0 and " ZEND_ULONG_FMT, (PHP_TIMEOUT_ULL_MAX / 1000000.0));

Fix GH-14780: p(f)sockopen overflow on timeout argument. #14785

Fix GH-14780: p(f)sockopen overflow on timeout argument. #14785

Conversation

devnexen commented Jul 3, 2024

nielsdos commented Jul 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierrejoye commented Jul 22, 2024 via email

nielsdos commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierrejoye commented Jul 22, 2024 via email

nielsdos left a comment

Choose a reason for hiding this comment

nielsdos commented Jul 22, 2024