Fix some test cases that weren't getting executed #3463
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test case was never executed. Signed-off-by: Gilles Peskine <[email protected]>
The test cases were never executed. Signed-off-by: Gilles Peskine <[email protected]>
The metadata tests depend on the corresponding feature because there is no guarantee that the metadata is correct if the feature is disabled. There are metadata test cases for some algorithms and key types that are declared but not supported. These test cases are present but can never run. It is debatable whether having these test cases is a good thing in case they become runnable in the future, or a bad thing because they're dead code. We're working on detecting test cases that are never executed for accidental reasons (e.g. typo in a dependency or missing configuration on the CI), and having test cases that are deliberately never executed messes this up. So remove these test cases. If we do implement the corresponding feature, it'll be easy to add the corresponding metadata test cases. The features that had metadata tests but no implementations were: * SHA-512/256 and SHA-512/224 (hypothetical dependency: MBEDTLS_SHA512_256) * DSA (hypothetical dependency: MBEDTLS_DSA_C) * SHA-3 and HMAC-SHA-3 (hypothetical dependency: MBEDTLS_SHA3_C) Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
added
needs-review
mpg
previously approved these changes
Jul 2, 2020
tests/ssl-opt.sh
Outdated
| ;; | ||
| esac | ||
|
|
||
| unset cmd ciphersuite |
There was a problem hiding this comment.
Minor: this function doesn't set cmd so it probably doesn't need to unset it.
There was a problem hiding this comment.
Accidental leftover from an earlier draft that used cmd. Since this was the last commit, I amended it to fully remove cmd.
Test cases that force a specific ciphersuites are only executed if this ciphersuite is enabled. But there are test cases (for RC4) whose goal is to check that the ciphersuite is not used. These test cases must run even if (or only if) the ciphersuite is disable, so add an exception for these test cases. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
force-pushed
the
tests-not-executed
branch
from
5645a77 to
0d72165
Compare
gilles-peskine-arm
removed
the
needs-backports
|
These are bug fixes that should be backported where applicable, but it turns out that all of them are in new code that didn't exist in 2.16. |
ronald-cron-arm
added
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix some test cases that weren't getting executed for one reason or another. See this analysis for background. This is not intended to fix all cases, just some that are easy to fix.
The check for test cases that are never executed (informational in #3458, mandatory in a follow-up that is yet to come) will ensure that these bug don't come back.
None of the changes here apply to 2.16 or 2.7. All the changes in unit tests are for new cases added since 2.16, and the bug fix in
ssl-opt.shis caused byrequires_ciphersuitewhich was added after 2.16.