[tests] Fix incorrect memory_cleanse(…) call in crypto_tests.cpp #10912

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:arrays-are-not-pointers changing 1 files +2 −2

practicalswift commented at 10:03 PM on July 22, 2017: contributor

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.
fanquake added the label Tests on Jul 22, 2017
in src/wallet/test/crypto_tests.cpp:29 in 5b8fdddaf6 outdated
```
  22 | @@ -23,14 +23,7 @@ bool OldSetKeyFromPassphrase(const SecureString& strKeyData, const std::vector<u
  23 |      if (nDerivationMethod == 0)
  24 |          i = EVP_BytesToKey(EVP_aes_256_cbc(), EVP_sha512(), &chSalt[0],
  25 |                            (unsigned char *)&strKeyData[0], strKeyData.size(), nRounds, chKey, chIV);
  26 | -
  27 | -    if (i != (int)WALLET_CRYPTO_KEY_SIZE)
  28 | -    {
  29 | -        memory_cleanse(chKey, sizeof(chKey));
```
jonasschnelli commented at 7:49 AM on July 24, 2017:

Change the sizeof to WALLET_CRYPTO_KEY_SIZE, and WALLET_CRYPTO_IV_SIZE?

practicalswift commented at 7:53 AM on July 24, 2017:

@jonasschnelli Yes, that is the correct fix if we want to keep the memory_cleanse(…)-calls. Do we? :-) If so, I'll fix them!

jonasschnelli commented at 8:49 AM on July 24, 2017:

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.
jonasschnelli commented at 7:55 AM on July 24, 2017: contributor

Nice catch! AFAIK this is the only place where (we intended) to test memory_cleanse. Maybe keep it and switch the sizeof's to the correct constants?
sipa commented at 7:56 AM on July 24, 2017: member

Having memory_cleanse in a test seems pointless.
practicalswift commented at 8:16 AM on July 24, 2017: contributor

Perhaps memory_cleanse deserves a dedicated test (a subject for another PR)? :-)

If so I'll leave this PR as-is.
jonasschnelli commented at 8:47 AM on July 24, 2017: contributor

Agree with a dedicated test. @sipa is right that cleaning memory in test is pointless, although I think it makes sense to test the memory_cleanse function (to eventually make sure compiler doesn't optimise out the cleanse [volatile] function)?
sipa commented at 8:52 AM on July 24, 2017: member

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)
jonasschnelli commented at 8:55 AM on July 24, 2017: contributor

If the memory_cleanse would somehow fail to do its job, I don't think this would cause any test to fail?

Yes. We should have a dedicated test for memory_cleanse then. Just executing the code has only little advantages (crashes/exceptions will be detected, memory leaks when running with valgrind, etc.).
practicalswift force-pushed on Jul 24, 2017
practicalswift commented at 9:12 AM on July 24, 2017: contributor

I've now updated the PR: now calling memory_cleanse(…) with the expected args.
jonasschnelli commented at 9:19 AM on July 24, 2017: contributor

utACK 17a1f639c906a14e02ac03b51b7e030f3f47a86c
TheBlueMatt commented at 6:32 PM on July 25, 2017: member

utACK. Yes, lets actually /test/ memory_cleanse at some point.
MarcoFalke commented at 7:39 PM on July 25, 2017: member

Please amend the commit message/pull request body and pull title.
practicalswift renamed this:
~~[tests] Remove incorrect memory_cleanse(…) call in crypto_tests.cpp~~
[tests] Fix incorrect memory_cleanse(…) call in crypto_tests.cpp
on Jul 25, 2017
practicalswift force-pushed on Jul 25, 2017
practicalswift commented at 10:05 PM on July 25, 2017: contributor

@MarcoFalke Done! :-)

[tests] Fix incorrect memory_cleanse(…) call in crypto_tests.cpp

chKey and chIV are pointers, not arrays :-)

Probably the result of copy-pasting of old code which was
operating on arrays instead of pointers.

065039da1f

practicalswift force-pushed on Jul 25, 2017
laanwj merged this on Jul 26, 2017
laanwj closed this on Jul 26, 2017
laanwj referenced this in commit 5c8eb7916d on Jul 26, 2017
laanwj commented at 10:10 AM on July 26, 2017: member

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.
MarcoFalke commented at 10:37 AM on July 30, 2017: member

post mege utACK 065039d
PastaPastaPasta referenced this in commit 07d08a8224 on Aug 2, 2019
PastaPastaPasta referenced this in commit ad531f7689 on Aug 6, 2019
barrystyle referenced this in commit 148e556f9f on Jan 22, 2020
practicalswift deleted the branch on Apr 10, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels