Improve argument error messages in ext/sodium #5197

kocsismate · 2020-02-22T10:08:40Z

There are some notes:

Error messages starting with ...() expects argument ... are only usable for positive sentences, but not for negative ones (e.g. message too long for a single key). Usually, conversion isn't much of a problem (usually, the message even becomes more clear this way), but it seems to me that error messages starting with Argument ... passed to would be more generally usable.
Currently an Error exceptions must be derived from Error notice is generated when zend_argument_error() is called, because Sodium uses the SodiumException that is of course incompatible with Errors. I'm not sure what is the best course of action, maybe adding a zend_argument_exception() that can throw Exceptions?
Another question is whether we'd like to display the provided values in the error messages? Sometimes it would be trivial to do, but sometimes it's not (e.g. in messages in connection with length)

Girgias · 2020-02-22T22:24:35Z

Wouldn't it maybe be better to have a sort of argument error messages formatter function? But maybe adding a dedicated function which can throw exceptions like that is better, or maybe just convert them to ValueErrors directly?

nikic · 2020-02-23T17:13:49Z

* Currently an `Error exceptions must be derived from Error` notice is generated when `zend_argument_error()` is called, because Sodium uses the `SodiumException` that is of course incompatible with `Error`s. I'm not sure what is the best course of action, maybe adding a `zend_argument_exception()` that can throw `Exception`s?

I think you can just lift that restriction in zend_throw_error. There's no particular reason why exceptions can't be used there.

* Another question is whether we'd like to display the provided values in the error messages? Sometimes it would be trivial to do, but sometimes it's not (e.g. in messages in connection with length)

I'd say, as a matter of general policy: No, values should not be included in error messages. People leaving display_errors=1 in production is unfortunately not as rare as it should be, and displaying values could leak sensitive information. Part of the reason why arguments in exception backtraces can be disabled since PHP 7.4.

ext/sodium/libsodium.c

nikic · 2020-02-23T17:17:33Z

Uh, is this extension not getting tested on azure/travis?

ext/sodium/libsodium.c

kocsismate · 2020-02-23T20:54:12Z

I'd say, as a matter of general policy: No, values should not be included in error messages. People leaving display_errors=1 in production is unfortunately not as rare as it should be, and displaying values could leak sensitive information.

Oh, I see now, thanks for the info. I haven't thought about this dimension of the question.

Uh, is this extension not getting tested on azure/travis?

Yeah, I was also surprised.

nikic · 2020-02-24T11:46:11Z

ext/sodium/libsodium.c

@@ -3156,23 +3037,23 @@ PHP_FUNCTION(sodium_crypto_kdf_derive_from_key)
 		RETURN_THROWS();
 	}
 	if (subkey_len < crypto_kdf_BYTES_MIN) {
-		zend_throw_exception(sodium_exception_ce, "subkey cannot be smaller than SODIUM_CRYPTO_KDF_BYTES_MIN", 0);
+		zend_argument_error(sodium_exception_ce, 1, "to be more than or equal to SODIUM_CRYPTO_KDF_BYTES_MIN");


more than -> greater than

Also, it's possible to formulate this as to be at least BYTES_MIN and to be at most BYTES_MAX, which would be more compact.

So do you think we should use at least and at most instead of greater than or equal to and less than or equal to in general? I'll update my PRs then

I think so. It's more concise and, at least to me, just as clear. I could see an argument being made that "greater than or equal" is more obvious though.

Weell, it does depend on the case. "must be at least 10 bytes long" is definitely better than "must be greater than or equal to 10 bytes long", but I'm not so sure about "must be at least 10" and "must be greater than or equal to 10". The case of "most be at least 0" specifically sounds outright weird (but can be "must be non-negative" or "must not be negative").

So overall: Not sure :)

@Girgias Do you maybe have any other suggestion or preference? On the one hand, I like the greater than or equal to form because it's obvious and it's the most consistent with greater than. On the other hand, at least is surely a more concise way to say the same. Additionally, we have positive/negative/non-negative etc. which - at least for me - need a little bit more processing time. Furthermore, according to what I remember, they are not ubiquitous everywhere (see my comment more below. Or it is just a myth?).

That said, my preference is to use greater/less than or equal to for pure minimum/maximum values, and at least/at most for minimum/maximum length etc.. It's not a strong opinion though, so I welcome any opposition. :)

I would prefer the explicit greater/less than or equal to and at least/at most as it is straight forward, positivity and negativity is where French and English doesn't really agree on, famous example is how ℕ is ambiguous as French mathematician will include 0 in it whereas British won't for the most part (as it's meant to represent positive numbers).
This is mainly due to how in French "greater" and "less" are inclusive.

It also seems Japanese has this ambiguity (they literally have a brand called +-0 https://www.plusminuszero.jp/)

nikic · 2020-02-24T11:47:59Z

ext/sodium/libsodium.c

-	if (blocksize > SIZE_MAX) {
-		zend_throw_exception(sodium_exception_ce, "block size is too large", 0);
+	if (blocksize <= 0 || blocksize > SIZE_MAX) {
+		zend_argument_error(sodium_exception_ce, 2, "to be greater than 0");


As 0 is a particular common case: This can also be to be positive. Or to be non-negative for the >= case.

I'd prefer to get rid of positive/negative from messages because I feel that the exact number is easier to understand - but maybe it's just me. :)

Furthermore, as far as I remember the position of 0 is not clear everywhere in the world. I found a comment about it: https://math.stackexchange.com/questions/26705/is-zero-positive-or-negative. But I might be wrong here as well.

nikic · 2020-02-24T11:49:02Z

ext/sodium/libsodium.c

 		RETURN_THROWS();
 	}
 	if (blocksize > SIZE_MAX) {
-		zend_throw_exception(sodium_exception_ce, "block size is too large", 0);
+		zend_argument_error(sodium_exception_ce, 2, "to be less than or equal to %d", SIZE_MAX);


Per your comment above, this shouldn't include SIZE_MAX.

I fixed this, but the new message is a bit weird too. For now, it's .. to be less than or equal to the maximum allowed value.

nikic · 2020-02-24T11:50:06Z

ext/sodium/libsodium.c

 		RETURN_THROWS();
 	}
 	if (msg_len > crypto_secretstream_xchacha20poly1305_MESSAGEBYTES_MAX ||
 		msg_len > SIZE_MAX - crypto_secretstream_xchacha20poly1305_ABYTES) {
-		zend_throw_exception(sodium_exception_ce, "message cannot be larger than SODIUM_CRYPTO_SECRETSTREAM_XCHACHA20POLY1305_MESSAGEBYTES_MAX bytes", 0);
+		zend_argument_error(sodium_exception_ce, 2, "to be shorter than or equal to SODIUM_CRYPTO_SECRETSTREAM_XCHACHA20POLY1305_MESSAGEBYTES_MAX bytes");


Especially for cases like this, I think that phrasing this in the to be at most xxx bytes long style is more readable. "shorter than or equal" is pretty odd.

Fixed, it was very odd indeed. :)

nikic · 2020-02-26T20:05:24Z

ext/sodium/libsodium.c

+		RETURN_THROWS();
+	}
+	if (hash_len >= 0xffffffff) {
+		zend_argument_error(sodium_exception_ce, 1, "must be less than the maximum allowed value");


Now that we are no longer restricted to positive messages, we can also use something like XXX is too large or YYY is too long here. I think that might be better for cases like this where we have a non-explicit upper bound.

I absolutely agree! Actually, I've already changed a similar message to "XXX is too large" in my other PR, so I'll review these cases here as well shortly.

nikic · 2020-02-26T20:06:40Z

ext/sodium/libsodium.c

@@ -1792,6 +1736,7 @@ PHP_FUNCTION(sodium_crypto_pwhash_str_needs_rehash)

 	if (zend_parse_parameters(ZEND_NUM_ARGS(), "sll",
 									&hash_str, &hash_str_len, &opslimit, &memlimit) == FAILURE) {
+		// TODO: what's this error message here?


Should just be removed, I believe.

nikic · 2020-02-26T20:08:07Z

ext/sodium/libsodium.c

 		RETURN_THROWS();
 	}
 	sodium_separate_string(state_zv);
 	state = (unsigned char *) Z_STRVAL(*state_zv);
 	state_len = Z_STRLEN(*state_zv);
 	if (state_len != sizeof (crypto_secretstream_xchacha20poly1305_state)) {
-		zend_throw_exception(sodium_exception_ce, "incorrect state length", 0);
+		zend_argument_error(sodium_exception_ce, 1, "to have a correct length");


Probably this should use negative form now.

I think "... must have a correct length" should do it as well.

But if you have a better suggestion, I'm definitely open for a change.

kocsismate · 2020-02-27T18:38:57Z

ext/sodium/libsodium.c

@@ -558,7 +558,7 @@ PHP_FUNCTION(sodium_add)
 	val = (unsigned char *) Z_STRVAL(*val_zv);
 	val_len = Z_STRLEN(*val_zv);
 	if (val_len != addv_len) {
-		zend_throw_exception(sodium_exception_ce, "values must have the same length", 0);
+		zend_argument_error(sodium_exception_ce, 1, "and argument #2 ($string_2) must have the same size");


Other possibilities:

and argument #2 ($string_2) must have the same length

must be as long as argument #2 ($string_2)

and argument #2 ($string_2) must be the same long (is it even correct?)

must have the same length looks like the best option to me.

nikic · 2020-02-28T14:23:27Z

ext/sodium/libsodium.c

@@ -558,7 +558,7 @@ PHP_FUNCTION(sodium_add)
 	val = (unsigned char *) Z_STRVAL(*val_zv);
 	val_len = Z_STRLEN(*val_zv);
 	if (val_len != addv_len) {
-		zend_throw_exception(sodium_exception_ce, "values must have the same length", 0);
+		zend_argument_error(sodium_exception_ce, 1, "and argument #2 ($string_2) must have the same size");


must have the same length looks like the best option to me.

nikic · 2020-02-28T14:24:33Z

ext/sodium/libsodium.c

+		RETURN_THROWS();
+	}
+	if (hash_len >= 0xffffffff) {
+		zend_argument_error(sodium_exception_ce, 1, "is too long");


Suggested change

zend_argument_error(sodium_exception_ce, 1, "is too long");

zend_argument_error(sodium_exception_ce, 1, "is too large");

Unlike the similar message below, this is for an integer argument.

kocsismate · 2020-02-28T16:24:27Z

ext/sodium/libsodium.c

 		RETURN_THROWS();
 	}
 	if (padded_len < blocksize) {
-		zend_throw_exception(sodium_exception_ce, "invalid padding", 0);
+		zend_argument_error(sodium_exception_ce, 1, "must be less than the blocksize");


sorry, I've just realized this error message makes zero sense. :/ So what about something like Argument # 1 ($string) must have a length greater than or equal to argument # 2 ($length) instead? Unfortunately, I couldn't come up with a better one.

I don't think we need to force ourselves to always mention all arguments in this format. Something like Argument #1 ($string) must not be shorter than the block size maybe?

PS: It's annoying that the error messages in this ext aren't getting tested, so it's not visible whether the result is actually good. For example, in this case the parameter name should really be changed to $blockSize instead of $length. We haven't cared a lot about quality of parameter names in arginfo before, but now with it appearing in error messages, we may have to fix some stuff...

Something like Argument #1 ($string) must not be shorter than the block size maybe?

Seems good! I'll change the argument name too.

It's annoying that the error messages in this ext aren't getting tested

:/

now with it appearing in error messages, we may have to fix some stuff...

I agree. To make things even worse, param names in the documentation also have bad quality... Plus, I am sure there are a lots of inconsistencies between param names at php.net and param names in stubs (even though php.net uses $length in this case). That's why I've already planned a param name "overhaul" + sync. Hopefully, I can make it if I can finish my planned RFCs in time.

Hmm, just realized that it would be very cool to be able to generate function signatures in the documentation from the stubs.

nikic reviewed Feb 23, 2020

View reviewed changes

ext/sodium/libsodium.c Outdated Show resolved Hide resolved

nikic reviewed Feb 23, 2020

View reviewed changes

ext/sodium/libsodium.c Outdated Show resolved Hide resolved

kocsismate force-pushed the consistent-error-messages3-sodium branch from 1cf0dd9 to f5fd02a Compare February 23, 2020 20:52

kocsismate force-pushed the consistent-error-messages3-sodium branch from f5fd02a to 042fc09 Compare February 24, 2020 09:25

nikic reviewed Feb 24, 2020

View reviewed changes

kocsismate force-pushed the consistent-error-messages3-sodium branch 4 times, most recently from 386fff8 to 02f70d9 Compare February 24, 2020 22:01

Improve argument error messages in ext/sodium

4c65a39

kocsismate force-pushed the consistent-error-messages3-sodium branch from 02f70d9 to 4c65a39 Compare February 26, 2020 14:18

nikic reviewed Feb 26, 2020

View reviewed changes

Address review comments

e64af7d

kocsismate force-pushed the consistent-error-messages3-sodium branch from 4d97999 to e64af7d Compare February 27, 2020 08:07

kocsismate commented Feb 27, 2020

View reviewed changes

nikic approved these changes Feb 28, 2020

View reviewed changes

Address code review comments

172d83f

kocsismate commented Feb 28, 2020

View reviewed changes

php-pulls closed this in 118b04b Feb 28, 2020

kocsismate deleted the consistent-error-messages3-sodium branch February 28, 2020 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve argument error messages in ext/sodium #5197

Improve argument error messages in ext/sodium #5197

kocsismate commented Feb 22, 2020 •

edited

Loading

Girgias commented Feb 22, 2020

nikic commented Feb 23, 2020

nikic commented Feb 23, 2020

kocsismate commented Feb 23, 2020 •

edited

Loading

nikic Feb 24, 2020

nikic Feb 24, 2020

kocsismate Feb 24, 2020

nikic Feb 26, 2020

nikic Feb 26, 2020

kocsismate Feb 27, 2020

Girgias Feb 27, 2020

nikic Feb 24, 2020

kocsismate Feb 24, 2020

nikic Feb 24, 2020

kocsismate Feb 24, 2020

nikic Feb 24, 2020

kocsismate Feb 24, 2020

nikic Feb 26, 2020

kocsismate Feb 27, 2020

nikic Feb 26, 2020

nikic Feb 26, 2020

kocsismate Feb 27, 2020

kocsismate Feb 27, 2020

kocsismate Feb 27, 2020 •

edited

Loading

nikic Feb 28, 2020

nikic Feb 28, 2020

nikic Feb 28, 2020

kocsismate Feb 28, 2020 •

edited

Loading

nikic Feb 28, 2020

kocsismate Feb 28, 2020 •

edited

Loading

kocsismate Feb 28, 2020

	zend_argument_error(sodium_exception_ce, 1, "is too long");
	zend_argument_error(sodium_exception_ce, 1, "is too large");

Improve argument error messages in ext/sodium #5197

Improve argument error messages in ext/sodium #5197

Conversation

kocsismate commented Feb 22, 2020 • edited Loading

Girgias commented Feb 22, 2020

nikic commented Feb 23, 2020

nikic commented Feb 23, 2020

kocsismate commented Feb 23, 2020 • edited Loading

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

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

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

kocsismate Feb 27, 2020 • edited Loading

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

kocsismate Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate commented Feb 22, 2020 •

edited

Loading

kocsismate commented Feb 23, 2020 •

edited

Loading

kocsismate Feb 27, 2020 •

edited

Loading

kocsismate Feb 28, 2020 •

edited

Loading

kocsismate Feb 28, 2020 •

edited

Loading