refactor: use std:: prefix for std lib funcs #25172

fanquake · 2022-05-19T15:35:49Z

We currently use a mix of std::mem*( and mem*( throughout the codebase. Consolidate to std::mem*(.

Follows up to a comment in #24962:

Maybe switch to std::memmove, which comes with the documentation:

Also includes a change to std::vsnprintf and header changes required to perform compilation.

DrahtBot · 2022-05-20T09:28:26Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26177 (refactor / kernel: Move non-gArgs chainparams functionality to kernel by TheCharlatan)
#26153 (Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa)
#25903 (Use static member functions from class instead of instances by aureleoules)
#25830 (refactor: Replace m_params with chainman.GetParams() by aureleoules)
#25361 (BIP324: Cipher suite by dhruv)
#25152 (refactor: Split util/system into exception, shell, and fs-specific files by Empact)
#24994 (build: Build libbitcoinconsensus from its own convenience library by hebasto)
#23561 (BIP324: Handshake prerequisites by dhruv)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/dbwrapper.cpp

aureleoules · 2022-05-20T17:17:40Z

I believe some were missed:

Git diff

diff --git a/src/addrdb.cpp b/src/addrdb.cpp
index 31f8eadf9..cd124d157 100644
--- a/src/addrdb.cpp
+++ b/src/addrdb.cpp
@@ -92,7 +92,7 @@ void DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
     unsigned char pchMsgTmp[4];
     verifier >> pchMsgTmp;
     // ... verify the network matches ours
-    if (memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp))) {
+    if (std::memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp))) {
         throw std::runtime_error{"Invalid network magic number"};
     }
 
diff --git a/src/base58.cpp b/src/base58.cpp
index 11c1ce739..de9a82d46 100644
--- a/src/base58.cpp
+++ b/src/base58.cpp
@@ -150,7 +150,7 @@ std::string EncodeBase58Check(Span<const unsigned char> input)
     }
     // re-calculate the checksum, ensure it matches the included 4-byte checksum
     uint256 hash = Hash(Span{vchRet}.first(vchRet.size() - 4));
-    if (memcmp(&hash, &vchRet[vchRet.size() - 4], 4) != 0) {
+    if (std::memcmp(&hash, &vchRet[vchRet.size() - 4], 4) != 0) {
         vchRet.clear();
         return false;
     }
diff --git a/src/net.cpp b/src/net.cpp
index 88f4cef5b..47ccede26 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -732,7 +732,7 @@ int V1TransportDeserializer::readHeader(Span<const uint8_t> msg_bytes)
     }
 
     // Check start string, network magic
-    if (memcmp(hdr.pchMessageStart, m_chain_params.MessageStart(), CMessageHeader::MESSAGE_START_SIZE) != 0) {
+    if (std::memcmp(hdr.pchMessageStart, m_chain_params.MessageStart(), CMessageHeader::MESSAGE_START_SIZE) != 0) {
         LogPrint(BCLog::NET, "Header error: Wrong MessageStart %s received, peer=%d\n", HexStr(hdr.pchMessageStart), m_node_id);
         return -1;
     }
@@ -793,7 +793,7 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds
     RandAddEvent(ReadLE32(hash.begin()));
 
     // Check checksum and header message type string
-    if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
+    if (std::memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
         LogPrint(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n",
                  SanitizeString(msg.m_type), msg.m_message_size,
                  HexStr(Span{hash}.first(CMessageHeader::CHECKSUM_SIZE)),
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index 17ab226a3..88a90e256 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -768,7 +768,7 @@ bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, c
 
         filein >> blk_start >> blk_size;
 
-        if (memcmp(blk_start, message_start, CMessageHeader::MESSAGE_START_SIZE)) {
+        if (std::memcmp(blk_start, message_start, CMessageHeader::MESSAGE_START_SIZE)) {
             return error("%s: Block magic mismatch for %s: %s versus expected %s", __func__, pos.ToString(),
                          HexStr(blk_start),
                          HexStr(message_start));
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index c4d13d728..1cabf65ac 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1877,7 +1877,7 @@ static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion,
             exec_script = CScript(script_bytes.begin(), script_bytes.end());
             uint256 hash_exec_script;
             CSHA256().Write(exec_script.data(), exec_script.size()).Finalize(hash_exec_script.begin());
-            if (memcmp(hash_exec_script.begin(), program.data(), 32)) {
+            if (std::memcmp(hash_exec_script.begin(), program.data(), 32)) {
                 return set_error(serror, SCRIPT_ERR_WITNESS_PROGRAM_MISMATCH);
             }
             return ExecuteWitnessScript(stack, exec_script, flags, SigVersion::WITNESS_V0, checker, execdata, serror);
diff --git a/src/validation.cpp b/src/validation.cpp
index fefbe033e..b3fcba2cd 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3548,7 +3548,7 @@ static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& stat
                 return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-nonce-size", strprintf("%s : invalid witness reserved value size", __func__));
             }
             CHash256().Write(hashWitness).Write(block.vtx[0]->vin[0].scriptWitness.stack[0]).Finalize(hashWitness);
-            if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) {
+            if (std::memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) {
                 return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-merkle-match", strprintf("%s : witness merkle commitment mismatch", __func__));
             }
             fHaveWitness = true;
@@ -4294,7 +4294,7 @@ void CChainState::LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp)
                 blkdat.FindByte(m_params.MessageStart()[0]);
                 nRewind = blkdat.GetPos() + 1;
                 blkdat >> buf;
-                if (memcmp(buf, m_params.MessageStart(), CMessageHeader::MESSAGE_START_SIZE)) {
+                if (std::memcmp(buf, m_params.MessageStart(), CMessageHeader::MESSAGE_START_SIZE)) {
                     continue;
                 }
                 // read size

fanquake · 2022-05-23T06:59:44Z

I believe some were missed:

Thanks. Have addressed more cases.

dunxen

ACK 7749a28

laanwj · 2022-05-26T13:23:11Z

No problem with this, I agree it's good to be consistent here, though not generally a fan of 'all-over-the-place' PRs.

I do think we need a way to enforce this to prevent reintroducing unqualified std functions, for example adapt one of the linters.

Empact · 2022-06-18T16:35:41Z

In the spirit of "Include what you use", we should also ensure we are including the appropriate headers for these functions, <cstring> for std::mem* and <cstdlib> for std::getenv.

w0xlt

Strong ACK 7749a28

-BEGIN VERIFY SCRIPT- sed -i -e "s/ memset(/ std::memset(/" $(git grep -l "memset(" -- ":(exclude)src/bench/nanobench.h" ":(exclude)src/secp256k1" ":(exclude)src/crc32c" ":(exclude)src/leveldb" ":(exclude)test/functional") sed -i -e "s/string.h/cstring/" src/crypto/aes.cpp src/crypto/chacha20.cpp src/crypto/chacha_poly_aead.cpp src/crypto/hmac_sha256.cpp src/crypto/hmac_sha512.cpp -END VERIFY SCRIPT-

-BEGIN VERIFY SCRIPT- sed -i -e "s/ memcmp(/ std::memcmp(/" $(git grep -l "memcmp(" -- ":(exclude)src/secp256k1" ":(exclude)src/leveldb" src) sed -i -e "s/(memcmp(/(std::memcmp(/" $(git grep -l "memcmp(" -- ":(exclude)src/secp256k1" ":(exclude)src/leveldb" ":(exclude)src/crypto/ctaes" src) -END VERIFY SCRIPT-

-BEGIN VERIFY SCRIPT- sed -i -e "s/ memcpy(/ std::memcpy(/" $(git grep -l "memcpy(" -- ":(exclude)src/secp256k1" ":(exclude)src/leveldb" src) sed -i -e "s/string.h/cstring/" src/crypto/common.h -END VERIFY SCRIPT-

hebasto · 2022-07-20T16:46:02Z

Concept ACK.

maflcko · 2022-07-20T16:54:49Z

Was #25172 (comment) addressed?

fanquake · 2022-07-22T16:39:01Z

Was #25172 (comment) addressed?

Not yet, but I am going to address this via clang-tidy plugin, rather than a python linter. Although there a few steps before that will be available.

fanquake · 2022-07-27T13:05:25Z

Looks like Clang is moving in the direction of adding warnings for unqualified std function usage, beginning with std::move and std::forward. Starting with LLVM 15 there will be an -Wunqualified-std-cast-call warning. See https://reviews.llvm.org/D119670.

hebasto

ACK 8dd3c4a, I have reviewed the code and it looks OK, I agree it can be merged.

maflcko · 2022-08-03T08:56:40Z

glozow removed the Waiting for author label 21 hours ago

Well, if we want the changes, I'd still prefer to enforce them. If we don't, then we could close this pull.

DrahtBot · 2022-10-19T09:31:37Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

fanquake added the Refactoring label May 19, 2022

fanquake mentioned this pull request May 19, 2022

prevector: enforce is_trivially_copyable_v #24962

Merged

kristapsk reviewed May 20, 2022

View reviewed changes

src/dbwrapper.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request May 20, 2022

Alter the ChaCha20Poly1305@Bitcoin AEAD to the new specification #20962

Closed

fanquake changed the title ~~refactor: use std:: for mem*( funcs~~ refactor: use std:: prefix for std lib funcs May 23, 2022

fanquake force-pushed the use_std_variants branch from 1b7b070 to 1d2d121 Compare May 23, 2022 06:58

fanquake force-pushed the use_std_variants branch 2 times, most recently from 14d5f20 to 7749a28 Compare May 23, 2022 09:09

dunxen approved these changes May 24, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 2, 2022

Severity-based logging -- parent PR #25203

Closed

6 tasks

This was referenced Jun 15, 2022

[crypto] Reduce wasted pseudorandom bytes in ChaCha20 #25354

Closed

refactor: Split util/system into exception, shell, and fs-specific files #25152

Closed

w0xlt approved these changes Jun 18, 2022

View reviewed changes

This was referenced Jun 25, 2022

BIP324: Cipher suite #25361

Closed

BIP324: Add encrypted p2p transport {de}serializer #23233

Closed

This was referenced Jul 16, 2022

[kernel 3e/n] Decouple CDBWrapper and its kernel users from ArgsManager #25623

Closed

BIP324: Handshake prerequisites #23561

Closed

refactor: Remove unused includes from dbwrapper.h #25645

Merged

DrahtBot added the Needs rebase label Jul 19, 2022

fanquake added 2 commits July 20, 2022 17:35

refactor: use std::memmove() over memmove()

e3f36cd

refactor: use std::vsnprintf over vsnprintf

2ce64a9

fanquake added 4 commits July 20, 2022 17:35

scripted-diff: use std::memcpy() over memcpy()

5a40961

-BEGIN VERIFY SCRIPT- sed -i -e "s/ memcpy(/ std::memcpy(/" $(git grep -l "memcpy(" -- ":(exclude)src/secp256k1" ":(exclude)src/leveldb" src) sed -i -e "s/string.h/cstring/" src/crypto/common.h -END VERIFY SCRIPT-

refactor: use std::getenv() over getenv()

8dd3c4a

fanquake force-pushed the use_std_variants branch from 7749a28 to 8dd3c4a Compare July 20, 2022 16:37

DrahtBot removed the Needs rebase label Jul 20, 2022

maflcko added the Waiting for author label Jul 23, 2022

This was referenced Jul 25, 2022

crypto: avoid potential buffer overread in ChaCha20::SetKey #25698

Closed

crypto: drop 16 byte key support for ChaCha20 #25712

Closed

hebasto approved these changes Jul 27, 2022

View reviewed changes

glozow removed the Waiting for author label Aug 2, 2022

This was referenced Aug 5, 2022

tidy: Enable more clang-tidy bugprone checks #25780

Closed

Improve runtime performance of --reindex #16981

Merged

DrahtBot mentioned this pull request Aug 13, 2022

refactor: Replace m_params with chainman.GetParams() #25830

Merged

DrahtBot mentioned this pull request Aug 23, 2022

Use static member functions from class instead of instances #25903

Closed

DrahtBot mentioned this pull request Sep 13, 2022

BIP324: Enable v2 P2P encrypted transport #24545

Closed

This was referenced Sep 22, 2022

Reduce wasted pseudorandom bytes in ChaCha20 + various improvements #26153

Merged

refactor / kernel: Move non-gArgs chainparams functionality to kernel #26177

Merged

DrahtBot mentioned this pull request Oct 10, 2022

test: Remove unused txmempool include from tests #26286

Merged

DrahtBot added the Needs rebase label Oct 19, 2022

fanquake closed this Dec 5, 2022

fanquake deleted the use_std_variants branch September 11, 2023 09:36

bitcoin locked and limited conversation to collaborators Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: use std:: prefix for std lib funcs #25172

refactor: use std:: prefix for std lib funcs #25172

Uh oh!

fanquake commented May 19, 2022 •

edited

Loading

Uh oh!

DrahtBot commented May 20, 2022 •

edited

Loading

Uh oh!

Uh oh!

aureleoules commented May 20, 2022

Uh oh!

fanquake commented May 23, 2022

Uh oh!

dunxen left a comment

Uh oh!

laanwj commented May 26, 2022

Uh oh!

Empact commented Jun 18, 2022 •

edited

Loading

Uh oh!

w0xlt left a comment

Uh oh!

hebasto commented Jul 20, 2022

Uh oh!

maflcko commented Jul 20, 2022

Uh oh!

fanquake commented Jul 22, 2022

Uh oh!

fanquake commented Jul 27, 2022

Uh oh!

hebasto left a comment

Uh oh!

maflcko commented Aug 3, 2022

Uh oh!

DrahtBot commented Oct 19, 2022

Uh oh!

Uh oh!

refactor: use std:: prefix for std lib funcs #25172

refactor: use std:: prefix for std lib funcs #25172

Uh oh!

Conversation

fanquake commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

aureleoules commented May 20, 2022

Uh oh!

fanquake commented May 23, 2022

Uh oh!

dunxen left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented May 26, 2022

Uh oh!

Empact commented Jun 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 20, 2022

Uh oh!

maflcko commented Jul 20, 2022

Uh oh!

fanquake commented Jul 22, 2022

Uh oh!

fanquake commented Jul 27, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 3, 2022

Uh oh!

DrahtBot commented Oct 19, 2022

Uh oh!

Uh oh!

fanquake commented May 19, 2022 •

edited

Loading

DrahtBot commented May 20, 2022 •

edited

Loading

Empact commented Jun 18, 2022 •

edited

Loading