[php-src] Issue #21643: OpenSSL extension has many inconsistencies in how errors are handled/reported/propagated
[email protected] (ndossche)
| Newsgroups | php.bugs |
|---|---|
| Message-ID | <[email protected]> |
Issue: https://github.com/php/php-src/issues/21643 Author: ndossche ### Description The OpenSSL extension has a number of error-handling inconsistencies that I'm unsure how to handle. We should come to a consensus on how to move forward. Inconsistencies include: * Failure reason is not properly communicated to the caller of the function, or can't be distinguished at the call site. * Some functions emit warnings, some don't. * Some error-handling paths call `php_openssl_store_errors()`, some don't. * Some error-handling paths call `php_openssl_store_errors()` but still return a success. This means that partial success isn't obviously reported to the user in some cases, so theoretically they'd always have to check the `openssl_error_string()` function even if the function returned success, which isn't great. ## Some examples Partial failure example (error stored but loop not aborted, may be intentional but this isn't communicated): https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl_backend_common.c#L97-L99 --- The following two calls may fail because of an inexistent file or some I/O failure https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl_backend_common.c#L1293-L1297 but at least one of the callsites can't distinguish between different kinds of failure: https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl.c#L940-L944 --- Very similarly to the previous one, different kinds of I/O or validation failures are not distinguished here: https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl_backend_common.c#L511 --- Example of silently swallowing errors without returning a different return value or without emitting a warning; even though this same function emits warnings for other cases: https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl_backend_common.c#L827-L838 --- There's a bunch of calls to `php_openssl_conf_get_string` that silently ignore errors: because e.g. the key may be optional but note that a configuration error can also result in a silent failure. For example here: https://github.com/php/php-src/blob/6a4d0d9456bf872286722ad5f043ad4928e92c9a/ext/openssl/openssl_backend_common.c#L254-L257 but also in various spots in https://github.com/php/php-src/blob/6a4d0d9456bf872286722ad5f043ad4928e92c9a/ext/openssl/openssl_backend_common.c#L293 etc --- These are just some examples, but shows a much broader problem with the error-handling design of the extension. ### PHP Version ```plain Report was made on master, but applies to all versions really. ``` ### Operating System _No response_