OSSL_PARAM_BLD_push_octet_*(): Allow NULL buffer with 0 bsize#30730
OSSL_PARAM_BLD_push_octet_*(): Allow NULL buffer with 0 bsize#30730t8m wants to merge 2 commits intoopenssl:masterfrom
Conversation
|
However, it is debatable if this is what we want to support. Please note that OSSL_PARAM_get_octet_string() will fail on such param. @paulidale @slontis @mattcaswell Opinions? |
xnox
left a comment
There was a problem hiding this comment.
In Wolfi, we applied a similar fix already. So approved.
Also would be appreciated to backport this to 3.6 and 3.5 series, once it lands.
Don't know why I approved since I already knew of the test failures...
| int secure; | ||
|
|
||
| if (bld == NULL || key == NULL || buf == NULL) { | ||
| ERR_raise(ERR_LIB_CRYPTO, ERR_R_PASSED_NULL_PARAMETER); |
There was a problem hiding this comment.
Do we need to cater for empty strings? (I assume not??)
There was a problem hiding this comment.
Empty string still has NUL.
We currently crash if bsize is 0 for UTF8 strings/ptrs.
| int secure; | ||
|
|
||
| if (bld == NULL || key == NULL || buf == NULL) { | ||
| if (bld == NULL || key == NULL || (buf == NULL && bsize != 0)) { |
There was a problem hiding this comment.
Change looks ok to me...
It wont alloc any blocks and will correctly set the param to be data=NULL, data_size=0
|
Fixed the test failure. @paulidale @slontis please re-review |
|
This pull request is ready to merge |
| OSSL_PARAM_BLD_DEF *pd; | ||
|
|
||
| if (bld == NULL || key == NULL) { | ||
| if (bld == NULL || key == NULL || buf == NULL) { |
There was a problem hiding this comment.
This change is mentioned neither in PR description nor in commit message.
|
Thank you. Pushed to |
Fixes #30728