From 2ed9aed2ea9e302b3b36b37806ec03f2cccaaed1 Mon Sep 17 00:00:00 2001 From: Bitflash Date: Thu, 30 Jul 2026 10:38:12 -0300 Subject: build: check format strings, which nothing ever did (#44) The segfault in issue #5 was a format string: `%I64d` is MSVC-only, glibc parses it as the GNU `I` flag with a width of 64 and an `int` conversion, the vararg run desynchronises, and the following `%s` dereferences an integer. It killed the node mid-sync at the first block carrying a real transaction, deterministically. GCC can catch this. It never got the chance, for two reasons that had to be fixed together: - `-w` sat in both makefiles and suppressed every diagnostic. It cannot be narrowed; it wins over any -W or -Werror= that follows, so it had to go. The four -Wno- entries replacing it are the categories this 2009-era tree actually produces (measured: 248 deprecated-declarations, 157 write-strings, 18 narrowing, 1 unused-result), silenced by name instead of wholesale. - Even with -Wformat on, nothing was checked. GCC only inspects format strings for functions it knows are printf-like, and every format string here goes through strprintf, error, my_snprintf, or OutputDebugStringF -- `printf` itself is #defined to the last of those. None carried a format attribute, so the entire tree was invisible to the check. They are annotated now. Annotating surfaced 77 warnings across 17 distinct sites, all fixed here. Most were size_t or int64 passed to %d, harmless on x86-64 but wrong. Two were not cosmetic: - CBlockIndex::ToString printed pprev/pnext with %08x, truncating both pointers to 32 bits, so the two fields that line exists to show were unreliable on every 64-bit build. - CInv::GetCommand threw "type=% unknown type", where the % ate the space as a flag and the u as its conversion. The message read "type= 3nknown type". PRId64 is now one spelling for every target. Its MSVCRT branch was what produced "I64d" in the first place, it is pointless against UCRT which takes the C99 forms, the rest of the tree already prints int64 with %lld on Windows, and its only user (i64tostr) is dead code. Collapsing it lets both builds share the gnu_printf archetype -- ms_printf would reject %zu, which this tree uses and UCRT supports, while waving through the %I64d that crashes. Verified on both platforms: clean build, and reintroducing the issue #5 specifier now fails compilation rather than shipping. main.h:410:50: error: unknown conversion type character 'I' in format main.h:410:70: error: format '%s' expects argument of type 'char*', but argument 3 has type 'long long int' That second line is the crash itself, caught at build time. Runtime checked on both: node syncs past the block from issue #5, and the repaired log lines read correctly -- before: CTxOut(nValue= 50.%08I64d, scriptPubKey=(null)) after: CTxOut(nValue=50.00000000, scriptPubKey=0xE1C7EF3E706002) Four '_WIN32_WINNT redefined' warnings remain on Windows. Silencing them means letting a system header pick the Windows API level instead of this tree, which is a separate decision from format checking. --- src/Makefile | 21 ++++++++++++++++++++- src/btfaddr.cpp | 3 +++ src/main.cpp | 16 ++++++++-------- src/main.h | 14 ++++++++------ src/makefile.mingw | 9 ++++++++- src/net.cpp | 2 +- src/net.h | 4 +++- src/nostr.cpp | 6 +++++- src/util.cpp | 4 ++-- src/util.h | 44 ++++++++++++++++++++++++++++++++++---------- 10 files changed, 92 insertions(+), 31 deletions(-) diff --git a/src/Makefile b/src/Makefile index 9846d8e..f0c6706 100644 --- a/src/Makefile +++ b/src/Makefile @@ -9,7 +9,26 @@ RANDOMX_DIR ?= $(HOME)/RandomX IMGUI_DIR = imgui -CXXFLAGS = -std=gnu++14 -O2 -w -fpermissive -pthread \ +# -w used to sit here and suppressed every diagnostic in the build, including +# the one that mattered: `%I64d` is MSVC-only, glibc mis-parses it as the GNU +# `I` flag with a width of 64 and an `int` conversion, and the resulting vararg +# desynchronisation segfaulted the node while logging a received block (issue +# #5, deterministic at height 1062 -- the first block carrying a real +# transaction). GCC had been reporting it all along: +# +# warning: format '%d' expects argument of type 'int', +# but argument 4 has type 'int64_t' +# +# -w cannot be narrowed: it wins over any -W or -Werror= that follows it, so it +# has to go. The four -Wno- entries below are the categories this 2009-era tree +# genuinely produces (measured: 248 deprecated-declarations, 157 write-strings, +# 18 narrowing, 1 unused-result); silencing them by name instead of silencing +# everything keeps the build quiet while leaving format errors fatal. +WARNFLAGS = -Wformat -Werror=format \ + -Wno-deprecated-declarations -Wno-write-strings \ + -Wno-narrowing -Wno-unused-result + +CXXFLAGS = -std=gnu++14 -O2 $(WARNFLAGS) -fpermissive -pthread \ -DSECP256K1_STATIC \ -I$(SECP_INC) -I$(RANDOMX_DIR)/src \ -I/usr/include/nlohmann \ diff --git a/src/btfaddr.cpp b/src/btfaddr.cpp index 55bb6c5..6edcc91 100644 --- a/src/btfaddr.cpp +++ b/src/btfaddr.cpp @@ -7,7 +7,10 @@ #include "btfaddr.h" #include +// See the note in nostr.cpp: the makefiles pass -DSECP256K1_STATIC too. +#ifndef SECP256K1_STATIC #define SECP256K1_STATIC +#endif #include #include #include diff --git a/src/main.cpp b/src/main.cpp index bb317d2..c882b0b 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -898,7 +898,7 @@ bool CTransaction::ConnectInputs(CTxDB& txdb, map& mapTestPoo } if (prevout.n >= txPrev.vout.size() || prevout.n >= txindex.vSpent.size()) - return error("ConnectInputs() : %s prevout.n out of range %d %d %d", GetHash().ToString().substr(0,6).c_str(), prevout.n, txPrev.vout.size(), txindex.vSpent.size()); + return error("ConnectInputs() : %s prevout.n out of range %d %d %d", GetHash().ToString().substr(0,6).c_str(), prevout.n, (int)txPrev.vout.size(), (int)txindex.vSpent.size()); // If prev is coinbase, check that it's matured if (txPrev.IsCoinBase()) @@ -1690,14 +1690,14 @@ void PrintBlockTree() pindex->nBlockPos, block.GetHash().ToString().substr(0,14).c_str(), DateTimeStr(block.nTime).c_str(), - block.vtx.size()); + (int)block.vtx.size()); CRITICAL_BLOCK(cs_mapWallet) { if (mapWallet.count(block.vtx[0].GetHash())) { CWalletTx& wtx = mapWallet[block.vtx[0].GetHash()]; - printf(" mine: %d %d %d", wtx.GetDepthInMainChain(), wtx.GetBlocksToMaturity(), wtx.GetCredit()); + printf(" mine: %d %d %lld", wtx.GetDepthInMainChain(), wtx.GetBlocksToMaturity(), wtx.GetCredit()); } } printf("\n"); @@ -1759,7 +1759,7 @@ bool ProcessMessages(CNode* pfrom) CDataStream& vRecv = pfrom->vRecv; if (vRecv.empty()) return true; - LogPrint("net", "ProcessMessages(%d bytes)\n", vRecv.size()); + LogPrint("net", "ProcessMessages(%d bytes)\n", (int)vRecv.size()); // // Message format @@ -1783,7 +1783,7 @@ bool ProcessMessages(CNode* pfrom) break; } if (pstart - vRecv.begin() > 0) - if (LogAcceptsCategory("net")) printf("\n\nPROCESSMESSAGE SKIPPED %d BYTES\n\n", pstart - vRecv.begin()); + if (LogAcceptsCategory("net")) printf("\n\nPROCESSMESSAGE SKIPPED %d BYTES\n\n", (int)(pstart - vRecv.begin())); vRecv.erase(vRecv.begin(), pstart); // Read header @@ -1836,7 +1836,7 @@ bool ProcessMessages(CNode* pfrom) bool ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) { static map > mapReuseKey; - if (LogAcceptsCategory("net")) { printf("received: %-12s (%d bytes) ", strCommand.c_str(), vRecv.size()); for (int i = 0; i < min(vRecv.size(), (unsigned int)25); i++) printf("%02x ", vRecv[i] & 0xff); printf("\n"); } + if (LogAcceptsCategory("net")) { printf("received: %-12s (%d bytes) ", strCommand.c_str(), (int)vRecv.size()); for (int i = 0; i < min(vRecv.size(), (unsigned int)25); i++) printf("%02x ", vRecv[i] & 0xff); printf("\n"); } if (nDropMessagesTest > 0 && GetRand(nDropMessagesTest) == 0) { if (LogAcceptsCategory("net")) printf("dropmessages DROPPING RECV MESSAGE\n"); @@ -2171,7 +2171,7 @@ bool ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) if (!vRecv.empty()) - if (LogAcceptsCategory("net")) printf("ProcessMessage(%s) : %d extra bytes\n", strCommand.c_str(), vRecv.size()); + if (LogAcceptsCategory("net")) printf("ProcessMessage(%s) : %d extra bytes\n", strCommand.c_str(), (int)vRecv.size()); return true; } @@ -2885,7 +2885,7 @@ bool BitcoinMiner() } pblock->nBits = nBits; pblock->vtx[0].vout[0].nValue = pblock->GetBlockValue(nBestHeight + 1, nFees); - if (LogAcceptsCategory("net")) printf("\n\nRunning BitcoinMiner with %d transactions in block\n", pblock->vtx.size()); + if (LogAcceptsCategory("net")) printf("\n\nRunning BitcoinMiner with %d transactions in block\n", (int)pblock->vtx.size()); // diff --git a/src/main.h b/src/main.h index fde3a4e..7e5bdf2 100644 --- a/src/main.h +++ b/src/main.h @@ -650,11 +650,11 @@ public: string ToString() const { string str; - str += strprintf("CTransaction(hash=%s, ver=%d, vin.size=%d, vout.size=%d, nLockTime=%d)\n", + str += strprintf("CTransaction(hash=%s, ver=%d, vin.size=%d, vout.size=%d, nLockTime=%u)\n", GetHash().ToString().substr(0,6).c_str(), nVersion, - vin.size(), - vout.size(), + (int)vin.size(), + (int)vout.size(), nLockTime); for (int i = 0; i < vin.size(); i++) str += " " + vin[i].ToString() + "\n"; @@ -1081,7 +1081,7 @@ public: hashPrevBlock.ToString().substr(0,14).c_str(), hashMerkleRoot.ToString().substr(0,6).c_str(), nTime, nBits, nNonce, - vtx.size()); + (int)vtx.size()); for (int i = 0; i < vtx.size(); i++) { printf(" "); @@ -1223,8 +1223,10 @@ public: string ToString() const { - return strprintf("CBlockIndex(nprev=%08x, pnext=%08x, nFile=%d, nBlockPos=%-6d nHeight=%d, merkle=%s, hashBlock=%s)", - pprev, pnext, nFile, nBlockPos, nHeight, + // pprev/pnext are pointers: %08x truncated them to 32 bits on a 64-bit + // build, so the two fields this line exists to show were unreliable. + return strprintf("CBlockIndex(nprev=%p, pnext=%p, nFile=%d, nBlockPos=%-6d nHeight=%d, merkle=%s, hashBlock=%s)", + (void*)pprev, (void*)pnext, nFile, nBlockPos, nHeight, hashMerkleRoot.ToString().substr(0,6).c_str(), GetBlockHash().ToString().substr(0,14).c_str()); } diff --git a/src/makefile.mingw b/src/makefile.mingw index 7d35d92..01f7702 100644 --- a/src/makefile.mingw +++ b/src/makefile.mingw @@ -7,7 +7,14 @@ DEPS = ../deps RANDOMX = ../RandomX IMGUI_DIR = imgui -CXXFLAGS = -std=gnu++14 -O2 -w -fpermissive -Wno-deprecated-declarations -pthread \ +# See the long note in Makefile: -w hid the format bug that segfaulted the node +# on glibc (issue #5), and -w cannot be narrowed, so format diagnostics are now +# fatal and only the categories this tree actually produces are silenced. +WARNFLAGS = -Wformat -Werror=format \ + -Wno-deprecated-declarations -Wno-write-strings \ + -Wno-narrowing -Wno-unused-result + +CXXFLAGS = -std=gnu++14 -O2 $(WARNFLAGS) -fpermissive -pthread \ -DWIN32 -DNOPCH \ -DSECP256K1_STATIC \ -I$(DEPS)/include -I$(RANDOMX)/src \ diff --git a/src/net.cpp b/src/net.cpp index 08a3348..395092d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -920,7 +920,7 @@ void ThreadSocketHandler2(void* parg) if (pnodeExtra->GetRefCount() <= (pnodeExtra->fNetworkNode ? 1 : 0)) { - LogPrint("net", "(%d nodes) disconnecting duplicate: %s\n", vNodes.size(), pnodeExtra->addr.ToString().c_str()); + LogPrint("net", "(%d nodes) disconnecting duplicate: %s\n", (int)vNodes.size(), pnodeExtra->addr.ToString().c_str()); if (pnodeExtra->fNetworkNode && !pnode->fNetworkNode) { pnode->AddRef(); diff --git a/src/net.h b/src/net.h index d13de61..364fbfb 100644 --- a/src/net.h +++ b/src/net.h @@ -392,7 +392,9 @@ public: const char* GetCommand() const { if (!IsKnownType()) - throw std::out_of_range(strprintf("CInv::GetCommand() : type=% unknown type", type)); + // Was "type=% unknown type": the % swallowed the space as a flag and + // the u as the conversion, so this read "type= 3nknown type". + throw std::out_of_range(strprintf("CInv::GetCommand() : type=%d unknown type", type)); return ppszTypeName[type]; } diff --git a/src/nostr.cpp b/src/nostr.cpp index 482a835..088174a 100644 --- a/src/nostr.cpp +++ b/src/nostr.cpp @@ -6,8 +6,12 @@ #include "headers.h" #include #include -// We link libsecp256k1 statically (without this the header assumes DLL import) +// We link libsecp256k1 statically (without this the header assumes DLL import). +// Both makefiles also pass -DSECP256K1_STATIC; the guard keeps that from +// warning while leaving this file correct if built without the flag. +#ifndef SECP256K1_STATIC #define SECP256K1_STATIC +#endif #include #include #include diff --git a/src/util.cpp b/src/util.cpp index e4e314f..d52ca88 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -180,7 +180,7 @@ void RandAddSeed(bool fPerfmon) RAND_add(&hash, sizeof(hash), min(nSize/500.0, (double)sizeof(hash))); hash = 0; memset(pdata, 0, nSize); - printf("RandAddSeed() got %d bytes of performance data\n", nSize); + printf("RandAddSeed() got %lu bytes of performance data\n", nSize); } } #else @@ -472,7 +472,7 @@ void AddTimeData(unsigned int ip, int64 nTime) if (vTimeOffsets.empty()) vTimeOffsets.push_back(0); vTimeOffsets.push_back(nOffsetSample); - printf("Added time data, samples %d, ip %08x, offset %+lld (%+lld minutes)\n", vTimeOffsets.size(), ip, vTimeOffsets.back(), vTimeOffsets.back()/60); + printf("Added time data, samples %d, ip %08x, offset %+lld (%+lld minutes)\n", (int)vTimeOffsets.size(), ip, vTimeOffsets.back(), vTimeOffsets.back()/60); if (vTimeOffsets.size() >= 5 && vTimeOffsets.size() % 2 == 1) { sort(vTimeOffsets.begin(), vTimeOffsets.end()); diff --git a/src/util.h b/src/util.h index b583248..797ed27 100644 --- a/src/util.h +++ b/src/util.h @@ -14,7 +14,9 @@ typedef unsigned long long uint64; #define for if (false) ; else for #endif -#ifndef _MSC_VER +// MinGW already provides __forceinline for MSVC compatibility, so defining it +// unconditionally warned ten times per build once -w was removed. +#if !defined(_MSC_VER) && !defined(__forceinline) #define __forceinline inline #endif @@ -40,17 +42,17 @@ typedef unsigned long long uint64; #endif #define snprintf my_snprintf +// One definition for every target. The MSVCRT branch that used to sit here +// produced "I64d", which is the specifier that segfaults on glibc (issue #5) -- +// and it was pointless besides: this build targets UCRT, which accepts the C99 +// forms, and the rest of the tree already prints int64 with %lld on Windows. +// Keeping one spelling everywhere is also what lets the format attributes below +// use a single archetype, so a bad specifier fails the build on both platforms. #ifndef PRId64 -#if defined(_MSC_VER) || defined(__BORLANDC__) || defined(__MSVCRT__) -#define PRId64 "I64d" -#define PRIu64 "I64u" -#define PRIx64 "I64x" -#else #define PRId64 "lld" #define PRIu64 "llu" #define PRIx64 "llx" #endif -#endif // This is needed because the foreach macro can't get over the comma in pair #define PAIRTYPE(t1, t2) pair @@ -102,10 +104,30 @@ void LogPrintDedup(const char* category, const char* file, int line, const std:: #define LogPrint(category, ...) \ do { if (LogAcceptsCategory(category)) LogPrintDedup((category), __FILE__, __LINE__, strprintf(__VA_ARGS__)); } while (0) +// Every format string in this project goes through one of the four functions +// below -- `printf` itself is remapped to OutputDebugStringF just above, and +// snprintf to my_snprintf. None of them carried a format attribute, and GCC +// only checks format strings for functions it knows are printf-like, so +// -Wformat had nothing to inspect and the whole tree was unchecked. That is +// how `%I64d` (MSVC-only, and a segfault on glibc) survived in +// CTxOut::ToString until it crashed a node mid-sync -- see issue #5. +// +// gnu_printf on both platforms, deliberately. MinGW's other option, ms_printf, +// models the legacy msvcrt: it rejects %zu, which this tree uses and UCRT +// supports, and it accepts %I64d, which is the specifier that crashes on glibc. +// It would give false positives where the code is right and stay silent where it +// is dangerous. gnu_printf describes UCRT's C99 behaviour accurately, so one +// archetype covers both builds and a bad specifier fails either one. +#if defined(__GNUC__) +#define BF_FORMAT(fmt, args) __attribute__((format(gnu_printf, fmt, args))) +#else +#define BF_FORMAT(fmt, args) +#endif + void RandAddSeed(bool fPerfmon=false); -int my_snprintf(char* buffer, size_t limit, const char* format, ...); -string strprintf(const char* format, ...); -bool error(const char* format, ...); +int my_snprintf(char* buffer, size_t limit, const char* format, ...) BF_FORMAT(3, 4); +string strprintf(const char* format, ...) BF_FORMAT(1, 2); +bool error(const char* format, ...) BF_FORMAT(1, 2); void PrintException(std::exception* pex, const char* pszThread); void ParseString(const string& str, char c, vector& v); string FormatMoney(int64 n, bool fPlus=false); @@ -266,6 +288,8 @@ void PrintHex(const T pbegin, const T pend, const char* pszFormat="%s", bool fSp // Forward declaration -- defined in main.cpp string GetAppDir(); +inline int OutputDebugStringF(const char* pszFormat, ...) BF_FORMAT(1, 2); + inline int OutputDebugStringF(const char* pszFormat, ...) { #if 1 // debug.log always enabled -- cgit v1.2.3