Improve error reporting in random extension #9071

TimWolla · 2022-07-20T17:45:49Z

No description provided.

This exposes the underlying exception, improving debugging: Fatal error: Uncaught Exception: Cannot open source device in php-src/test.php:5 Stack trace: #0 php-src/test.php(5): Random\Engine\Secure->generate() #1 {main} Next RuntimeException: Random number generation failed in php-src/test.php:5 Stack trace: #0 php-src/test.php(5): Random\Engine\Secure->generate() #1 {main} thrown in php-src/test.php on line 5

This exposes the underlying exception, improving debugging: Exception: Cannot open source device in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3) #1 {main} Next RuntimeException: Random number generation failed in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3) #1 {main}

This improves debugging, because the actual reason for the failure is available as a previous Exception: DomainException: The returned string must not be empty in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getBytes(123) #1 {main} Next RuntimeException: Random number generation failed in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getBytes(123) #1 {main}

…rs in 50 attempts This improves debugging, because the actual reason for the failure is available as a previous Exception: RuntimeException: Failed to generate an acceptable random number in 50 attempts in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3) #1 {main} Next RuntimeException: Random number generation failed in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3) #1 {main}

Select parameters for ->getInt() that will actually lead to unsafe behavior.

If an engine fails once it will be permanently poisoned by setting `->last_unsafe`. This is undesirable for the test, because it skews the results. Fix this by creating a fresh engine for each "assertion".

zeriyoshi · 2022-07-21T03:04:55Z

I was struggling with the appropriate message.
This looks very good. Thank you!

Girgias · 2022-07-21T15:03:04Z

ext/random/engine_user.c

@@ -50,6 +53,7 @@ static uint64_t generate(php_random_status *status)
 			result += ((uint64_t) (unsigned char) Z_STRVAL(retval)[i]) << (8 * i);
 		}
 	} else {
+		zend_throw_exception(spl_ce_DomainException, "A random engine must return a non-empty string", 0);


I would rather use a ValueError here

I considered that, but ValueError is documented as:

A ValueError is thrown when the type of an argument is correct but the value of it is incorrect.

Thus it is documented to apply to parameters / arguments, which is not exactly the case here.

We could update the ValueError documentation; but probably throwing an Error directly is the way to go. zend_throw_error() accepts NULL for the first parameter, and that is already used quite a lot across php-src. I don't think that introducing a new error type would be particularly useful, since Errors are not supposed to be caught.

ext/random/engine_user.c

ext/random/random.c

As we print the full stringified exception we implicitly assert the type of the exception. No need to be overly specific in the catch block.

Danack · 2022-07-23T17:47:06Z

Some generic thoughts on exceptions, followed by opinions for the PR.

Each 'module' should define and only use its own exceptions.

It is strongly preferred to create new exceptions rather than re-using exceptions that already exist in the engine.

The reason for this is your should be able to write code like this:

try {
   somethingSPLRelated();
   somethingRandomRelated();
}
catch (SplInvalidArgumentException $e) {...}
catch (RandomInvalidArgumentException $e) {...}

Rather than have to break blocks up like this:

try {
   somethingSPLRelated();
}
catch (InvalidArgumentException $e) {...}

try {
   somethingRandomRelated();
}
catch (InvalidArgumentException $e) {...}

I know PHP has been terrible at this in the past. And also that exactly what a 'module' is internally inside core PHP is not entirely well-defined. But still. We can try to do better.

Each 'module' should define a base extension that can be caught:

For anyone calling your code, it is useful to be able to catch any exception your code might throw with a single catch statement:

try {
   foo();
}
catch (FooException $fe) {
   // something went wrong with foo, and
   // we don't particularly care what.
}

rather than having to catch every possible exception:

try {
   foo();
}
catch (InvalidArgumentException|OutOfRangeException $fe) {
   // something went wrong with foo, and
   // we don't particularly care what.
}

Though the exact amount of value that produces depends on how many child/specific exceptions there are, with this approach providing more value when there are more exceptions.

Should base extensions be a class or interface?

I don't know. You should ask someone smarter than me. If I had to guess, I say interface and give myself about a 60% chance of being correct.

Exceptions or Error

Exceptions are for when the program is probably written correctly, but something external to the program or computer is failing. E.g. when the database has fallen over in the middle of a query, throwing an exception is correct.

Errors are for when the program is probably written incorrectly, and a someone is going to have to go into the code and fix something. Or for when the computer it is running on is seriously sick. e.g. not being able to open a file descriptor because the machine has reached the file descriptor limit means it's time to stop trying to run the program.

Exception names should duplicate the namespace name to avoid collisions.

If we had two exceptions of Spl\OutOfRangeException and Random\OutOfRangeException using both of these in the same file would be annoying. Users would have to alias the names themself.

e.g. this errors:

<?php

use Spl\OutOfRangeException;
use Random\OutOfRangeException;

// Fatal error: Cannot use Random\OutOfRangeException

Have to alias:

<?php

use Spl\OutOfRangeException as SplOutOfRange;
use Random\OutOfRangeException as SplOutOfRange;

// Fatal error: Cannot use Random\OutOfRangeException

By including the namespace or module name and instead having Spl\SplOutOfRangeException and Random\RandomOutOfRangeException life is made better for end-users.

This is probably the part people would be most likely to disagree on.

And so...

When the string returned is zero length aka engine_user.c:56:

RandomLogicError extends/implements RandomError

Logic error because it’s the implementation of the random generator that is very probably at fault.

And RandomError extends \Error.

Unserialize

The places where an exception is thrown in PHP_METHOD(Random_Randomizer, __unserialize) could be left as they are.

Technically it would be far saner if there was a single exception of UnserializeFailedException that was used across all the PHP code base. But it's probably a bit late for that.

Everywhere else

Pretty much all of the rest should probably be:

RandomRuntimeError extends/implements RandomError

RuntimeError because something went wrong that might not be in the programmers control, but also can't be ignored. e.g. a complete lack of entropy or file descriptors.

So that includes both of those in php_random_bytes:
php_src/php-src-tim/ext/random/random.c:475
php_src/php-src-tim/ext/random/random.c:488

And all of the other ones that you weren't even asking about, e.g:

zend_throw_exception(zend_ce_exception, "Cannot open source device", 0);

When this happens something is probably very wrong with the system.

Anyway, "It's just naming things, how hard could it be?" /s.

…mpts

TimWolla · 2022-07-24T09:48:35Z

@Danack Thank you for the extensive essay on this matter. That thing should likely be preserved somewhere for others in my position.

Also thanks to the others who voiced their opinions of not using the SPL exceptions. I have adjusted this PR to use a plain Error for the throws I added. Those will not be directly exposed to the user, as they are still wrapped in the existing RuntimeException from the initial implementation. As such this PR is compatible with what was proposed in the RFC.

As adjusting the "user-visible" Exception might need additional RM approval I plan to create a follow-up PR that does so based on Danack's suggestions.

TimWolla · 2022-07-24T09:49:23Z

Technically it would be far saner if there was a single exception of UnserializeFailedException that was used across all the PHP code base. But it's probably a bit late for that.

@Danack Is it? I believe it should not break compatibility if that new exception would extend the plain Exception, as any existing catch blocks continue to catch.

zeriyoshi · 2022-07-24T23:57:05Z

@Girgias @TimWolla @Danack
Excellent documentation. Thank you very much.
I agree with you that modules should use their own exceptions.
However, I think this PR focuses on normalizing error messages, so I think it should be merged once. The changes look good to me.

By the way, changing the type of Throwable to be thrown causes a conflict with the RFC.
Is this a problem? If so, how should this be resolved?

https://wiki.php.net/rfc/rng_extension

Girgias · 2022-07-25T00:48:47Z

@Girgias @TimWolla @Danack Excellent documentation. Thank you very much. I agree with you that modules should use their own exceptions. However, I think this PR focuses on normalizing error messages, so I think it should be merged once. The changes look good to me.

By the way, changing the type of Throwable to be thrown causes a conflict with the RFC. Is this a problem? If so, how should this be resolved?

https://wiki.php.net/rfc/rng_extension

That's a minor implementation detail and not really of concern for a conflict with the RFC. And this is resolved by just merging...

zeriyoshi · 2022-07-25T00:50:40Z

@Girgias
OK. I approval it.

TimWolla added 7 commits July 20, 2022 19:11

Improve user_unsafe test

12b07ce

Select parameters for ->getInt() that will actually lead to unsafe behavior.

Fix user_unsafe test

3e18441

If an engine fails once it will be permanently poisoned by setting `->last_unsafe`. This is undesirable for the test, because it skews the results. Fix this by creating a fresh engine for each "assertion".

Remove duplication in user_unsafe.phpt

8adb67a

TimWolla requested a review from zeriyoshi July 20, 2022 17:45

github-actions bot added the Extension: random label Jul 20, 2022

zeriyoshi approved these changes Jul 21, 2022

View reviewed changes

Girgias reviewed Jul 21, 2022

View reviewed changes

Catch Throwable in user_unsafe.phpt

3090379

As we print the full stringified exception we implicitly assert the type of the exception. No need to be overly specific in the catch block.

TimWolla added 2 commits July 24, 2022 11:40

Throw an error if an engine returns an empty string

39b996a

Throw an Error if range fails to find an acceptable number in 50 atte…

734c79c

…mpts

TimWolla requested review from zeriyoshi and Girgias July 24, 2022 09:49

Girgias approved these changes Jul 25, 2022

View reviewed changes

zeriyoshi approved these changes Jul 25, 2022

View reviewed changes

TimWolla merged commit 60f149f into php:master Jul 25, 2022

TimWolla deleted the random-error-reporting branch July 25, 2022 07:01

This was referenced Jul 25, 2022

Remove ->last_unsafe from php_random_status #9132

Merged

Improve error message of an engine fails to seed from the CSPRNG #9160

Merged

TimWolla mentioned this pull request Aug 1, 2022

Add ext/random Exception hierarchy #9220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error reporting in random extension #9071

Improve error reporting in random extension #9071

TimWolla commented Jul 20, 2022

zeriyoshi commented Jul 21, 2022

Girgias Jul 21, 2022

TimWolla Jul 21, 2022

cmb69 Jul 23, 2022

Danack commented Jul 23, 2022 •

edited

Loading

TimWolla commented Jul 24, 2022

TimWolla commented Jul 24, 2022

zeriyoshi commented Jul 24, 2022

Girgias commented Jul 25, 2022

zeriyoshi commented Jul 25, 2022

Improve error reporting in random extension #9071

Improve error reporting in random extension #9071

Conversation

TimWolla commented Jul 20, 2022

zeriyoshi commented Jul 21, 2022

Girgias Jul 21, 2022

Choose a reason for hiding this comment

TimWolla Jul 21, 2022

Choose a reason for hiding this comment

cmb69 Jul 23, 2022

Choose a reason for hiding this comment

Danack commented Jul 23, 2022 • edited Loading

Each 'module' should define and only use its own exceptions.

Each 'module' should define a base extension that can be caught:

Should base extensions be a class or interface?

Exceptions or Error

Exception names should duplicate the namespace name to avoid collisions.

And so...

When the string returned is zero length aka engine_user.c:56:

Unserialize

Everywhere else

TimWolla commented Jul 24, 2022

TimWolla commented Jul 24, 2022

zeriyoshi commented Jul 24, 2022

Girgias commented Jul 25, 2022

zeriyoshi commented Jul 25, 2022

Danack commented Jul 23, 2022 •

edited

Loading