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
test suite nits #4873
test suite nits #4873
Conversation
Avoid memory leaks in error paths, and correctly apply parentheses to function calls in a long if-chain.
There's no reason to wrap this call in TEST_true() if we're not checking the return value of TEST_true() -- all of the surrounding similar calls do not have the macro wrapping them.
make_dummy_resp() uses OCSP types, and get_cert_and_key() is unused once make_dummy_resp() is compiled out, so neither can be included in the build when OCSP is disabled and strict warnings are active.
s_client -status is not available in this configuration.
return NULL; | ||
goto err; | ||
bs_out = bs; | ||
bs = 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.
Presumably the point of this is so that we can free bs
if an error occurs - but I don't see that happening anywhere
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.
Good catch, thanks!
test/recipes/70-test_tls13messages.t
Outdated
#Test 5: A status_request handshake (client and server) | ||
#TODO(TLS1.3): TLS1.3 doesn't actually have CertificateStatus messages. This is | ||
#a temporary test until such time as we do proper TLS1.3 style certificate | ||
#status |
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.
Maybe not in scope of this PR but this TODO is now out of date. It can be removed.
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.
It's easy enough to remove it while I'm touching the code already. Plus it gets rid of a too-long line!
Per Matt's review. To be squashed, but the action noted in the resulting commit message.
Avoid memory leaks in error paths, and correctly apply parentheses to function calls in a long if-chain. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #4873)
There's no reason to wrap this call in TEST_true() if we're not checking the return value of TEST_true() -- all of the surrounding similar calls do not have the macro wrapping them. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #4873)
make_dummy_resp() uses OCSP types, and get_cert_and_key() is unused once make_dummy_resp() is compiled out, so neither can be included in the build when OCSP is disabled and strict warnings are active. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #4873)
s_client -status is not available in this configuration. While here, remove an outdated TODO(TLS1.3) comment. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #4873)
Pushed to master; closing. |
Avoid memory leaks in error paths, and correctly apply parentheses to function calls in a long if-chain. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#4873) (cherry picked from commit b6306d8)
make_dummy_resp() uses OCSP types, and get_cert_and_key() is unused once make_dummy_resp() is compiled out, so neither can be included in the build when OCSP is disabled and strict warnings are active. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#4873) (cherry picked from commit cb09129)
Fix some issues spotted by coverity and unbreak no-ocsp
@levitte the failure mode for the no-ocsp enable-tls1_3 mode is pretty poor, as s_client gives usage() but s_server keeps running, so the test suite appears to hang. Is there something more systematic we should be doing as well?