OpenSSL extension has many inconsistencies in how errors are handled/reported/propagated
还没有人认领这个 Issue。
- 主要语言
- C
- 星标
- 40.4k
- 派生
- 8.1k
- 平均合并
- 2 天 13 小时
- 30 天内合并 PR
- 96
描述
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:
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:
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...
Usage of E_ERROR for some reason, which sometimes could be appropriate but sometimes isn't.
Examples:
here we're dealing with allocation failure so it may be appropriate.
However, here it seems inappropriate:
These are just some examples, but shows a much broader problem with the error-handling design of the extension.
A concrete list found by my tool:
- Function: ASN1_STRING_to_UTF8 | Source: /work/php-src/ext/openssl/openssl_backend_common.c:74 | Return: -1
- Function: BIO_new_mem_buf | Source: /work/php-src/ext/openssl/openssl_backend_common.c:1269 | Return: 0
- Function: BIO_new_mem_buf | Source: /work/php-src/ext/openssl/openssl_backend_common.c:532 | Return: 0
- Function: PEM_read_bio_PrivateKey_ex | Source: /work/php-src/ext/openssl/openssl_backend_common.c:1292 | Return: 0
- Function: X509_LOOKUP_ctrl | Source: /work/php-src/ext/openssl/openssl_backend_common.c:834 | Return: -1
- Function: X509_LOOKUP_ctrl | Source: /work/php-src/ext/openssl/openssl_backend_common.c:834 | Return: 0
- Function: X509_LOOKUP_ctrl | Source: /work/php-src/ext/openssl/openssl_backend_common.c:840 | Return: -1
- Function: X509_LOOKUP_ctrl | Source: /work/php-src/ext/openssl/openssl_backend_common.c:840 | Return: 0
- Function: X509_STORE_add_lookup | Source: /work/php-src/ext/openssl/openssl_backend_common.c:834 | Return: 0
- Function: X509_STORE_add_lookup | Source: /work/php-src/ext/openssl/openssl_backend_common.c:840 | Return: 0
- Function: NCONF_load | Source: /work/php-src/ext/openssl/openssl_backend_common.c:301 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:255 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:323 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:325 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:327 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:396 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:939 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:949 | Return: 0
- Function: PEM_write_bio_X509 | Source: /work/php-src/ext/openssl/openssl.c:1507 | Return: 0
- Function: PEM_write_bio_X509 | Source: /work/php-src/ext/openssl/openssl.c:1541 | Return: 0
- Function: PEM_write_bio_X509 | Source: /work/php-src/ext/openssl/openssl.c:2791 | Return: 0
- Function: PEM_write_bio_X509 | Source: /work/php-src/ext/openssl/openssl.c:3443 | Return: 0
- Function: PEM_write_bio_PrivateKey | Source: /work/php-src/ext/openssl/openssl.c:1520 | Return: 0
- Function: PEM_ASN1_read_bio | Source: /work/php-src/ext/openssl/openssl_backend_v3.c:871 | Return: 0
- Function: BIO_new | Source: /work/php-src/ext/openssl/openssl.c:1507 | Return: 0
PHP Version
Report was made on master, but applies to all versions really.
Operating System
No response
贡献指南
从这里开始
- 先读完整个 Issue,再读项目的贡献指南。
- 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
- Fork 仓库,在一个分支上完成修改。
- 提交 Pull Request,并在描述里引用这个 Issue 编号。
调研方向
首先检查 ext/openssl/openssl_backend_common.c 和 ext/openssl/openssl.c 中的错误路径,尤其是所引用的 php_openssl_store_errors()、php_openssl_conf_get_string() 调用以及 OpenSSL I/O 函数。比较列出的不一致之处,并针对返回值、警告、已存储的错误以及 E_ERROR 处理建立一致的策略。当该扩展已有达成共识的设计,受影响的调用点均一致地遵循该设计,并且已对变更后的行为进行覆盖时,即表示完成。
由索引模型根据 Issue 内容生成。
评估
- 技术栈
- c
- 领域
- backend, security
- Issue 类型
- 重构
- 难度
- 5/5
- 预计耗时
- 一周以上
- 活跃度
- 冷清
- 描述清晰度
- 需要澄清
- 新手友好度
- 25/100