Uninitialized array variable #23298
Uninitialized array variable #23298
Conversation
array"key" is uninitialized and it is being read directly in function SipHash_Init() as per the below statements making a way for the garbage values : uint64_t k0 = U8TO64_LE(k); uint64_t k1 = U8TO64_LE(k + 8); CLA: trivial
test/siphash_internal_test.c
Outdated
| @@ -257,7 +257,7 @@ static int test_siphash(int idx) | |||
| static int test_siphash_basic(void) | |||
| { | |||
| SIPHASH siphash = { 0, }; | |||
| unsigned char key[SIPHASH_KEY_SIZE]; | |||
| unsigned char key[SIPHASH_KEY_SIZE] = {0}; | |||
There was a problem hiding this comment.
This isn't needed, its initalized on line 191 before use
There was a problem hiding this comment.
There , it is initialized in the function test_siphash() but not in test_siphash_basic(). In both the functions, it's a local variable . In test_siphash_basic(), it's being passed directly to the following function SipHash_Init().
TEST_true(SipHash_Init(&siphash, key, 0, 0));
There was a problem hiding this comment.
Also, test_siphash_basic() is called at the earliest as suggested by the following code:
int setup_tests(void)
{
ADD_TEST(test_siphash_basic);
ADD_ALL_TESTS(test_siphash, OSSL_NELEM(tests));
return 1;
}
There was a problem hiding this comment.
in test_siphash_basic, it seems whatever garbage is on the stack is being used as a key, and SipHash_Init translates the unsigned char that we pass to two uint64 variables starting at indexes 0 and 8. Given that SIPHASH_KEY_SIZE is defined as 16, it won't overrun the array. It doesn't hurt to initalize it, but what you're doing means we're going to test with a key that is all zeros, which is fine, but probably not whats intended. If it needs to be initalized, it should likely be initalized with some concrete key that is something other than all zeros (though that might not hurt to test for either)
There was a problem hiding this comment.
Uninitialised memory on the stack will either lead to unspecified values or trap representations being used. A trap representation would lead to undefined behaviour, but uint64_t on two's complement systems (in practice all that are likely to be seen) does not have a trap value, so on two's complement systems this won't lead to undefined behaviour.
On one's complement systems it's implementation-defined whether "all ones" is a trap representation, so this could potentially be an issue there.
I agree that "all zeroes" might not be the best value to use - it might be best to bring in the initialisation from test_siphash()
There was a problem hiding this comment.
I think using all-zero key for this particular test is OK. Please just make the variable static const as well.
There was a problem hiding this comment.
Done. Please review.
t8m
added
branch: master
Making the array variable static const as well.
static const unsigned char key[SIPHASH_KEY_SIZE] = {0};
CLA: trivial
t8m
added
the
approval: review pending
|
OK with CLA: trivial |
tom-cosgrove-arm
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
|
Merged to the master, 3.2, 3.1 and 3.0 branches. Thank you for your contribution. |
array"key" is uninitialized and it is being read directly in function SipHash_Init() as per the below statements making a way for the garbage values : uint64_t k0 = U8TO64_LE(k); uint64_t k1 = U8TO64_LE(k + 8); CLA: trivial Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23298) (cherry picked from commit a0826b1)
array"key" is uninitialized and it is being read directly in function SipHash_Init() as per the below statements making a way for the garbage values : uint64_t k0 = U8TO64_LE(k); uint64_t k1 = U8TO64_LE(k + 8); CLA: trivial Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23298)
array"key" is uninitialized and it is being read directly in function SipHash_Init() as per the below statements making a way for the garbage values : uint64_t k0 = U8TO64_LE(k); uint64_t k1 = U8TO64_LE(k + 8); CLA: trivial Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23298) (cherry picked from commit a0826b1)
array"key" is uninitialized and it is being read directly in function SipHash_Init() as per the below statements making a way for the garbage values : uint64_t k0 = U8TO64_LE(k); uint64_t k1 = U8TO64_LE(k + 8); CLA: trivial Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23298) (cherry picked from commit a0826b1)
array"key" is uninitialized and it is being read directly in function SipHash_Init() as per the below statements making a way for the garbage values : uint64_t k0 = U8TO64_LE(k);
uint64_t k1 = U8TO64_LE(k + 8);
CLA: trivial
Checklist