[tests] Fix incorrect memory_cleanse(…) call in crypto_tests.cpp#10912
[tests] Fix incorrect memory_cleanse(…) call in crypto_tests.cpp#10912laanwj merged 1 commit intobitcoin:masterfrom
Conversation
src/wallet/test/crypto_tests.cpp
Outdated
There was a problem hiding this comment.
Change the sizeof to WALLET_CRYPTO_KEY_SIZE, and WALLET_CRYPTO_IV_SIZE?
There was a problem hiding this comment.
@jonasschnelli Yes, that is the correct fix if we want to keep the memory_cleanse(…)-calls. Do we? :-) If so, I'll fix them!
There was a problem hiding this comment.
I don't care that much. I personably would try to keep it even if cleaning memory is pointless because I generally follow "test more is better"-approach.
|
Nice catch! |
|
Having memory_cleanse in a test seems pointless. |
|
Perhaps If so I'll leave this PR as-is. |
|
Agree with a dedicated test. |
|
Testing more is better, but I think this code is not testing more. If the memory_cleanse would somehow fail to do its job, I don't think this would cause any test to fail? (I haven't looked closely, please correct me if I'm wrong) |
Yes. We should have a dedicated test for |
practicalswift
force-pushed
the
arrays-are-not-pointers
branch
from
5b8fddd to
17a1f63
Compare
|
I've now updated the PR: now calling |
|
utACK 17a1f639c906a14e02ac03b51b7e030f3f47a86c |
|
utACK. Yes, lets actually /test/ memory_cleanse at some point. |
|
Please amend the commit message/pull request body and pull title. |
practicalswift
changed the title
practicalswift
force-pushed
the
arrays-are-not-pointers
branch
from
17a1f63 to
8ca59d8
Compare
|
@MarcoFalke Done! :-) |
chKey and chIV are pointers, not arrays :-) Probably the result of copy-pasting of old code which was operating on arrays instead of pointers.
practicalswift
force-pushed
the
arrays-are-not-pointers
branch
from
8ca59d8 to
065039d
Compare
…tests.cpp 065039d [tests] Fix incorrect memory_cleanse(…) call in crypto_tests.cpp (practicalswift) Pull request description: `chKey` and `chIV` are pointers, not arrays :-) Probably the result of copy-pasting of old code where the code was operating on arrays instead of pointers. If I'm reading the code correctly the absence/presence of these `memory_cleanse(…)` calls won't alter the outcome of the test in question (`TestPassphraseSingle`) even if fixed. Therefore removing. Tree-SHA512: a053b2817bedf6ef889744e546ce9a0f165dee94aef6850d9d6a6bb05b0018789597371ecf154a4aec8588c0ef5626ef08c23c35e35927f6b0497b5f086146fe
|
Apparently this tests nothing, if this error was not detected! But it's clear that the new code is what the intention was so meh-ACK. |
|
post mege utACK 065039d |
…crypto_tests.cpp 065039d [tests] Fix incorrect memory_cleanse(…) call in crypto_tests.cpp (practicalswift) Pull request description: `chKey` and `chIV` are pointers, not arrays :-) Probably the result of copy-pasting of old code where the code was operating on arrays instead of pointers. If I'm reading the code correctly the absence/presence of these `memory_cleanse(…)` calls won't alter the outcome of the test in question (`TestPassphraseSingle`) even if fixed. Therefore removing. Tree-SHA512: a053b2817bedf6ef889744e546ce9a0f165dee94aef6850d9d6a6bb05b0018789597371ecf154a4aec8588c0ef5626ef08c23c35e35927f6b0497b5f086146fe
7 participants
chKeyandchIVare pointers, not arrays :-)Probably the result of copy-pasting of old code where the code was operating on arrays instead of pointers.
If I'm reading the code correctly the absence/presence of these
memory_cleanse(…)calls won't alter the outcome of the test in question (TestPassphraseSingle) even if fixed. Therefore removing.