-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 platform abstraction for memory #6429
Draft
gilles-peskine-arm
wants to merge
6
commits into
Mbed-TLS:development
Choose a base branch
from
gilles-peskine-arm:platform-memory-test
base: development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Test platform abstraction for memory #6429
gilles-peskine-arm
wants to merge
6
commits into
Mbed-TLS:development
from
gilles-peskine-arm:platform-memory-test
+297
−30
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
MBEDTLS_MEMORY_BUFFER_ALLOC_C is not set in full, so there is no need to unset it. The calls to unset it were remainders of a time when the option was set in full. This commit extends 24600e8 to work that was in flight at the time. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
added
enhancement
component-platform
Portability layer and build scripts
needs-ci
Needs to pass CI tests
size-s
Estimated task size: small (~2d)
component-test
Test framework and CI scripts
priority-low
Low priority - this may not receive review soon
labels
Oct 14, 2022
gilles-peskine-arm
added
priority-medium
Medium priority - this can be reviewed as time permits
and removed
priority-low
Low priority - this may not receive review soon
labels
Oct 14, 2022
Test that MBEDTLS_PLATFORM_CALLOC_MACRO and MBEDTLS_PLATFORM_FREE_MACRO have the expected effect, namely, to set what mbedtls_calloc() and mbedtls_free() do. The tests are only enabled in a special test configuration where those macros are set to specific functions from the test framework. This special configuration can be activated by setting MBEDTLS_TEST_PLATFORM_MACROS and including "tests/configs/user-config-for-test.h" (typically as MBEDTLS_USER_CONFIG_FILE). This is intended to be the start of a framework that will be generalized to other platform abstraction macros. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
There is no reason to require the platform module just to define replacement macros for platform abstraction functions. This commit removes the requirement for calloc and free, and adds a CI build to ensure this is tested. I expect follow-ups to generalize this to other platform abstraction macros. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
force-pushed
the
platform-memory-test
branch
from
October 17, 2022 08:56
d42da25
to
04e590d
Compare
Test mbedtls_platform_set_calloc_free(). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The test code needs to know the default functions to restore them after changing them, and the runtime API doesn't provide this information, so the compile-time settings are necessary. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
force-pushed
the
platform-memory-test
branch
from
October 17, 2022 11:32
04e590d
to
2cf4ff7
Compare
gilles-peskine-arm
added
needs-review
Every commit must be reviewed by at least two team members,
needs-reviewer
This PR needs someone to pick it up for review
and removed
needs-ci
Needs to pass CI tests
labels
Oct 17, 2022
tom-daubney-arm
added
the
historical-reviewing
Currently reviewing (for legacy PR/issues)
label
Jul 14, 2023
We are now converting older PRs to draft PRs where the following conditions are met: They have not been updated in the last 3 months, and they need work to complete; in this case there are conflicts. |
tom-daubney-arm
added
needs-work
historical-reviewed
Reviewed & agreed to keep legacy PR/issue
and removed
needs-review
Every commit must be reviewed by at least two team members,
needs-reviewer
This PR needs someone to pick it up for review
historical-reviewing
Currently reviewing (for legacy PR/issues)
labels
Jul 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
component-platform
Portability layer and build scripts
component-test
Test framework and CI scripts
enhancement
historical-reviewed
Reviewed & agreed to keep legacy PR/issue
needs-work
priority-medium
Medium priority - this can be reviewed as time permits
size-s
Estimated task size: small (~2d)
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.
Test the platform abstraction macros and function that allow customizing
mbedtls_calloc
andmbedtls_free
. Previously, we were testing builds using those features, but not testing that the features had the desired effect.Remove the restriction that
MBEDTLS_PLATFORM_CALLOC_MACRO
andMBEDTLS_PLATFORM_FREE_MACRO
requireMBEDTLS_PLATFORM_C
. This restriction was added a long time ago, as part of a batch that made both macro-based replacements and variable-based replacements requireMBEDTLS_PLATFORM_C
, but only the variable-based replacements actually needplatform.c
. (See #6199 for a bit of discussion on this topic.)I've purposefully kept the scope to just calloc/free. Once we're happy with the general approach, it should be extended to the rest of
platform.h
.Changelog: removing a configuration restriction is a new feature, so it gets a changelog entry.
Backport: at least the new tests. Possibly even the removal of the configuration restriction: it doesn't change anything for users who don't try out the newly possible configuration, and I think it'll make our life easier because we'll have an easier time backporting test configurations.