gh-ARMmbed/mbed-crypto
Watch
1
0
Fork
You've already forked mbed-crypto
 0
Code Issues 36 Pull requests 1 Releases 7 Activity

Disallow use of invalid contexts #58

Merged
Patater merged 8 commits from disallow-invalid-context into development 2019-02-21 11:37:05 -06:00
Conversation 47 Commits 8 Files changed 8 +446 -41
Patater commented 2019-02-15 09:37:07 -06:00 (Migrated from github.com)

Disallow use of MAC, hash, and cipher contexts that haven't been set up (i.e. has a *_setup() function called on them).

Disallow use of MAC, hash, and cipher contexts that haven't been set up (i.e. has a `*_setup()` function called on them).
gilles-peskine-arm (Migrated from github.com) requested changes 2019-02-15 12:53:34 -06:00
gilles-peskine-arm (Migrated from github.com) left a comment
  • The implementation needs a design discussion about how to cope with bad-state errors. We have an unresolved question about the API spec at https://github.com/ARMmbed/psa-crypto/issues/45 (private link).
  • The tests don't cover all the cases that I'd expect. They're missing some bad-state cases for MAC and cipher.
  • Some of the tests need restructuring.
* The implementation needs a design discussion about how to cope with bad-state errors. We have an unresolved question about the API spec at https://github.com/ARMmbed/psa-crypto/issues/45 (private link). * The tests don't cover all the cases that I'd expect. They're missing some bad-state cases for MAC and cipher. * Some of the tests need restructuring.
library/psa_crypto.c
gilles-peskine-arm (Migrated from github.com) commented 2019-02-15 12:50:58 -06:00

I'm not sure if it's a good idea to call abort on a BAD_STATE error. What is likely to happen?

If the operation object was uninitialized or corrupted, abort has undefined behavior and this could be a way for an attacker to leverage a memory corruption error into remote code execution.

If the object was in a well-formed state, but not ready for an update, then for a hash that means that the object is already in a non-setup state and abort will do nothing.

I'm not sure if it's a good idea to call `abort` on a `BAD_STATE` error. What is likely to happen? If the operation object was uninitialized or corrupted, `abort` has undefined behavior and this could be a way for an attacker to leverage a memory corruption error into remote code execution. If the object was in a well-formed state, but not ready for an update, then for a hash that means that the object is already in a non-setup state and `abort` will do nothing.
tests/suites/test_suite_psa_crypto.function
gilles-peskine-arm (Migrated from github.com) commented 2019-02-15 12:44:15 -06:00

Please avoid mixing input validation tests with operation correctness tests. That makes it harder to understand what is tested, and makes it harder to run just the input validation tests without also running the more costly operation correctness tests. This function is for operation correctness.

This also goes for mac_verify.

Also, why test sign_finish followed by sign_finish, but not other invalid combinations such as sign_finish followed by update, sign_setup followed by verify_finish, etc.?

Please avoid mixing input validation tests with operation correctness tests. That makes it harder to understand what is tested, and makes it harder to run just the input validation tests without also running the more costly operation correctness tests. This function is for operation correctness. This also goes for `mac_verify`. Also, why test `sign_finish` followed by `sign_finish`, but not other invalid combinations such as `sign_finish` followed by `update`, `sign_setup` followed by `verify_finish`, etc.?
NirSonnenschein (Migrated from github.com) reviewed 2019-02-17 03:43:37 -06:00
NirSonnenschein (Migrated from github.com) left a comment

Looks ok to me besides the comments already made by Gilles. Are there any additional dependencies that need to be updated (e.g. example repo , usage by tls, etc..)?

Looks ok to me besides the comments already made by Gilles. Are there any additional dependencies that need to be updated (e.g. example repo , usage by tls, etc..)?
Patater (Migrated from github.com) reviewed 2019-02-18 11:28:05 -06:00
library/psa_crypto.c
Patater (Migrated from github.com) commented 2019-02-18 11:28:05 -06:00

The same reasoning seems to apply to psa_hash_finish() as well. I'll update there, too.

If implementation functions like mbedtls_sha1_finish_ret() and can ever return an error equivalent to PSA_ERROR_BAD_STATE, then we may have a pre-existing bug in psa_hash_finish() as we will call psa_hash_abort() in such a case.

The same reasoning seems to apply to `psa_hash_finish()` as well. I'll update there, too. If implementation functions like `mbedtls_sha1_finish_ret()` and can ever return an error equivalent to `PSA_ERROR_BAD_STATE`, then we may have a pre-existing bug in `psa_hash_finish()` as we will call `psa_hash_abort()` in such a case.
Patater commented 2019-02-19 06:22:16 -06:00 (Migrated from github.com)

Rebasing to address review feedback, extend tests, and fix #10.

Rebasing to address review feedback, extend tests, and fix #10.
Patater commented 2019-02-19 06:23:41 -06:00 (Migrated from github.com)

Rebased. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-1

Rebased. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-1
Patater commented 2019-02-19 07:39:18 -06:00 (Migrated from github.com)

I'm looking into some memory leaks.

In general, if you start a cipher operation, and some internal memory is allocated on your behalf (e.g. AES context created), and then you call finish twice and get a PSA_ERROR_BAD_STATE error, there is no way to free that lost memory anymore, is there? You can't call abort, since after you get an PSA_ERROR_BAD_STATE, all bets are off.

I'm looking into some memory leaks. In general, if you start a cipher operation, and some internal memory is allocated on your behalf (e.g. AES context created), and then you call finish twice and get a `PSA_ERROR_BAD_STATE` error, there is no way to free that lost memory anymore, is there? You can't call abort, since after you get an `PSA_ERROR_BAD_STATE`, all bets are off.
NirSonnenschein commented 2019-02-19 07:45:32 -06:00 (Migrated from github.com)

wouldn't the first call to finish de-allocate the resources?

wouldn't the first call to finish de-allocate the resources?
gilles-peskine-arm commented 2019-02-19 07:49:55 -06:00 (Migrated from github.com)

after you get an PSA_ERROR_BAD_STATE, all bets are off.

That's not true. All bets are off if you have a bad state because the object was corrupted or never initialized. But if you've correctly initialized the object and subsequently only ever modified it through API functions, then abort is guaranteed to succeed.

> after you get an PSA_ERROR_BAD_STATE, all bets are off. That's not true. All bets are off if you have a bad state because the object was corrupted or never initialized. But if you've correctly initialized the object and subsequently only ever modified it through API functions, then abort is guaranteed to succeed.
Patater commented 2019-02-19 07:50:58 -06:00 (Migrated from github.com)

@NirSonnenschein Yes, it likely would. Good point.

I'll change my example. How about if you call cipher finish without first setting an IV? Finish would fail with PSA_ERROR_BAD_STATE, but memory would have been allocated during cipher setup.

@NirSonnenschein Yes, it likely would. Good point. I'll change my example. How about if you call cipher finish without first setting an IV? Finish would fail with `PSA_ERROR_BAD_STATE`, but memory would have been allocated during cipher setup.
Patater commented 2019-02-19 07:54:42 -06:00 (Migrated from github.com)

Thanks for the clarification, Gilles. I'll switch from using memset to all 0 between bad order tests and use abort instead.

Also note that one isn't guaranteed to get BAD_STATE if the context is corrupt or never initialized, so I'm not sure why we should be concerned, as per your comment: https://github.com/ARMmbed/mbed-crypto/pull/58#discussion_r257349820, about calling abort if BAD_STATE is encountered in these sorts of "well-formed" situations.

Can we conclude that it is OK to call abort on BAD_STATE? Are there cases we shouldn't call abort given we've gotten a BAD_STATE?

Thanks for the clarification, Gilles. I'll switch from using memset to all 0 between bad order tests and use abort instead. Also note that one isn't guaranteed to get BAD_STATE if the context is corrupt or never initialized, so I'm not sure why we should be concerned, as per your comment: https://github.com/ARMmbed/mbed-crypto/pull/58#discussion_r257349820, about calling abort if BAD_STATE is encountered in these sorts of "well-formed" situations. Can we conclude that it is OK to call abort on BAD_STATE? Are there cases we shouldn't call abort given we've gotten a BAD_STATE?
gilles-peskine-arm commented 2019-02-19 08:04:43 -06:00 (Migrated from github.com)

https://github.com/ARMmbed/mbed-crypto/pull/58#discussion_r257349820 was about calling abort internally, not about calling it from the application. I distinguished two cases: well-formed situations, where it's debatable whether the right behavior is to abort or to not modify the object, and non-well-formed situations, where abort could make things worse.

In application code, you should always initialize your memory, and you should always call abort unless you have a guarantee that the object is in its initial state (e.g. because you never set it up or the last function that you applied to it was a finish function).

https://github.com/ARMmbed/mbed-crypto/pull/58#discussion_r257349820 was about calling abort internally, not about calling it from the application. I distinguished two cases: well-formed situations, where it's debatable whether the right behavior is to abort or to not modify the object, and non-well-formed situations, where abort could make things worse. In application code, you should always initialize your memory, and you should always call abort unless you have a guarantee that the object is in its initial state (e.g. because you never set it up or the last function that you applied to it was a finish function).
Patater commented 2019-02-19 08:08:48 -06:00 (Migrated from github.com)

Rebase incoming to switch from memset to abort between bad order tests.

Rebase incoming to switch from memset to abort between bad order tests.
Patater commented 2019-02-19 08:10:37 -06:00 (Migrated from github.com)

Rebased. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-3

Rebased. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-3
Patater commented 2019-02-19 08:12:12 -06:00 (Migrated from github.com)

Forgot to apply a fixup commit. Rebasing again.

Forgot to apply a fixup commit. Rebasing again.
Patater commented 2019-02-19 08:13:35 -06:00 (Migrated from github.com)

Rebased to apply fixup commit. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-4

Rebased to apply fixup commit. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-4
Patater commented 2019-02-19 08:13:53 -06:00 (Migrated from github.com)

All done rebasing. Ready to review.

All done rebasing. Ready to review.
Patater commented 2019-02-19 09:14:26 -06:00 (Migrated from github.com)

Added commits to address https://github.com/ARMmbed/mbed-crypto/pull/58#discussion_r257349820

Will apply as fixup with a rebase if review goes well

Added commits to address https://github.com/ARMmbed/mbed-crypto/pull/58#discussion_r257349820 Will apply as fixup with a rebase if review goes well
Patater commented 2019-02-19 09:51:08 -06:00 (Migrated from github.com)

This is getting a bit too messy. I'm going to have to rebase before review.

This is getting a bit too messy. I'm going to have to rebase before review.
Patater commented 2019-02-19 10:12:35 -06:00 (Migrated from github.com)

Rebased to apply fixups and make some style improvements. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-5

Rebased to apply fixups and make some style improvements. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-5
Patater commented 2019-02-19 10:12:41 -06:00 (Migrated from github.com)

Rebasing done.

Rebasing done.
Patater commented 2019-02-20 03:19:00 -06:00 (Migrated from github.com)

Will rebase to apply fixup commit now

Will rebase to apply fixup commit now
Patater commented 2019-02-20 03:25:02 -06:00 (Migrated from github.com)

Rebased to apply fixup and fix CI issues with depend_hashes.pl.

Rebased to apply fixup and fix CI issues with `depend_hashes.pl`.
Patater commented 2019-02-20 03:25:37 -06:00 (Migrated from github.com)

Rebasing and rework done for now.

Rebasing and rework done for now.
Patater commented 2019-02-20 04:41:54 -06:00 (Migrated from github.com)

This has CI failures. I'll be doing some rework to address this.

This has CI failures. I'll be doing some rework to address this.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-02-20 04:43:49 -06:00
library/psa_crypto.c
@ -1373,7 +1379,13 @@ psa_status_t psa_hash_setup( psa_hash_operation_t *operation,
psa_algorithm_t alg )
gilles-peskine-arm (Migrated from github.com) commented 2019-02-20 04:43:49 -06:00

Style (here and in a few other places):

    if ( operation->alg != 0 )
Style (here and in a few other places): ```suggestion if ( operation->alg != 0 ) ```
gilles-peskine-arm commented 2019-02-20 04:44:42 -06:00 (Migrated from github.com)

I've given the PR a first glance and it looks like it's going in the right direction. I'll hold doing a detailed code review until you've finished the rework.

I've given the PR a first glance and it looks like it's going in the right direction. I'll hold doing a detailed code review until you've finished the rework.
Patater commented 2019-02-20 04:54:41 -06:00 (Migrated from github.com)

We've uncovered pre-existing bugs where we didn't initialize the operation context before calling setup. Rebasing to add commits to fix these issues.

We've uncovered pre-existing bugs where we didn't initialize the operation context before calling setup. Rebasing to add commits to fix these issues.
Patater commented 2019-02-20 04:55:43 -06:00 (Migrated from github.com)

Rebased to add commits to fix uninitialized context bugs and edit commits with bad style. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-8

Rebased to add commits to fix uninitialized context bugs and edit commits with bad style. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-8
Patater commented 2019-02-20 07:35:48 -06:00 (Migrated from github.com)

CI passed. Ready for review.

CI passed. Ready for review.
gilles-peskine-arm (Migrated from github.com) requested changes 2019-02-20 07:49:39 -06:00
gilles-peskine-arm (Migrated from github.com) left a comment

psa_mac_update is not consistent with the other update/finish functions wrt an argument in initial state: it calls abort. This is not an absolute blocker because we still haven't decided what behavior the specification will mandate, but I'd prefer the implementation to be consistent.

`psa_mac_update` is not consistent with the other update/finish functions wrt an argument in initial state: it calls abort. This is not an absolute blocker because we still haven't decided what behavior the specification will mandate, but I'd prefer the implementation to be consistent.
library/cipher.c
@ -1238,3 +1238,3 @@
psa_status_t status;
psa_cipher_operation_t cipher_op;
psa_cipher_operation_t cipher_op = PSA_CIPHER_OPERATION_INIT;
size_t part_len;
gilles-peskine-arm (Migrated from github.com) commented 2019-02-20 07:09:44 -06:00

Why wasn't this caught by CI? Are we missing a test case or a test configuration?

Why wasn't this caught by CI? Are we missing a test case or a test configuration?
library/psa_crypto.c
@ -2232,6 +2247,11 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation,
{
psa_status_t status;
gilles-peskine-arm (Migrated from github.com) commented 2019-02-20 07:24:27 -06:00

Style: missing space before ) (here and elsewhere)

Style: missing space before `)` (here and elsewhere)
tests/suites/test_suite_psa_crypto.function
@ -1960,6 +1961,14 @@ void hash_operation_init( )
gilles-peskine-arm (Migrated from github.com) commented 2019-02-20 07:39:53 -06:00

What does “default hash operation” mean? I understand from context, but this is the code explaining the comment whereas it should be the other way round. Suggestion: “freshly-initialized”

What does “default hash operation” mean? I understand from context, but this is the code explaining the comment whereas it should be the other way round. Suggestion: “freshly-initialized”
tests/suites/test_suite_psa_crypto.function
@ -2016,0 +2038,4 @@
PSA_ASSERT( psa_hash_abort( &operation ) );
/* Call verify after finish. */
PSA_ASSERT( psa_hash_setup( &operation, alg ) );
gilles-peskine-arm (Migrated from github.com) commented 2019-02-20 07:46:50 -06:00

This is getting to be a long series of ad hoc tests. Have we reached the point where they would be easier to write and easier to read if we made the code data-driven? That would make it easier to add other sequences.

for(c in string) {
    switch(c) {
    case 'a': ret = abort();
    case 'u': ret = update();
    ...
    }
    TEST_EQUAL(ret, c is last ? PSA_SUCCESS : expected_last_status);
}
abort();
setup(); update(); finish(); // test that the operation object is usable after abort
This is getting to be a long series of ad hoc tests. Have we reached the point where they would be easier to write and easier to read if we made the code data-driven? That would make it easier to add other sequences. ``` for(c in string) { switch(c) { case 'a': ret = abort(); case 'u': ret = update(); ... } TEST_EQUAL(ret, c is last ? PSA_SUCCESS : expected_last_status); } abort(); setup(); update(); finish(); // test that the operation object is usable after abort ```
tests/suites/test_suite_psa_crypto.function
@ -2016,0 +2044,4 @@
TEST_EQUAL( psa_hash_verify( &operation,
valid_hash, sizeof( valid_hash ) ),
PSA_ERROR_BAD_STATE );
PSA_ASSERT( psa_hash_abort( &operation ) );
gilles-peskine-arm (Migrated from github.com) commented 2019-02-20 07:38:11 -06:00

Technically, you're calling update after abort. This is equivalent to update without setup only if abort behaves correctly.

Technically, you're calling update after abort. This is equivalent to update without setup only if abort behaves correctly.
tests/suites/test_suite_psa_crypto.function
gilles-peskine-arm (Migrated from github.com) commented 2019-02-20 07:43:00 -06:00

Leftover from a previous iteration.

Leftover from a previous iteration.
Patater (Migrated from github.com) reviewed 2019-02-20 08:14:47 -06:00
library/cipher.c
@ -1238,3 +1238,3 @@
psa_status_t status;
psa_cipher_operation_t cipher_op;
psa_cipher_operation_t cipher_op = PSA_CIPHER_OPERATION_INIT;
size_t part_len;
Patater (Migrated from github.com) commented 2019-02-20 08:14:47 -06:00

This was caught by CI, but it wasn't a failure until we started checking that the context is initialized before allowing setup() (by fixing #10). Same with the X.509 issues. all.sh catches this when it runs "make test" when using the MBEDTLS_USE_PSA_CRYPTO option.

This was caught by CI, but it wasn't a failure until we started checking that the context is initialized before allowing setup() (by fixing #10). Same with the X.509 issues. all.sh catches this when it runs "make test" when using the `MBEDTLS_USE_PSA_CRYPTO` option.
Patater (Migrated from github.com) reviewed 2019-02-20 08:15:06 -06:00
library/psa_crypto.c
@ -2232,6 +2247,11 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation,
{
psa_status_t status;
Patater (Migrated from github.com) commented 2019-02-20 08:15:06 -06:00

Gah, I thought I fixed all of these...

Gah, I thought I fixed all of these...
Patater (Migrated from github.com) reviewed 2019-02-20 08:16:07 -06:00
tests/suites/test_suite_psa_crypto.function
@ -2016,0 +2044,4 @@
TEST_EQUAL( psa_hash_verify( &operation,
valid_hash, sizeof( valid_hash ) ),
PSA_ERROR_BAD_STATE );
PSA_ASSERT( psa_hash_abort( &operation ) );
Patater (Migrated from github.com) commented 2019-02-20 08:16:07 -06:00

Would you recommend we call both abort and memset the context to 0? I'm depending on abort setting the context to 0, and if it doesn't, the test will fail.

Would you recommend we call both abort and memset the context to 0? I'm depending on abort setting the context to 0, and if it doesn't, the test will fail.
Patater (Migrated from github.com) reviewed 2019-02-20 08:16:31 -06:00
tests/suites/test_suite_psa_crypto.function
@ -1960,6 +1961,14 @@ void hash_operation_init( )
Patater (Migrated from github.com) commented 2019-02-20 08:16:31 -06:00

I've also seen us use "blank". I'll change to freshly-initialized.

I've also seen us use "blank". I'll change to freshly-initialized.
Patater (Migrated from github.com) reviewed 2019-02-20 08:21:06 -06:00
tests/suites/test_suite_psa_crypto.function
Patater (Migrated from github.com) commented 2019-02-20 08:21:06 -06:00

Removed

Removed
gilles-peskine-arm (Migrated from github.com) reviewed 2019-02-20 08:21:32 -06:00
tests/suites/test_suite_psa_crypto.function
@ -2016,0 +2044,4 @@
TEST_EQUAL( psa_hash_verify( &operation,
valid_hash, sizeof( valid_hash ) ),
PSA_ERROR_BAD_STATE );
PSA_ASSERT( psa_hash_abort( &operation ) );
gilles-peskine-arm (Migrated from github.com) commented 2019-02-20 08:21:31 -06:00

If abort works properly, it doesn't matter. It's only a matter of how hard it will be to investigate a test failure that is ultimately due to abort returning the expected status but not having done its job properly.

I would prefer to make each unit test independent. But it's also good to have a stress test where a single object is reused.

If abort works properly, it doesn't matter. It's only a matter of how hard it will be to investigate a test failure that is ultimately due to abort returning the expected status but not having done its job properly. I would prefer to make each unit test independent. But it's also good to have a stress test where a single object is reused.
Patater (Migrated from github.com) reviewed 2019-02-20 08:22:57 -06:00
tests/suites/test_suite_psa_crypto.function
@ -2016,0 +2038,4 @@
PSA_ASSERT( psa_hash_abort( &operation ) );
/* Call verify after finish. */
PSA_ASSERT( psa_hash_setup( &operation, alg ) );
Patater (Migrated from github.com) commented 2019-02-20 08:22:57 -06:00

I did have this thought as well, but decided it wasn't worth the time yet.

I did have this thought as well, but decided it wasn't worth the time yet.
Patater commented 2019-02-20 09:29:31 -06:00 (Migrated from github.com)

Rebasing to address review feedback.

Rebasing to address review feedback.
Patater (Migrated from github.com) reviewed 2019-02-20 09:32:23 -06:00
tests/suites/test_suite_psa_crypto.function
@ -2016,0 +2044,4 @@
TEST_EQUAL( psa_hash_verify( &operation,
valid_hash, sizeof( valid_hash ) ),
PSA_ERROR_BAD_STATE );
PSA_ASSERT( psa_hash_abort( &operation ) );
Patater (Migrated from github.com) commented 2019-02-20 09:32:23 -06:00

I've raised #66 to add targeted abort tests.

I've raised #66 to add targeted abort tests.
Patater commented 2019-02-20 09:33:29 -06:00 (Migrated from github.com)

Rebased. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-9

Rebased. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-9
Patater commented 2019-02-20 11:40:23 -06:00 (Migrated from github.com)

Rebased to add a commit that returns bad state without aborting in more cases. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-12

Rebased to add a commit that returns bad state without aborting in more cases. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-12
gilles-peskine-arm (Migrated from github.com) approved these changes 2019-02-21 08:58:18 -06:00
yanesca (Migrated from github.com) reviewed 2019-02-21 09:40:56 -06:00
library/ssl_tls.c
@ -6531,3 +6531,3 @@
size_t hash_size;
psa_hash_operation_t sha256_psa;
psa_hash_operation_t sha256_psa = PSA_HASH_OPERATION_INIT;
psa_status_t status;
yanesca (Migrated from github.com) commented 2019-02-21 09:40:56 -06:00

Using PSA_HASH_OPERATION_INIT, psa_hash_operation_init(); or simply = 0 are all valid ways to do initialisation, right?

What are the advantages of different methods? In particular, why are we using on line 1449 and 1492 psa_hash_operation_init(); and PSA_HASH_OPERATION_INIT here?

Using `PSA_HASH_OPERATION_INIT`, `psa_hash_operation_init();` or simply `= 0` are all valid ways to do initialisation, right? What are the advantages of different methods? In particular, why are we using on line 1449 and 1492 `psa_hash_operation_init();` and `PSA_HASH_OPERATION_INIT` here?
gilles-peskine-arm (Migrated from github.com) reviewed 2019-02-21 09:48:35 -06:00
library/ssl_tls.c
@ -6531,3 +6531,3 @@
size_t hash_size;
psa_hash_operation_t sha256_psa;
psa_hash_operation_t sha256_psa = PSA_HASH_OPERATION_INIT;
psa_status_t status;
gilles-peskine-arm (Migrated from github.com) commented 2019-02-21 09:48:34 -06:00

{0} and INIT (macro) can be used to initialize global variables, but init() (function) can't. {0} is generic but some compilers warn that this implicitly initializes other fields to 0. init() is the only one that's typed, so you'll get an error if you use it in the wrong context. memset to 0 is also defined as equivalent because it allows applications to use e.g. calloc, and in practice it produces the same value as {0} on virtually all platforms.

In most cases, the choice is indifferent.

`{0}` and `INIT` (macro) can be used to initialize global variables, but `init()` (function) can't. `{0}` is generic but some compilers warn that this implicitly initializes other fields to 0. `init()` is the only one that's typed, so you'll get an error if you use it in the wrong context. `memset` to 0 is also defined as equivalent because it allows applications to use e.g. `calloc`, and in practice it produces the same value as `{0}` on virtually all platforms. In most cases, the choice is indifferent.
Patater (Migrated from github.com) reviewed 2019-02-21 09:58:05 -06:00
library/ssl_tls.c
@ -6531,3 +6531,3 @@
size_t hash_size;
psa_hash_operation_t sha256_psa;
psa_hash_operation_t sha256_psa = PSA_HASH_OPERATION_INIT;
psa_status_t status;
Patater (Migrated from github.com) commented 2019-02-21 09:58:04 -06:00

PSA_HASH_OPERATION_INIT is all static, so you can use it for static globals. The inline function can be used where functions can be used, which is a few less places, but it uses the static initializer under-the-hood. = {0} is usually ok, but some compilers will complain about missing field initializers.

I personally prefer using the macro everywhere, but previous authors inside library/ssl_tls.c liked the function. There is no functional difference in this case.

`PSA_HASH_OPERATION_INIT` is all static, so you can use it for static globals. The inline function can be used where functions can be used, which is a few less places, but it uses the static initializer under-the-hood. `= {0}` is usually ok, but some compilers will complain about missing field initializers. I personally prefer using the macro everywhere, but previous authors inside library/ssl_tls.c liked the function. There is no functional difference in this case.
yanesca (Migrated from github.com) approved these changes 2019-02-21 11:32:18 -06:00
yanesca (Migrated from github.com) left a comment

Looks good to me!

Looks good to me!
Patater commented 2019-02-21 11:36:50 -06:00 (Migrated from github.com)

CI failure is only FreeBSD timing test, a known-flaky test.

CI failure is only FreeBSD timing test, a known-flaky test.
Sign in to join this conversation.
Reviewers
No reviewers
gilles-peskine-arm
yanesca
Labels
Clear labels   
api-spec
Issue or PR about the PSA specifications

  
bug
Something isn't working

  
CLA requested

   
CLA valid

   
compliance
Discrepancy between the library and the PSA Crypto API specification

  
DO NOT MERGE
The PR is not intended to be merged (yet). Usually used for a review of worked in progress.

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
feature branch
This issue or PR is for a short-term feature branch which doesn't have its own label.

  
good first issue
Good for newcomers

  
HEADER FORMATTING ISSUE

   
help wanted
Extra attention is needed

  
Information

   
invalid
This doesn't seem right

  
Jira status: AWAITING REVIEW

   
Jira status: CLOSED

   
Jira status: DONE

   
Jira status: IN PROGRESS

   
Jira status: IN REVIEW

   
Jira status: OPEN

   
Jira status: REOPENED

   
Jira status: RESOLVED

   
mirrored

   
mirror internally

   
needs: backports
Needs backports to Mbed TLS branches

  
needs: ci
Needs a passing full CI run

  
needs: design review

   
needs: preceding PR
Requires another PR to be merged first

  
needs: review
The pull request is ready for review. This generally means that it has no known issues.

  
needs: work
The pull request needs rework before it can be merged.

  
notabug
The bug is not considered to be a defect, but by design.

  
open for community contribution

   
Product Backlog

   
question
Further information is requested

  
ready for merge
Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only.

  
security
Security expertise requested in review

  
stale
This PR is not being actively worked on. It can be considered abandoned work.

  
type: bug

   
type: enhancement

   
type: question

   
web pages
The /docs directory in the master branch is shown on https://armmbed.github.io/mbed-crypto

  
wontfix
The cost of fixing the bug or managing it in the bug list is not justified.

  
writing
Documentation wording and formatting

No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-ARMmbed/mbed-crypto!58
No description provided.