ext/sockets: socket_strerror follow-up on GH-16267 fix.

[devnexen]

boundaries should be INT_MIN <= val < INT_MAX in fact.

[cmb69]

Suggested change

	zend_argument_value_error(1, "must be greater than %d and lower than %d", INT_MIN, INT_MAX);
	zend_argument_value_error(1, "must be greater than %d and less than %d", INT_MIN, INT_MAX);

[cmb69]

This has basically the same problem (32bit systems) as the bug we're trying to fix; the number 2147483648 overflows there.

Did you try:

 ext/sockets/sockets.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/sockets/sockets.c b/ext/sockets/sockets.c
index 08b8d8406d..5cc66f2d8f 100644
--- a/ext/sockets/sockets.c
+++ b/ext/sockets/sockets.c
@@ -370,7 +370,7 @@ char *sockets_strerror(int error) /* {{{ */
 
 #ifndef PHP_WIN32
 	if (error < -10000) {
-		error = -error - 10000;
+		error = (int)(unsigned int) error - 10000;
 
 #ifdef HAVE_HSTRERROR
 		buf = hstrerror(error);

PS: no need to try: suggestion is nonsense. Disregard.

[Girgias]

The standard wording is:

Suggested change

	zend_argument_value_error(1, "must be greater than %d and lower than %d", INT_MIN, INT_MAX);
	zend_argument_value_error(1, "must be between %d and %d", INT_MIN, INT_MAX);

[cmb69]

Yeah, but in this case INT_MIN is no longer supported, so should be:

Suggested change

	zend_argument_value_error(1, "must be greater than %d and lower than %d", INT_MIN, INT_MAX);
	zend_argument_value_error(1, "must be between %d and %d", INT_MIN + 1, INT_MAX);

But I'm still not convinced that we need to constrain the allowed range.

[Girgias]

I'm just pointing out the wording :)

[Girgias]

Suggested change

	socket_strerror(): Argument #1 ($error_code) must be between %s and %d
	socket_strerror(): Argument #1 ($error_code) must be between %s and %d
	socket_strerror(): Argument #1 ($error_code) must be between %d and %d
	socket_strerror(): Argument #1 ($error_code) must be between %d and %d

[devnexen]

sorry does not work for me, I guess because it s a negative.

[Girgias]

Right, use %i

[cmb69]

The fix looks okay to me, but why not fix the root cause? My previous suggestion was nonsense, but we could special case INT_MIN:

 ext/sockets/sockets.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ext/sockets/sockets.c b/ext/sockets/sockets.c
index 48221479a7..eab26737e6 100644
--- a/ext/sockets/sockets.c
+++ b/ext/sockets/sockets.c
@@ -370,7 +370,11 @@ char *sockets_strerror(int error) /* {{{ */
 
 #ifndef PHP_WIN32
 	if (error < -10000) {
-		error = -error - 10000;
+		if (error = INT_MIN) {
+			error = 2147473648;
+		} else {
+			error = -error - 10000;
+		}
 
 #ifdef HAVE_HSTRERROR
 		buf = hstrerror(error);

That may not matter in practice, in which case we can go with this fix, but I would add an assertion (basically a precondition) in sockets_strerror() that error > INT_MIN.

[devnexen]

It s mainly because socket_strerror is the only call using user supplied error code for sockets_strerror. But yes I can put an assert sure.

[cmb69]

If we support INT_MIN, we can revert this change.

[cmb69]

Superfluous assertion; since error is of type int it is always >= INT_MIN.

Other than that the patch looks good to me.

[devnexen]

closed with 3bea6a2.