New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI_process() didn't generate errors #2037
Conversation
Since there are many parts of UI_process() that can go wrong, it isn't very helpful to only return -1 with no further explanation. With this change, the error message will at least show which part went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two nits. also okay to backport if you want.
|
||
if (ui->meth->ui_open_session && !ui->meth->ui_open_session(ui)) | ||
return -1; | ||
if (ui->meth->ui_open_session && !ui->meth->ui_open_session(ui)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need explicit "!= NULL" test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only there? This is old stuff and that kind of change is needed all over the place. I'd say that's for a separate cleanup PR.
|
||
if (ok == -1) { | ||
UIerr(UI_F_UI_PROCESS, UI_R_PROCESSING_ERROR); | ||
ERR_add_error_data(2, "while ", state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh, clever.
@@ -427,9 +427,13 @@ static int print_error(const char *str, size_t len, UI *ui) | |||
int UI_process(UI *ui) | |||
{ | |||
int i, ok = 0; | |||
const char *state = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be something generic like "processing" to future-proof from future bunglers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, yeah ok
Since there are many parts of UI_process() that can go wrong, it isn't very helpful to only return -1 with no further explanation. With this change, the error message will at least show which part went wrong. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2037)
Since there are many parts of UI_process() that can go wrong, it isn't very helpful to only return -1 with no further explanation. With this change, the error message will at least show which part went wrong. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2037) (cherry picked from commit 0a687ab)
Merged to master and 1.1.0 |
Checklist
Description of change
Since there are many parts of UI_process() that can go wrong, it isn't
very helpful to only return -1 with no further explanation. With this
change, the error message will at least show which part went wrong.