Backport 2.7: Clarify that the full config enables everything that can be tested together #3295
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
These options haven't existed for a long time. Signed-off-by: Gilles Peskine <[email protected]>
Enable MBEDTLS_X509_ALLOW_EXTENSIONS_NON_V3 in the full config. There's no reason to keep it out. We weren't testing it at all on the CI. Signed-off-by: Gilles Peskine <[email protected]>
Remove the duplicated, and often out-of-date, list in the comments. Instead explain in a comment, and have a single copy of the list which is in the code. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
The intended logic around MBEDTLS_xxx_ALT is to exclude them from full because they require the alternative implementation of one or more library functions, except that MBEDTLS_PLATFORM_xxx_ALT are different: they're alternative implementations of a platform function and they have a built-in default, so they should be included in full. Document this. Fix a bug whereby MBEDTLS_PLATFORM_xxx_ALT didn't catch symbols where xxx contains an underscore. As a consequence, MBEDTLS_PLATFORM_NV_SEED_ALT is now enabled in the full config. Explicitly exclude MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT because it behaves like the non-platform ones, requiring an extra build-time dependency. Explicitly exclude MBEDTLS_PLATFORM_NV_SEED_ALT from baremetal because it requires MBEDTLS_ENTROPY_NV_SEED, and likewise explicitly unset it from builds that unset MBEDTLS_ENTROPY_NV_SEED. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
added
mbed TLS team
needs-review
gilles-peskine-arm
force-pushed
the
config-full-clarify-2.7
branch
from
036800d to
1177945
Compare
mpg
previously approved these changes
Apr 30, 2020
gilles-peskine-arm
added
needs-ci
An earlier botched backport (d56ca65) had the wrong name for a variable and a missing header inclusion. Signed-off-by: Gilles Peskine <[email protected]>
It works in practice on almost every platform, given that we're only using the wrong type in cases where the value is guaranteed to stay within the value bits of a signed int. But even in this case it may or may not be strictly conforming. Anyway `gcc -std=c99 -pedantic` rejects it. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
force-pushed
the
config-full-clarify-2.7
branch
from
1177945 to
2bb9cef
Compare
ronald-cron-arm
previously approved these changes
Apr 30, 2020
| } | ||
|
|
||
| component_test_full_deprecated_warning () { | ||
| # Test that there is nothing deprecated in the full configuraration. |
There was a problem hiding this comment.
Same typo as in 2.16.
|
It seems the CI is not completely happy though. |
|
It turns out that 2.7 hadn't had the benefit of 2682edf. That patch changes how some library files are built, and I'd prefer not to do that in an LTS branch if there isn't a good reason. “It breaks on some platform that someone cares about” would be a good reason, but I don't think “it removes a difference between branches” is. So I'm going to rework this PR to not start doing a build in strict c99 mode with the platform code enabled. In 2.7, unlike the more recent versions, only no-platform builds pass in strict c99 mode. |
Don't use string literals that are longer than 4095 bytes, which is the minimum that C99 compilers are required to support. Compilers are extremely likely to support longer literals, but `gcc -std=c99 -pedantic` complains. Signed-off-by: Gilles Peskine <[email protected]>
In the full config, don't set MBEDTLS_DEPRECATED_WARNING. This is debatable: the full config does not enable deprecated features in this branch, so MBEDTLS_DEPRECATED_WARNING is compatible with the other features. Exclude it to keep LTS branches closer to development. In any case, baremetal and full should have the same settings regarding deprecated features, so don't do anything about DEPRECATED_xxx in baremetal. Signed-off-by: Gilles Peskine <[email protected]>
build_deprecated combined the testing of deprecated features, and testing of the build without deprecated features. Also, it violated the component naming convention by being called build_xxx but running tests. Replace it by: * test_default_no_deprecated: check that you can remove deprecated features from the default build. * test_full_deprecated_warning: check that enabling DEPRECATED_WARNING doesn't cause any warning from our own code. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
force-pushed
the
config-full-clarify-2.7
branch
from
2bb9cef to
adaaddb
Compare
gilles-peskine-arm
added
needs-review
ronald-cron-arm
approved these changes
May 4, 2020
There was a problem hiding this comment.
I checked the diff between this new version and the previous one and the differences are those explained in the PR first comment (#3295 (comment)) which I agree with. LGTM.
mpg
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.
This is a partial backport of #3190. It is mostly similar to the 2.16 backport. Differences with the 2.16 backport:
all.shcomponents withMBEDTLS_PLATFORM_NV_SEED_ALT.-std=c99build in the full configuration and some of the commits to strict-c99-ify the code. I kept strict-c99-ification commits only where there was an advantage to including the commit to keep the code aligned between branches to facilitate future backports. None of these risk of breaking the build on some “weird” platform where macros like_POSIX_C_SOURCEare not compliant.If we decide that 2.7 should compile with
gcc -std=c99, let's do that in a separate PR that backports parts of gmtime_r and mbedtls_time on Windows #1198 and Backport 2.7: Clarify that the full config enables everything that can be tested together #3295 and does whatever else is necessary.