Fix full build including non-boolean with Asan: crypto part #144
Conversation
This file contains hidden or 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
gilles-peskine-arm
added
bug
Patater
suggested changes
Jun 7, 2019
There was a problem hiding this comment.
The commit that adds TEST_ASSERT( psa_crypto_init( ) == PSA_SUCCESS );, replacing 0 with PSA_SUCCESS, has the same commit message as a commit that does something quite different. Please write a correct commit message for the commit that changes 0 to PSA_SUCCESS.
Otherwise, LGTM.
gilles-peskine-arm
force-pushed
the
oss-fuzz-fix-build-crypto
branch
from
3afcc53 to
88eefb8
Compare
|
Rebased to fix copypasta in the commit message. |
Patater
previously approved these changes
Jun 10, 2019
|
This PR is primarily a precursor to Mbed-TLS/mbedtls#2684 . I've split that TLS PR into pieces, the first of which Mbed-TLS/mbedtls#2697 is supported by a new crypto PR #148. I'm not sure yet whether I'll split this PR further or just rebase it on top of the merge of #148 so I'm leaving it open for now. |
gilles-peskine-arm
added
needs: work
psa_crypto_init() can fail. Do check its return code. Don't call it before initializing local objects that are going to be cleaned up.
Writing 0 instead of PSA_SUCCESS is correct, but bad form.
memset has undefined behavior when either pointer can be NULL, which is the case when it's the result of malloc/calloc with a size of 0. The memset calls here are useless anyway since they come immediately after calloc.
Call mbedtls_entropy_free on test failure. Restore the previous NV seed functions which the call to mbedtls_platform_set_nv_seed() changed. This didn't break anything, but only because the NV seed functions used for these tests happened to work for the tests that got executed later in the .data file.
This test case was only executed if the SHA-512 module was enabled and MBEDTLS_ENTROPY_FORCE_SHA256 was not enabled, so "config.pl full" didn't have a chance to reach it even if that enabled MBEDTLS_PLATFORM_NV_SEED_ALT. Now all it takes to enable this test is MBEDTLS_PLATFORM_NV_SEED_ALT and its requirements, and the near-ubiquitous MD module.
gilles-peskine-arm
force-pushed
the
oss-fuzz-fix-build-crypto
branch
from
5b4cb75 to
66afcca
Compare
gilles-peskine-arm
added
needs: review
This was referenced Jul 19, 2019
AndrzejKurek
approved these changes
Aug 14, 2019
gilles-peskine-arm
removed
the
needs: review
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 pull requests fixes some issues in test suites that we hadn't caught before because they aren't tested:
Enabling[split out in https://github.com/Make test suites compatible with #include <assert.h> #148]MBEDTLS_CHECK_PARAMSas suggested inconfig.hdoesn't work (Havege does arithmetic on signed int Mbed-TLS/mbedtls#2598)Fix undefined behavior in HAVEGE (Havege does arithmetic on signed int Mbed-TLS/mbedtls#2598)[split out in https://github.com/Fix misuse of signed ints in the HAVEGE module #149]This PR does not add CI jobs that would detect those issues. This will come through PR in the mbedtls repository for which this PR is a prerequisite: Mbed-TLS/mbedtls#2684 (or in several PRs that 2684 may be split into).
Backports: Mbed-TLS/mbedtls#2754 Mbed-TLS/mbedtls#2755