ext/standard warning to error promotions #5814

Girgias · 2020-07-06T16:20:24Z

Does should be rather uncontroversial I think

kocsismate · 2020-07-06T16:39:17Z

ext/standard/array.c

 	HashTable *ht;

 	ZEND_PARSE_PARAMETERS_START(2, 2)
-		Z_PARAM_ZVAL(key)
+		Z_PARAM_STR_OR_LONG(string_key, int_key)


array_key_exists() is not a good target for this. Please see Nikita's comment here: #4887

isset does a usual int silent type cast, string array keys are the only weird thing where "03" will remain "03" but "3" would become the integer index 3 thus I think the behaviour would be consistent but can double check

ext/standard/dns.c

ext/standard/streamsfuncs.c

kocsismate · 2020-08-01T21:13:26Z

ext/standard/dns.c

 		/* name too long, protect from CVE-2015-0235 */
-		php_error_docref(NULL, E_WARNING, "Host name is too long, the limit is %d characters", MAXFQDNLEN);
-		RETURN_STRINGL(hostname, hostname_len);
+		zend_argument_value_error(1, "must be less than %d characters", MAXFQDNLEN);


what about cannot be longer than %d characters? As far as I see the condition, less than MAXFQDNLEN isn't true, because being equal is also okay.

kocsismate · 2020-08-01T21:15:11Z

ext/standard/dns.c

-			php_error_docref(NULL, E_WARNING,
-				"Numeric DNS record type must be between 1 and 65535, '" ZEND_LONG_FMT "' given", type_param);
-			RETURN_FALSE;
+			zend_argument_value_error(2, "must be between 1 and 65535 when Argument #5 ($raw) is true");


Suggested change

zend_argument_value_error(2, "must be between 1 and 65535 when Argument #5 ($raw) is true");

zend_argument_value_error(2, "must be between 1 and 65535 when argument #5 ($raw) is true");

Girgias · 2020-08-01T23:10:17Z

I dropped the array_key_exist one while addressing the reviews

ext/standard/dns.c

nikic · 2020-08-06T09:39:02Z

ext/standard/dns.c

-			php_error_docref(NULL, E_WARNING, "Type '%s' not supported", rectype);
-			RETURN_FALSE;
+			zend_argument_value_error(2, "must be one of \"A\", \"NS\", \"MX\", \"PTR\", \"ANY\", \"SAO\", \"CAA\", "
+				"\"TXT\", \"CNAME\", \"AAAA\", \"SRV\", \"NAPTR\", or \"A6\"");


I don't think we should list all possibilities here, as this may get extended and depends on environment (e.g. I don't think CAA works on Windows). Just say valid DNS record type or something like that.

nikic · 2020-08-06T09:40:15Z

ext/standard/dns.c

-			php_error_docref(NULL, E_WARNING, "Type '" ZEND_LONG_FMT "' not supported", type_param);
-			RETURN_FALSE;
+			zend_argument_value_error(2, "must be one of DNS_A, DNS_CNAME, DNS_HINFO, DNS_CAA, DNS_MX, "
+				"DNS_NS, DNS_PTR, DNS_SOA, DNS_TXT, DNS_AAAA, DNS_SRV, DNS_NAPTR, DNS_A6, DNS_ALL, or DNS_ANY");


I'd only mention DNS_* here (we do this in a few places).

nikic · 2020-08-06T09:41:33Z

ext/standard/iptc.c

@@ -193,8 +193,8 @@ PHP_FUNCTION(iptcembed)
 	}

 	if (iptcdata_len >= SIZE_MAX - sizeof(psheader) - 1025) {
-		php_error_docref(NULL, E_WARNING, "IPTC data too large");
-		RETURN_FALSE;
+		zend_argument_value_error(1, "must be less than %d characters", SIZE_MAX - sizeof(psheader) - 1025);


I'd go for is too large here, the exact value doesn't add value as it's impossible to reach with sensible code.

nikic · 2020-08-06T09:42:56Z

ext/standard/tests/network/bug68925.phpt

+?>
+--EXPECT--
+gethostbyname(): Argument #1 ($hostname) cannot be longer than 255 characters
+gethostbynamel(): Argument #1 ($hostname) cannot be longer than 255 characters


These should be %d. Though I'm not really sure if we actually want to promote these. The number differs by platform and is something one could hit with reasonable code. As gethostbyname() is already fallible, it would currently be a necessary false check.

nikic · 2020-08-06T09:43:42Z

ext/standard/user_filters.c

-					"Err, filter \"%s\" is not in the user-filter map, but somehow the user-filter-factory was invoked for it!?", filtername);
+			zend_throw_error(NULL, "Filter \"%s\" is not in the user-filter map, "
+				"but user-filter-factory was invoked for it."
+				"This is a bug, please report it at https://bugs.php.net", filtername);


We should just ZEND_ASSERT(fdat) here.

Girgias · 2020-08-06T12:41:00Z

ext/standard/dns.c

-		php_error_docref(NULL, E_WARNING, "Host name is too long, the limit is %d characters", MAXFQDNLEN);
-		RETURN_STRINGL(hostname, hostname_len);
+		zend_argument_value_error(1, "cannot be longer than %d characters", MAXFQDNLEN);
+		RETURN_THROWS();


So compared to gethostbynamel(), gethostbyname() does not return false should I change this to return false if I revert the ValueError to warning?

carusogabriel added the Warning promotion label Jul 6, 2020

kocsismate reviewed Jul 6, 2020

View reviewed changes

Girgias force-pushed the standard-warning-to-error branch 2 times, most recently from 599506c to 2ead2f6 Compare July 7, 2020 18:26

kocsismate reviewed Aug 1, 2020

View reviewed changes

Girgias force-pushed the standard-warning-to-error branch from 2ead2f6 to f9bd96a Compare August 1, 2020 23:09

nikic reviewed Aug 6, 2020

View reviewed changes

Girgias commented Aug 6, 2020

View reviewed changes

Girgias force-pushed the standard-warning-to-error branch from f9bd96a to e477a5d Compare August 6, 2020 12:43

Girgias mentioned this pull request Aug 13, 2020

Warning to Error (TypeError/ValueError) in extensions php/php-tasks#18

Closed

71 tasks

Girgias added 12 commits September 2, 2020 17:36

Warning to error in parse_ini_file()

1ddddb6

DNS

37629a7

EXEC

881adfd

FileStats

38f7146

FTOK

3b57aea

IPCT

4dc51f6

Stream funcs

5ed963d

User filter

301150a

DNS review

06d874f

DNS reviews second round

9915265

IPTC reviews

d937df7

Use assertion for user filters

c0aea24

Girgias force-pushed the standard-warning-to-error branch from e477a5d to c0aea24 Compare September 2, 2020 15:39

Girgias added 3 commits September 2, 2020 18:04

Revert chown test file change

8dd1026

Revert changed to gethostbyname functions

ca509e5

Fix warning message in test after partial revert

438f411

php-pulls closed this in 4a438b4 Sep 3, 2020

Girgias deleted the standard-warning-to-error branch September 3, 2020 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ext/standard warning to error promotions #5814

ext/standard warning to error promotions #5814

Girgias commented Jul 6, 2020

kocsismate Jul 6, 2020

Girgias Jul 6, 2020

kocsismate Aug 1, 2020

kocsismate Aug 1, 2020

Girgias commented Aug 1, 2020

nikic Aug 6, 2020

nikic Aug 6, 2020

nikic Aug 6, 2020

nikic Aug 6, 2020

nikic Aug 6, 2020

Girgias Aug 6, 2020

	zend_argument_value_error(2, "must be between 1 and 65535 when Argument #5 ($raw) is true");
	zend_argument_value_error(2, "must be between 1 and 65535 when argument #5 ($raw) is true");

ext/standard warning to error promotions #5814

ext/standard warning to error promotions #5814

Conversation

Girgias commented Jul 6, 2020

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

Girgias commented Aug 1, 2020

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