wallet: Fix a bug where memcmp takes a pointer address as second argument #15083

pull bytting wants to merge 1 commits into bitcoin:master from bytting:20190103-fix-memcmp changing 1 files +1 −1

bytting commented at 4:26 AM on January 3, 2019: contributor

Fix a bug where memcmp takes the address of a pointer as second argument. As far as I can see, the intention with this line of code appears to be comparing two plain arrays.

I have not tested this fix thoroughly.
Fix a bug where memcmp takes a pointer address as second argument cd509e5994
fanquake added the label Wallet on Jan 3, 2019
in src/wallet/db.cpp:56 in cd509e5994
```
  52 | @@ -53,7 +53,7 @@ std::map<std::string, BerkeleyEnvironment> g_dbenvs GUARDED_BY(cs_db); //!< Map
  53 |  
  54 |  bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId& rhs) const
  55 |  {
  56 | -    return memcmp(value, &rhs.value, sizeof(value)) == 0;
  57 | +    return memcmp(value, rhs.value, sizeof(value)) == 0;
```
lavie commented at 5:49 AM on January 3, 2019:

Might I ask, why is this a memcmp at all, when value is basically a u_int8_t?

Wouldn't return rhs.value == value be simpler?

bytting commented at 6:32 AM on January 3, 2019:

It's an array

https://github.com/bitcoin/bitcoin/blob/master/src/wallet/db.h#L30

lavie commented at 6:35 AM on January 3, 2019:

😬sorry about that.

bytting commented at 7:08 AM on January 3, 2019:

No problem :smile:
sipa commented at 8:17 AM on January 3, 2019: member

Taking the address of an array gives a pointer to its first element. It does not give you a pointer-to-a-pointer. I believe both the old and the new code are correct here.
promag commented at 8:25 AM on January 3, 2019: member

utACK cd509e59, at least arguments are "consistent" now.
bytting commented at 9:16 AM on January 3, 2019: contributor

Taking the address of an array gives a pointer to its first element. It does not give you a pointer-to-a-pointer. I believe both the old and the new code are correct here.

That's what I was unsure about. For a static array this may be true, but are you sure it is defined behaviour across compilers?

If it was &rhs.value[0] it would be another matter.
sipa commented at 9:42 AM on January 3, 2019: member

This is not an implementation detail.

C arrays' behavior is specified by the language in this regard. For most operations, arrays automatically decay into a pointer to its first element. But there are a few operations specific to arrays for which this does not happen, including sizeof (which gives you the size of the array, not of a pointer to it), and the & operator, which also gives a pointer to the first element directly, not after first also converting to a pointer. That would be nonsensical anyway, as it'd be returning a pointer to a temporary pointer.
meshcollider added the label Refactoring on Jan 3, 2019
laanwj commented at 1:21 PM on January 3, 2019: member

If this fixes a bug, this needs a test. If not, please change the PR and commit title to say something like clarifying the code.
gmaxwell commented at 5:56 PM on January 3, 2019: contributor

NAK, doesn't fix a bug, doesn't make the code more clear: memcmp (&,&) is perfectly idiomatic code.
bytting commented at 3:06 PM on January 5, 2019: contributor

I take your word for it, this being defined behaviour and idiomatic code.

I'll close this one.
bytting closed this on Jan 5, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels