Add initializers for crypto structs

[Patater]

Add initialization macros and inline functions for all structs in crypto_struct.h. This allows initializing any of these structs by macro, inline function, memset to zero, or static initialization to zero.

Open questions:

• Users have a choice of at least 4 different ways to initialize all these structs. Should we prefer the MACRO initializer in our own code?

Out of scope of this PR:

• Removing any duplicate zeroization done by *_setup functions. We can do that in a follow up PR.
• Updating Mbed TLS to work with the new API change when MBEDTLS_USE_PSA_CRYPTO is set.

[Patater]

Clang 5 is a bit annoying with its -Wmissing-field-initializers. Rework incoming...

Clang 6 is just fine.

[Patater]

Rebased to add initializers that Clang 5.0 is happy with.

[gilles-peskine-arm]

This PR is mostly ok, but:

• A few more documentation changes are needed.
• The new structure initialization tests need to be rethought.

The following defect can be handled in a subsequent PR:

• The xxx_setup functions should check the operation state and return BAD_STATE if the operation has already been initialized (change the implementation, document the check, and test it). This can be done in a follow-up PR.

[gilles-peskine-arm]

It would make sense to test {0} as well, but that requires turning off some annoying compiler warnings.

[gilles-peskine-arm]

This isn't necessarily guaranteed. Mbed TLS does require logical zero to be represented as all-bits-zero for pointers and integers, so the part of the structure that is initialized is guaranteed to be all-bits-zero. However there could be structure or union padding that isn't zeroed. Structure padding is unlikely, but union padding is. If the structure contains a union we don't initialize the longest field of that union, we don't know what the union contains beyond the field we're initializing.

I think it would be better to do operational tests: for each of the permitted ways to initialize a structure, use it and verify that it's treated as a valid initialized “blank” structure. We should do the same with an operation structure that's been finished or aborted, by the way.

These tests are not critical at this point. We can add it to our test backlog.

[Patater]

I know what the standard says, but I'd love to see a platform where this actually breaks. When it does break, I really want to know about it because I'm sure we'll have other subtle portability issues on that platform. This test fails loudly when such a platform shows up.

Our current implementation depends (perhaps beyond what the C standard promises) on being able to initialize an entire struct that contains a union without clang initializer warnings by only initializing the smaller, simpler type (the dummy variable in the union) and not having union padding go uninitialized. It'd be good to have a test to catch if this dependency breaks due to new platforms or toolchains or versions thereof.

[gilles-peskine-arm]

The library assumes that all-bits-zero is a logical zero. This tests makes a wholly different assumption, which is that {0} is all-bits-zero even for a union. It would make sense to compile

union u v = {.x = 0};

to memset(&v, 0, sizeof(v.x)); rather than memset(&v, 0, sizeof(v)), although I can't catch gcc doing it on a toy example.

[Patater]

I agree we should instead test how the structure acts with each initializer, not how its represented, as we technically don't care how it's represented (and couldn't control for its representation portably even if we did). We can do that in another PR.

Regarding your example, I don't believe compilers are allowed to avoid initializing the other fields of the struct or union, but they probably could leave any padding uninitialized. It would be interesting to observe such a case, or to see that the standard requires padding to also have static initialization the same as any other not-specified initializer.

Separate from the fact that we are testing the representation of all values logical zero as all-bits-zero, what our current initializer implementation depends on today is that:

Given:

struct s {
  union {
    unsigned dummy;
    struct complicated {
        unsigned x;
        unsigned y;
#ifdef SOME_CONFIG_OPTION
        unsigned z;
#endif
    };
};

and

struct s s = {0, {0}};

that

s.y == 0

We depend on this because {0, {0}} is easier to have as "the one initializer that works everywhere even with Clang 5 and -Wmissing-field-initializers" instead of multiple different initializers for each of the configuration options. Technically, setting struct s s = {0}; should also work just as well per the standard...

I'll update the test to check that the values are 0 instead of all-bits-zero, even when statically initialized. This is the minimum we depend on. We don't technically require that the entire object is all-bits-zero, just that its values are.

[gilles-peskine-arm]

If you're thinking of testing what the library code relies on, our code doesn't depend on the union part of the operation struct being initialized at all. All we need during initialization is to initialize the alg fields and some other algorithm-independent fields.

But you can't test this from the unit tests, because they need to work with different implementations of crypto_struct.h that have a different layout. When the library is integrated as a PSA service, each operation struct is {psa_handle_t handle;}.

The library never depends on type punning in unions, as far as I know. Doing so is dangerous because C89 didn't allow it and compilers in C89 mode sometimes decide that if you're writing to one union member and reading from another union member, these two can't possibly be aliased. What the library relies on is that if a data structure is initialized to all-bits-zero (by calloc) then all of its integer members have the logical value 0 (which is the case because we also forbid padding bits in integers) and all of its pointer members are null (we require that all-bits-zero is null). These implementation properties (no padding bits, all-bits-zero is null) are tested in selftest.c.

[Patater]

Hmm, I don't think it's actually worth the time to update the test to check for all values logical zero for each of the initializers, given that we'll move to testing that the structs do what we want (vs are what we want). What you've requested is a weakening of a test, because we are testing for something more strict than what the standard allows compilers to do. But, if what we are testing for is true, then our weaker actual dependency will be met as well.

[Patater]

different implementations of crypto_struct.h that have a different layout

Our unit tests are specific to our implementation, not meant to work with other implementations. That's what the ACK is for. But, given that we'd like to use our tests with the service implementation, yes, we can't check that all values are logical zero because that's heavily dependent on the structs defined by crypto_struct.h. That's yet another reason to prefer testing based on what freshly initialized structs do versus how they are represented. Let's make this change to testing what the structs do in a separate PR.

[Patater]

I'd love to see a platform where this actually breaks.

MSVC then proceeded to immediately break. PR to fix this coming soon.

[Patater]

Rebased to add descriptions to commit messages, prefer using macro initializers consistently (key policy was using function initializers), update the getting started doc, revise the doxygen somewhat (more to revise still), and to move the tests.

[Patater]

Rebased to finish updating Doxygen

[Patater]

Rebased to fix style and a doxygen comment.

[gilles-peskine-arm]

Ok except for one thing: in the _init tests, add a comment that explains that this is not testing a requirement of the spec, but a stronger property that implies this requirement, namely, all the permitted ways of initializing a structure lead to the same bit pattern, therefore when other test functions test with one particular initialization method they are also effectively testing the other initialization methods. And also mention that one of the initialization methods, {0}, is omitted because we don't have the infrastructure to suppress the clang compiler warning.

[Patater]

Rebased to add comments about test deficiencies.

[gilles-peskine-arm]

The PR job and a local run of MSan and Valgrind pass except for some DTLS proxy tests which are known to be flaky or time out. Therefore this is good to merge.