Fix some issues raise by Coverity in the tests. #3846
Conversation
test/bio_enc_test.c
Outdated
| @@ -41,7 +41,8 @@ static int do_bio_cipher(const EVP_CIPHER* cipher, const unsigned char* key, | |||
| BIO *b; | |||
| static unsigned char inp[BUF_SIZE] = { 0 }; | |||
| unsigned char out[BUF_SIZE], ref[BUF_SIZE]; | |||
| int i, lref, len; | |||
| int i, lref; | |||
| size_t len; | |||
There was a problem hiding this comment.
What issue is this fixing? "len" seems to be being used in part to store the return value from BIO_read()...which can be -ve (not that we seem to check it).
There was a problem hiding this comment.
What's -ve? Or rather can we not use lingo [to that extent]?
There was a problem hiding this comment.
Hmmm, it was a complaint about a negative return and a signed variable. I'll revert this one since the code seems correct as it was.
test/ct_test.c
Outdated
| @@ -324,7 +324,7 @@ static int execute_cert_test(CT_TEST_FIXTURE fixture) | |||
| goto end; | |||
| } | |||
|
|
|||
| tls_sct_list_len = i2o_SCT_LIST(scts, &tls_sct_list); | |||
| tls_sct_list_len = (size_t)i2o_SCT_LIST(scts, &tls_sct_list); | |||
There was a problem hiding this comment.
i2o_SCT_LIST() can return a -ve failure code?
There was a problem hiding this comment.
Yeah, it can. Same warning as before. Reverting this too.
test/evp_test.c
Outdated
| } | ||
| if (EVP_PKEY_derive_init(kdata->ctx) <= 0) { | ||
| OPENSSL_free(kdata); | ||
| EVP_PKEY_CTX_free(kdata->ctx); |
There was a problem hiding this comment.
Use after free. These statements should be the other way around.
| @@ -46,7 +46,7 @@ static int test_pbelu(void) | |||
| last_type = pbe_type; | |||
| last_nid = pbe_nid; | |||
| } | |||
| return failed ? 0 : 1; | |||
| return 0; | |||
There was a problem hiding this comment.
The assignment on line 43 seems redundant. Side note: I am slightly confused by this logic:
ok ? "ERROR" : "OK"
There was a problem hiding this comment.
Yes, the assignment is redundant. I've reworked the function to simplify it a bit.
|
It means negative. And yeah, +1 to not using it ☺
|
|
ping? @mattcaswell ? |
There was a problem hiding this comment.
Please can you make sure you update Coverity to close off the issues associated with this PR (i.e. update the classification, severity and action fields as appropriate). Also any which you decided were false positives as part of this work should also be updated (set to False Positive/Insignificant/Ignore) - I normally also include a comment for these.
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #3846)
richsalz
added
the
approval: done
This PR addresses the Coverity issues in the tests that have a simple solution that doesn't require disabling things.