Fix std::format_error crash from Windows-1252 bytes in format strings

[jyh9521]

Fixes #399

Four call sites embed raw Windows-1252 bytes (\x95, \xA6) in std::format
format strings, which MSVC's <format> validates as UTF-8. Moves the bytes
out of the format strings; rendered output is unchanged.

Details and the diagnosis are in #399.

[is-this-c]

Compiling with /execution-charset:windows-1252 may be better.

[Copilot]

Pull request overview

This PR fixes a multiplayer crash caused by MSVC <format> throwing std::format_error when std::format format strings contain raw Windows-1252 bytes (e.g., \x95, \xA6). It preserves the exact rendered output/byte sequences required by the game’s bitmap fonts and network-visible chat prefixes by moving those bytes out of the format strings and into arguments / concatenation.

Changes:

• Reworks scoreboard header formatting to pass the Windows-1252 bullet (\x95) as a separate argument instead of embedding it in the std::format format string.
• Replaces std::format usage for \xA6-prefixed automated chat messages with string concatenation to avoid UTF-8 validation of the format string.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
game_patch/multi/multi.cpp	Avoids std::format with \xA6 in the format string by switching to concatenation for “Shot canceled” messages.
game_patch/multi/alpine_packets.cpp	Avoids std::format with \xA6 in the format string when sending legacy chat lines.
game_patch/hud/multi_scoreboard.cpp	Introduces a SEPARATOR constant (\x95) and updates scoreboard std::format calls to keep the bullet out of the format string.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[is-this-c]

I cannot reproduce nor on godbolt.org.

[GooberRF]

@jyh9521 When you have a chance, could you speak to the points Copilot and @is-this-c raised above?

[jyh9521]

I cannot reproduce nor on godbolt.org.

@jyh9521 When you have a chance, could you speak to the points Copilot and @is-this-c raised above?

Reply 1 — main PR / issue comment

Thanks both. I ran a probe, and it turns out /execution-charset:windows-1252 isn't just "may
be better" — it's the actual fix. It also explains why neither of you can reproduce this. My
original diagnosis in #399 was wrong; corrected below.

The variable is the ordinary literal encoding, which MSVC defaults to the build machine's
system ANSI code page. Mine is 932 (Shift-JIS), because the system locale is Japanese.

#include <cstdio>
#include <format>
#include <stdexcept>
 
int main()
{
#ifdef _MSVC_EXECUTION_CHARACTER_SET
    std::printf("_MSVC_EXECUTION_CHARACTER_SET = %d\n", _MSVC_EXECUTION_CHARACTER_SET);
#else
    std::printf("_MSVC_EXECUTION_CHARACTER_SET = (not defined)\n");
#endif
    try {
        const std::string s = std::format("{} \x95 {}/{} PLAYING", "DEATHMATCH", 3, 8);
        std::printf("ok, %zu bytes:", s.size());
        for (unsigned char c : s) {
            std::printf(" %02X", c);
        }
        std::printf("\n");
    }
    catch (const std::format_error& e) {
        std::printf("std::format_error: %s\n", e.what());
        return 1;
    }
    return 0;
}

Same machine, _MSC_VER = 1944, _MSVC_STL_UPDATE = 202503:

flags	_MSVC_EXECUTION_CHARACTER_SET	result
(none)	932	std::format_error: Invalid encoded character in format string.
/execution-charset:windows-1252	1252	ok — 44 45 41 54 48 4D 41 54 43 48 20 95 20 33 2F 38 20 50 4C 41 59 49 4E 47
/utf-8	65001	ok — same bytes

UTF-8 is fine too, so "the format string has to be valid UTF-8" — which I wrote in #399, and
which the Copilot review repeats — is not the rule. The actual trigger is narrower:

932 is a DBCS, and 0x95 is a lead byte in Shift-JIS. "\x95 " is 0x95 0x20, and 0x20
is not a valid trail byte. When the ordinary literal encoding is a multi-byte charset the
implementation has to decode the format string instead of scanning it byte by byte — in a DBCS
a trail byte can be 0x7B, which would otherwise be mistaken for {. That decode rejects the
invalid sequence and throws. A single-byte encoding like 1252 needs no decoding, and the UTF-8
path evidently doesn't validate, so neither of you sees it — and neither does godbolt.

So the trigger is the builder's system locale, not the code. Anyone building from source on a
Japanese, Chinese or Korean Windows install hits it — which is disproportionately the people
likely to be working on a localization.

Given that, I agree /execution-charset:windows-1252 is the better fix:

• it addresses the cause rather than four symptoms
• it covers every literal in the tree that embeds a Windows-1252 byte, not just the four format
  strings — \xA6 and \xA8 appear in a number of chat literals too, and any future one is
  then safe by construction
• it states the intent explicitly: narrow literals here really are Windows-1252, because the
  .vf bitmap fonts are indexed by Windows-1252 byte value
• builds stop depending on the machine they were made on
  Pairing it with /source-charset:utf-8 would additionally silence the C4819 warnings that show
  up on non-UTF-8 locales (a few headers have · and × in comments).

I'm happy to replace this PR with just the compiler flag, or keep both if you'd rather have the
format strings hardened independently of build flags. Say which you prefer and I'll update it.

Reply 2 — inline, alpine_packets.cpp (Copilot: per-client allocation)

Good catch, and it stands regardless of which fix we land: the pre-1.2.0 branch is inside the
for (rf::Player& player : ...) loop, so the message is rebuilt for every legacy client. Note
the original std::format("\xA6 {}", msg) did the same thing, so the PR doesn't regress it —
but there's no reason to keep it. Building it once, lazily:

std::optional<std::string> legacy_msg;
for (rf::Player& player : SinglyLinkedList{rf::player_list}) {
    ...
    } else {
        if (!legacy_msg) {
            legacy_msg = std::string{"\xA6 "} + std::string{msg};
        }
        send_chat_line_packet(*legacy_msg, &player);
    }
}

If we go with /execution-charset:windows-1252 this file doesn't need to change at all, in
which case this is worth a separate PR rather than folding it in here.

Reply 3 — inline, multi_scoreboard.cpp (Copilot: comment wording)

The comment is wrong, but not in the way suggested — the replacement wording is wrong too.
Narrow format strings are not validated as UTF-8: with /utf-8 (_MSVC_EXECUTION_CHARACTER_SET == 65001) this exact literal compiles and runs fine, 0x95 passes through untouched. See the
measurements in the main thread.

What actually breaks is a double-byte ordinary literal encoding, where 0x95 is a lead byte
with no valid trail byte after it. Accurate wording would be:

// Windows-1252 bullet. Must stay a raw byte for the .vf bitmap fonts, and must stay out of
// any std::format format string: when the ordinary literal encoding is a double-byte charset
// (932 is the default on a Japanese Windows), MSVC decodes the format string and rejects
// 0x95 as a stray lead byte.

That said, if we take /execution-charset:windows-1252 instead, this constant and its comment
go away entirely.

[is-this-c]

/source-charset:utf-8 /execution-charset:windows-1252 should be added to the root CMakeLists.txt along with equivalent arguments for GCC.

[jyh9521]

/source-charset:utf-8 /execution-charset:windows-1252 should be added to the root CMakeLists.txt along with equivalent arguments for GCC.

Agreed. I've replaced the four code changes with the compiler flags — the PR is now a single
hunk in the root CMakeLists.txt:

if(MSVC)
    add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
    add_compile_options(/arch:SSE2)
    # Pin both charsets. Sources are UTF-8; narrow string literals are Windows-1252,
    # because the .vf bitmap fonts are indexed by Windows-1252 byte value and several
    # literals embed those bytes directly (\x95 bullet, \xA6 chat prefix, ...).
    # Without this the ordinary literal encoding follows the build machine's system
    # code page: on a DBCS locale such as 932 those bytes become invalid multi-byte
    # sequences and std::format throws std::format_error at runtime.
    add_compile_options(/source-charset:utf-8 /execution-charset:windows-1252)
    # Statically link Microsoft's CRT.
    set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
else()
    add_compile_options(-msse2)
    add_compile_options(-finput-charset=UTF-8 -fexec-charset=WINDOWS-1252)
endif()

Two things I checked before proposing /source-charset:utf-8, since it changes how every
source file is read:

• All 486 non-vendor .cpp/.h files are valid UTF-8 today (86 of them contain non-ASCII
  bytes — mostly · and × in comments). So nothing gets misdecoded by pinning the source
  charset, and it also silences the C4819 warnings that currently appear on non-UTF-8 locales.
• WINDOWS-1252 is a valid iconv name for GCC's -fexec-charset; CP1252 works too if you
  prefer that spelling.
  Worth noting the flags fix strictly more than the code changes did: \xA6 and \xA8 appear in
  a number of other chat literals, and any future one is now safe by construction rather than
  depending on someone remembering the rule.

Copilot's two comments are moot with this approach, since none of the three source files change
anymore. For the record, though:

• The per-legacy-client allocation it flagged in af_broadcast_automated_chat_msg is a real
  (small) inefficiency that predates this PR — the original std::format("\xA6 {}", msg) is
  inside the same player loop. Happy to open a separate PR hoisting it out if that's wanted.
• Its suggested comment wording ("MSVC validates narrow format strings as UTF-8") is not
  accurate: with /utf-8 this exact literal compiles and runs fine and 0x95 passes through
  untouched. The trigger is specifically a double-byte ordinary literal encoding, where 0x95
  is a lead byte with no valid trail byte after it. Measurements are in the main thread above.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

CMakeLists.txt:50

• -finput-charset/-fexec-charset are being applied to all non-MSVC builds (including Linux/macOS toolchains). That can unintentionally transcode any non-ASCII UTF-8 literals into Windows-1252 bytes, changing runtime strings and potentially breaking builds on compilers that don’t accept these flags. If this is meant to address Windows/MinGW-only behavior, scope it (e.g. if(MINGW) / Windows) rather than globally for every non-MSVC target.

else()
    add_compile_options(-msse2)
    add_compile_options(-finput-charset=UTF-8 -fexec-charset=WINDOWS-1252)
endif()

docs/CHANGELOG.md:110

• This changelog entry describes the crash as limited to builds compiled on Windows with a Japanese/Chinese/Korean locale, but #399’s report reproduces on an English Windows 11 machine and attributes it to invalid Windows-1252 bytes inside std::format format strings (toolset-dependent). Consider rewording to reflect the actual scope/cause so users don’t mistakenly assume they’re unaffected.

[@jyh9521](https://github.com/jyh9521)
- Fix crash on join for builds compiled on Windows systems using a Japanese, Chinese, or Korean locale

[GooberRF]

@jyh9521 Merged. Thank you for the contribution!