fix wrong memcmp() usage in CKey::operator== #3176

pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:key changing 1 files +4 −3

Diapolo commented at 10:22 AM on October 28, 2013: none
- add a check for CKey::size() of a and b (size can be 0 or 32)
- change the fixed value in memcmp() to use a.size() instead
- fixes #3090
in src/key.h:None in 16b9dff891 outdated
```
 204 | @@ -205,7 +205,8 @@ class CKey {
 205 |      }
 206 |  
 207 |      friend bool operator==(const CKey &a, const CKey &b) {
 208 | -        return a.fCompressed == b.fCompressed && memcmp(&a.vch[0], &b.vch[0], 32);
 209 | +        return a.fCompressed == b.fCompressed &&
 210 | +               memcmp(&a.vch[0], &b.vch[0], a.size()) == 0;
```
laanwj commented at 10:59 AM on October 28, 2013:

If you're not trusting enough to assume that the size is 32, you need to compare the sizes of a and b before calling memcmp as well.

Diapolo commented at 11:17 AM on October 28, 2013:

Indeed... I just saw that unsigned int size() const { return (fValid ? 32 : 0); } can be 32 or 0, which made me think it would be a good idea to also use the real size.
Diapolo commented at 11:22 AM on October 28, 2013: none
Updated:
- add a check for CKey::size() of a and b (size can be 0 or 32)
laanwj commented at 11:26 AM on October 28, 2013: member

ACK
laanwj commented at 12:16 PM on October 28, 2013: member

Looks like something is wrong with the pull tester.
Diapolo commented at 12:20 PM on October 28, 2013: none

Indeed @TheBlueMatt, can you take a look?

fix wrong memcmp() usage in CKey::operator==

- add a check for CKey::size() of a and b (size can be 0 or 32)
- change the fixed value in memcmp() to use a.size() instead
- fixes #3090

a39967401e

BitcoinPullTester commented at 4:44 PM on October 28, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a39967401e0ffb22649b36782435bffdec980255 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
jgarzik commented at 5:06 PM on October 28, 2013: contributor

Why change hardcoded 32 to size check + variable size memcmp? Defeats compiler builtins, but meh, ACK anyway I suppose.
laanwj commented at 5:17 PM on October 28, 2013: member

@jgarzik agreed, though this has the advantage that it (indirectly) compares the valid status of the key, which was not considered before.

And in any case this is a function that's not used at all so it's not exactly performance critical, could even argue for removing it.
sipa commented at 6:43 PM on October 28, 2013: member

ACK
jgarzik commented at 7:37 PM on October 28, 2013: contributor

If it's dead code, remove it.
gavinandresen commented at 1:11 AM on October 30, 2013: contributor

Agree with @jgarzik, remove the dead code entirely, don't try to fix it.
Diapolo commented at 7:51 AM on October 30, 2013: none

Ouch, I'm sorry to say but this code IS used in: https://github.com/bitcoin/bitcoin/blob/master/src/key.h#L295
gavinandresen commented at 8:03 AM on October 30, 2013: contributor

... which is used by the bip32 unit tests. Got it. ACK.
gavinandresen referenced this in commit 42a12f22d6 on Oct 30, 2013
gavinandresen merged this on Oct 30, 2013
gavinandresen closed this on Oct 30, 2013
laanwj commented at 9:16 AM on October 30, 2013: member

Is it? I made all those functions private once, and managed to compile everything including the tests. Maybe it was introduced recently? In any case, keeping a == operator around is sane, even if it isn't used.
Bushstar referenced this in commit 9bc699ff25 on Apr 8, 2020
Bushstar referenced this in commit 16b6b6f7c2 on Apr 8, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#3090 CodeBug : should compare return value from memcmp with zero.