Closed mrdeep1 closed 1 year ago
Not sure, why the compiler warns.
AFAIK,
dtls_ec_key_to_uint32(pub_key_y, key_size, pub_y);
initialize pub_x
. That may be not too easy to detect, though key_size is passed in.
I wonder if it thinks that there is no guarantee on the value of key_size
, and hence possibly not all the 8 words of pub_x
get preset ahead of time.
Yes, the assumptions about the key_size
may be not clear.
Several thoughts here
Need to assert that key_size is not bigger than sizeof(pub_x) etc. Need to assert that key_size % sizeof(unit32_t) == 0 to prevent partial updates Is the key size going to only ever going to be 32 (256 bits)? Is there a need for other key size support?
AFAIK, only secp256r1
is supported.
The encoding may be an issue, see PR #58 .
Need to assert that key_size is not bigger than sizeof(pub_x) etc.
Yes.
Need to assert that key_size % sizeof(unit32_t) == 0 to prevent partial updates
Not sure, see my comment above. Requires more analysis.
Is the key size going to only ever going to be 32 (256 bits)? Is there a need for other key size support?
Not sure. I would consider to add ed25519/x25519 before any other stronger ecc.
Need to assert that key_size % sizeof(unit32_t) == 0 to prevent partial updates
Not sure, see my comment above. Requires more analysis.
Agreed that it does need further checking, but I would think that we are talking about raw keys here, as opposed to the asn.1 representation.
As we have hard-coded the ec key size (in bytes) to be DTLS_EC_KEY_SIZE
elsewhere, it should be fine to replace the usage of 8
for size of variables with (DTLS_EC_KEY_SIZE / sizeof(uint32_t))
in the appropriate places and assert(key_size == DTLS_EC_KEY_SIZE)
to see if that removes the compile complaints (may also need to replace parameter usage of key_size
with DTLS_EC_KEY_SIZE
).
We may use ecc.h - keyLengthInBytes / arrayLength consequently. Maybe rename it first?
keyLengthInBytes
is not currently in use (but arrayLength
is). I certainly prefer upper case #define for clarity that it is not a variable etc. Having 2 (or more) definitions for the same entity is likely to create downstream issues when things get (partially) updated.
Is this still pending or also fixed with #208 ?
Current compilation issue is fixed with #208. I had forgotten about this Issue, but if keysizes get changed, then this needs to get revisited.
Closing for now.
With Ubuntu 22.04, doing
produces warnings about potentially uninitialized variables as per sample
but using
-O2
(the default), all is fine.