Fix crash: replace malformed UTF-8 instead of throwing in PayloadDecoder

[lkarra2]

Summary

PayloadDecoder::DecodeRequest and DecodeRecord serialize decoded telemetry to a human-readable string with nlohmann::json::dump() using the default error_handler_t::strict. When a decoded event string contains bytes that are not valid UTF-8, dump() throws nlohmann::json::type_error (id 316, "invalid UTF-8 byte") from serializer::dump_escaped.

Telemetry event properties can legitimately contain arbitrary / binary bytes, so this throw can fire in the field. Because the decode path can be reached from a host application's debug-event listener with no surrounding try/catch, the unhandled C++ exception propagates out of the SDK and terminates the hosting process.

Representative stack:

...::serializer::dump_escaped        json.hpp   (throws type_error.316)
...::serializer::dump                 json.hpp
...::basic_json::dump                 json.hpp
Microsoft::Applications::Events::exporters::DecodeRequest   PayloadDecoder.cpp
... (host debug-event listener) ...
-> std::terminate

Fix

Use error_handler_t::replace so malformed UTF-8 is replaced with U+FFFD instead of throwing. Applied to both decoder entry points:

• DecodeRequest: j.dump(2) -> j.dump(2, ' ', false, json::error_handler_t::replace)
• DecodeRecord: j.dump(4) -> j.dump(4, ' ', false, json::error_handler_t::replace)

The decoder still returns the (sanitized) JSON; it can no longer throw or crash the host on non-UTF-8 telemetry.

Tests

Adds tests/unittests/PayloadDecoderTests.cpp (registered in tests/unittests/CMakeLists.txt):

• DecodeRecord_InvalidUtf8_DoesNotThrow: builds a minimal CsProtocol::Record, injects an invalid UTF-8 byte (0xFF) into a string field, and asserts DecodeRecord does not throw and emits U+FFFD (EF BF BD) rather than the raw byte.
• DecodeRecord_ValidUtf8_IsPreserved: verifies well-formed UTF-8 is left untouched.

Risk / compatibility

• Signature-compatible overload of dump(); no public API change.
• Output is unchanged for well-formed UTF-8 payloads; only malformed byte sequences are replaced with U+FFFD instead of throwing.

[Copilot]

Pull request overview

This PR prevents PayloadDecoder’s JSON serialization from terminating host processes when decoded telemetry contains malformed UTF-8, by switching nlohmann::json::dump() to use error_handler_t::replace.

Changes:

• Update DecodeRequest to call dump(..., json::error_handler_t::replace) and add rationale comments.
• Update DecodeRecord similarly to replace malformed UTF-8 instead of throwing.

---

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

tests/unittests/PayloadDecoderTests.cpp:62

• out.find('\xFF') has the same signed/unsigned char portability issue as the invalid-byte injection above and can also trigger -Werror constant-conversion warnings on some compilers. Use an explicit static_cast<char>(0xFF) when searching for the raw byte.

        EXPECT_NE(out.find("\xEF\xBF\xBD"), std::string::npos)
            << "Malformed UTF-8 should be replaced with U+FFFD";
        EXPECT_EQ(out.find('\xFF'), std::string::npos)
            << "Raw invalid byte must not survive in the output";

tests/unittests/PayloadDecoderTests.cpp:48

• "Bad\xFFName" relies on implementation-defined conversion of 0xFF into a char within a string literal. With the repo’s -Werror (see top-level CMakeLists), some toolchains warn on out-of-range/constant conversion for such escapes, which can break CI. Build the invalid UTF-8 byte via push_back(static_cast<char>(0xFF)) instead (similar to AIJsonSerializerTests).

This issue also appears on line 59 of the same file.

    CsProtocol::Record record = MakeMinimalRecord();
    // 0xFF is never a valid UTF-8 byte.
    record.name = std::string("Bad\xFFName");

tests/unittests/PayloadDecoderTests.cpp:43

• The behavior change in this PR is applied to both DecodeRecord and DecodeRequest, but the new regression tests only exercise DecodeRecord. There are call sites for DecodeRequest in functests, but they don’t assert the no-throw / U+FFFD replacement behavior. Adding a DecodeRequest_InvalidUtf8_DoesNotThrow test (constructing a minimal request buffer containing a record with an invalid UTF-8 byte) would better lock in the crash fix on the primary entry point described in the PR.

// A telemetry event field can legitimately contain bytes that are not valid
// UTF-8. nlohmann::json::dump() defaults to error_handler_t::strict, which
// throws type_error.316 on such input. Because DecodeRecord/DecodeRequest run
// on the Diagnostic Data Viewer path inside the hosting process, an unhandled
// throw terminates that process (Watson crash). These tests lock in the
// error_handler_t::replace behavior: no throw, and the malformed byte is
// emitted as the U+FFFD replacement character (EF BF BD).
TEST(PayloadDecoderTests, DecodeRecord_InvalidUtf8_DoesNotThrow)

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[lkarra2]

Addressed the low-confidence review notes on tests/unittests/PayloadDecoderTests.cpp in d99fdff:

• -Werror char conversion (lines 48/59 and 62): the invalid UTF-8 byte is now injected via push_back(static_cast<char>(0xFF)) and searched for with static_cast<char>(0xFF), so the tests no longer rely on implementation-defined conversion of \xFF in a string/char literal.
• DecodeRequest coverage: DecodeRequest applies the identical dump(..., json::error_handler_t::replace) call to the same to_json(Record) output that DecodeRecord serializes, so the no-throw / U+FFFD behavior is already locked in by DecodeRecord_InvalidUtf8_DoesNotThrow. A true DecodeRequest input is a full, framed (and normally compressed) HTTP request body — it is only ever fed real upload payloads (see tests/functests/EventDecoderListener.cpp), and its internal boundary-scanning decoder plus unconditional ext[0] indexing make a hand-built buffer unsafe to construct in a focused unit test. End-to-end DecodeRequest exercise therefore belongs in functests rather than this unit test.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

tests/unittests/PayloadDecoderTests.cpp:84

• Like the invalid-UTF8 case above, this test currently skips all assertions when decoded is false, which means it won’t fail if the full decoder unexpectedly starts returning false on feature-enabled builds. Consider asserting decoded/out based on HAVE_MAT_ZLIB + HAVE_MAT_JSONHPP so the test reliably validates the real implementation when it is compiled in.

    if (decoded)
    {
        EXPECT_NE(out.find("Valid.Event"), std::string::npos);
        EXPECT_EQ(out.find("\xEF\xBF\xBD"), std::string::npos)
            << "Valid UTF-8 must not be altered";

tests/unittests/PayloadDecoderTests.cpp:68

• This test only asserts behavior when decoded is true, so it can still pass even if the full decoder unexpectedly returns false (e.g., due to a regression) on builds that actually have Zlib + json.hpp enabled. Since the implementation is stubbed specifically when HAVE_MAT_ZLIB/HAVE_MAT_JSONHPP are missing, you can make the test deterministic by asserting decoded (and out) based on those feature macros.

This issue also appears on line 80 of the same file.

    // When the SDK is built with JSON + Zlib support the real decoder runs and
    // must have replaced the bad byte. In a stubbed build DecodeRecord returns
    // false with an empty string, in which case the no-throw guarantee above is
    // what this test protects.
    if (decoded)