/bitcoin

Avoid dereference-of-casted-pointer #10760

Merged
merged 1 commit into from Jul 15, 2017
+26 −18

Conversation

Reviewers

@promag promag

@dcousens dcousens

@gmaxwell gmaxwell

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
9 participants
@sipa @practicalswift @promag @TheBlueMatt @laanwj @jonasschnelli @dcousens @gmaxwell @fanquake
@sipa
Owner

sipa commented Jul 7, 2017

And prefer a static_cast to the intended reference type.

fanquake added the Refactoring label Jul 7, 2017

src/core_memusage.h
@@ -10,7 +10,7 @@
#include "memusage.h"
static inline size_t RecursiveDynamicUsage(const CScript& script) {
- return memusage::DynamicUsage(*static_cast<const CScriptBase*>(&script));
+ return memusage::DynamicUsage(static_cast<const CScriptBase&>(script));
@promag

promag Jul 7, 2017

Contributor

Remove static cast?

  return memusage::DynamicUsage(script);

In my system it builds fine.

@sipa

sipa Jul 7, 2017

Owner

Nice catch, fixed.

src/wallet/walletdb.cpp
@@ -94,23 +94,23 @@ bool CWalletDB::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey)
bool CWalletDB::WriteCScript(const uint160& hash, const CScript& redeemScript)
{
- return WriteIC(std::make_pair(std::string("cscript"), hash), *(const CScriptBase*)(&redeemScript), false);
+ return WriteIC(std::make_pair(std::string("cscript"), hash), static_cast<const CScriptBase&>(redeemScript), false);
@promag

promag Jul 7, 2017 • edited

Contributor

Removing the cast, shouldn't the compiler resolve to:

template<typename Stream, unsigned int N, typename T>
inline void Serialize(Stream& os, const prevector<N, T>& v)

If not I think we could implement CScript::Serialize to call super and drop all of these casts.

@sipa

sipa Jul 7, 2017

Owner

I somehow remembered that this didn't work, but it seems I was wrong. Fixed.

@practicalswift
Contributor

practicalswift commented Jul 7, 2017

Concept ACK 8be4385

Regarding the static_cast preference, see also PR #10498: "Use static_cast instead of C-style casts for non-fundamental types".
@promag
Contributor

promag commented Jul 7, 2017

tACK.
@gmaxwell

gmaxwell approved these changes Jul 9, 2017
View changes

utACK

@TheBlueMatt
Contributor

TheBlueMatt commented Jul 10, 2017

utACK 0aadc11
@laanwj
Owner

laanwj commented Jul 10, 2017

And prefer a static_cast to the intended reference type.

What is the rationale here? Just code cleanup?
@sipa
Owner

sipa commented Jul 10, 2017

Yes, cleanup. The existing practice risks hiding some bugs.
@jonasschnelli
Member

jonasschnelli commented Jul 13, 2017

utACK 0aadc11

@sipa sipa merged commit 0aadc11 into bitcoin:master Jul 15, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sipa sipa added a commit that referenced this pull request Jul 15, 2017

@sipa sipa Merge #10760: Avoid dereference-of-casted-pointer 
0aadc11 Avoid dereference-of-casted-pointer (Pieter Wuille)

Pull request description:

  And prefer a static_cast to the intended reference type.

Tree-SHA512: e83b20023a4dca6029b46f7040a8a6fd54e1b42112ec0c87c3c3b567ed641de97a9e2335b57a2efb075491f641e5b977bc226a474276bea0c3c3c71d8d6ac54d
This commit was signed with a verified signature.
@sipa sipa Pieter Wuille
GPG key ID: A636E97631F767E0 Learn about signing commits
10b22e3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment