Skip to content

build: check format strings, which nothing ever did - #44

Merged
Bitflash-sh merged 1 commit into
mainfrom
harden/format-warnings
Jul 30, 2026
Merged

build: check format strings, which nothing ever did#44
Bitflash-sh merged 1 commit into
mainfrom
harden/format-warnings

Conversation

@Bitflash-sh

Copy link
Copy Markdown
Owner

Follow-up to #5. That segfault was a format string, and the compiler could have caught it — this makes sure the next one is caught.

What happened

%I64d is MSVC-only. glibc reads it as the GNU I flag, a width of 64, and an int conversion; the vararg run desynchronises and the following %s dereferences an integer as a pointer. It killed the node mid-sync, deterministically, at the first block carrying a real transaction.

GCC diagnoses this. It never got the chance, for two reasons that had to be fixed together — fixing either alone does nothing:

  1. -w in both makefiles suppressed every diagnostic. It cannot be narrowed: it wins over any -W or -Werror= that follows, in any order. So it had to go. The four -Wno- entries replacing it are the categories this 2009-era tree actually produces — measured at 248 deprecated-declarations, 157 write-strings, 18 narrowing, 1 unused-result — silenced by name rather than wholesale.

  2. No format attributes. Even with -Wformat on, GCC only inspects format strings for functions it knows are printf-like. Every format string in this project goes through strprintf, error, my_snprintf or OutputDebugStringF — and printf itself is #defined to the last of those. None carried an attribute, so the whole tree was invisible to the check. That is the real reason %I64d survived, and it is why enabling warnings alone would have changed nothing.

What it found

77 warnings across 17 distinct sites (the same header lines multiplied across translation units), all fixed here. Most were size_t or int64 passed to %d — harmless on x86-64, still wrong. Two were not cosmetic:

  • CBlockIndex::ToString printed pprev/pnext with %08x, truncating both pointers to 32 bits. The two fields that line exists to show were unreliable on every 64-bit build.
  • CInv::GetCommand threw "type=% unknown type" — the % ate the space as a flag and the u as its conversion, so the message read type= 3nknown type.

PRId64

Now one spelling for every target. Its MSVCRT branch is 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 is what lets both builds share the gnu_printf archetype — ms_printf would reject %zu (used here, supported by UCRT) while waving through the %I64d that crashes.

Verification

Both platforms, clean build, then the specifier reintroduced on purpose:

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'

The second line is the crash itself, caught at build time instead of shipped.

Runtime on both platforms — node syncs past the issue #5 block, and the repaired log lines read correctly:

before:  CTxOut(nValue=                    50.%08I64d, scriptPubKey=(null))
after:   CTxOut(nValue=50.00000000, scriptPubKey=0xE1C7EF3E70600290DCEDB8)
  • Linux: 0 warnings, 0 errors, synced to 1388 past block 1062
  • Windows: 0 errors, binary runs and syncs
  • %I64d reintroduced fails the build on both
  • Log output verified correct on both

Known leftover

Four '_WIN32_WINNT' redefined warnings remain on Windows. Silencing them means letting a system header choose the Windows API level instead of this tree, which is a separate decision from format checking and does not belong in this PR.

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.
@Bitflash-sh
Bitflash-sh merged commit 8db7eee into main Jul 30, 2026
@Bitflash-sh Bitflash-sh mentioned this pull request Jul 30, 2026
Bitflash-sh added a commit that referenced this pull request Jul 30, 2026
Headless mining fix (#42), ARM64 build (#41), format-string
checking (#44), gitignore for key material (#43).

#42 is the reason this release exists: -gen has mined nothing on
headless nodes since v1.2.0.
Vic-Nas added a commit to Vic-Nas/bitflash-mod that referenced this pull request Jul 30, 2026
…r no-op fix)

My previous attempt (adding -Wformat -Werror=format after -w in
CXXFLAGS) did nothing. Verified by direct test: a genuine format bug
(%d given a long long) still compiles clean with -w present in the
command line, regardless of where -Wformat/-Werror=format are placed
relative to it. -w overrides them unconditionally.

Ported and adapted Bitflash-sh#44 properly instead, which diagnosed
this exact problem against the same root cause (the Bitflash-sh#5 segfault) and
fixed it correctly: -w removed entirely, replaced with -Wno-
deprecated-declarations/-Wno-write-strings/-Wno-narrowing/-Wno-
unused-result (the categories this tree's own code legitimately
triggers) plus -Wformat -Wformat-security -Werror=format. Also
required BF_FORMAT annotations on strprintf/my_snprintf/ErrorImpl/
OutputDebugStringF -- printf itself is #defined to the last of those,
and GCC only checks format strings for functions it knows are
printf-like, so without the attributes -Wformat had nothing to
inspect regardless of -w. gnu_printf archetype specifically: MinGW's
alternative, ms_printf, models legacy msvcrt and would reject %zu
(used here, supported by UCRT) while accepting %I64d (the specifier
that actually crashes on glibc) -- exactly backwards for what this
needs to catch. Collapsed PRId64/PRIu64/PRIx64 to one spelling
(dropping the MSVCRT branch that produced %I64d in the first place),
needed for both platforms to share one format archetype.

Adapted rather than copied where our fork has diverged: PrintBlockTree
doesn't exist here (removed earlier this session as dead code, so its
fixes in the upstream diff don't apply), and `error` here is a macro
over ErrorImpl (ours), not a plain function (theirs).

Removing -w surfaced real bugs beyond what the upstream PR's diff
covers, since this fork carries additional code: a SECP256K1_STATIC
macro-redefinition warning in nostr.cpp/btfaddr.cpp (guarded the same
way upstream did), and one more size_t-into-%d mismatch in util.cpp's
AddTimeData logging that wasn't in their tree.

Verified by actually compiling every source file in the tree with the
real flags (not just the ones this diff touches) -- main/net/gui/
main_gui/util/db/rpc/script/update/market/dashboard.cpp all clean.
nostr/btfaddr/compactblock/randomx_pow.cpp couldn't be verified this
same way here (their C dependencies -- secp256k1, libsodium, RandomX
-- aren't vendored in this sandbox, unrelated to this change), but
nostr.cpp and btfaddr.cpp got the one warning each that could be
checked without those headers (the macro redefinition, both fixed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant