From df5f07f0ff9b9f979f6050b0ff6b82b473d92d72 Mon Sep 17 00:00:00 2001 From: manaswi karra Date: Tue, 28 Jul 2026 12:49:12 -0700 Subject: [PATCH 1/3] Replace malformed UTF-8 instead of throwing in PayloadDecoder --- lib/decoder/PayloadDecoder.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/decoder/PayloadDecoder.cpp b/lib/decoder/PayloadDecoder.cpp index 5789d5939..e4127f0af 100644 --- a/lib/decoder/PayloadDecoder.cpp +++ b/lib/decoder/PayloadDecoder.cpp @@ -563,7 +563,13 @@ namespace MAT_NS_BEGIN { if (result) { - out = j.dump(2); + // Use error_handler_t::replace so that malformed UTF-8 in the + // decoded telemetry payload is replaced with U+FFFD instead of + // throwing nlohmann::json::type_error (id 316). Telemetry event + // strings can legitimately contain non-UTF-8 bytes; without this + // the unhandled C++ exception terminates the hosting process. + // Mirrors AIJsonSerializer::handleSerialize. + out = j.dump(2, ' ', false, json::error_handler_t::replace); } return result; @@ -580,7 +586,9 @@ namespace MAT_NS_BEGIN { nlohmann::json j; to_json(j, in); - std::string s = j.dump(4); + // See DecodeRequest above: replace malformed UTF-8 rather than + // throwing so a bad record cannot terminate the process. + std::string s = j.dump(4, ' ', false, json::error_handler_t::replace); out.assign(s.begin(), s.end()); return true; From aba19d0d33bb4e542ae7bf52a5c0fcc05894f189 Mon Sep 17 00:00:00 2001 From: manaswi karra Date: Tue, 28 Jul 2026 13:22:44 -0700 Subject: [PATCH 2/3] Address review: reword comment and add PayloadDecoder regression tests - Reword the DecodeRequest comment to state the local rationale for error_handler_t::replace instead of referencing the HAVE_MAT_AI-gated AIJsonSerializer, which is not part of the default OSS build. - Add PayloadDecoderTests covering the invalid-UTF-8 regression: a record whose string field carries a non-UTF-8 byte must be decoded without throwing type_error.316, and the bad byte must surface as U+FFFD. Also verifies valid UTF-8 is left untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ab3a16b-98f7-4fa7-a933-684b34196436 --- lib/decoder/PayloadDecoder.cpp | 1 - tests/unittests/CMakeLists.txt | 1 + tests/unittests/PayloadDecoderTests.cpp | 81 +++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 tests/unittests/PayloadDecoderTests.cpp diff --git a/lib/decoder/PayloadDecoder.cpp b/lib/decoder/PayloadDecoder.cpp index e4127f0af..6d86a7057 100644 --- a/lib/decoder/PayloadDecoder.cpp +++ b/lib/decoder/PayloadDecoder.cpp @@ -568,7 +568,6 @@ namespace MAT_NS_BEGIN { // throwing nlohmann::json::type_error (id 316). Telemetry event // strings can legitimately contain non-UTF-8 bytes; without this // the unhandled C++ exception terminates the hosting process. - // Mirrors AIJsonSerializer::handleSerialize. out = j.dump(2, ' ', false, json::error_handler_t::replace); } diff --git a/tests/unittests/CMakeLists.txt b/tests/unittests/CMakeLists.txt index 945fc23df..7233d2920 100644 --- a/tests/unittests/CMakeLists.txt +++ b/tests/unittests/CMakeLists.txt @@ -40,6 +40,7 @@ set(SRCS OfflineStorageTests_Room.cpp OfflineStorageTests_SQLite.cpp PackagerTests.cpp + PayloadDecoderTests.cpp PalTests.cpp RouteTests.cpp StringUtilsTests.cpp diff --git a/tests/unittests/PayloadDecoderTests.cpp b/tests/unittests/PayloadDecoderTests.cpp new file mode 100644 index 000000000..b71ed3033 --- /dev/null +++ b/tests/unittests/PayloadDecoderTests.cpp @@ -0,0 +1,81 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +#include "common/Common.hpp" +#include "PayloadDecoder.hpp" + +using namespace testing; +using namespace MAT; + +namespace +{ + // Builds a minimally-populated Common Schema record. to_json() in the + // PayloadDecoder unconditionally dereferences element [0] of every ext + // vector, so all seven must contain at least one element for serialization + // to succeed. + CsProtocol::Record MakeMinimalRecord() + { + CsProtocol::Record record; + record.ver = "3.0"; + record.name = "Test.Event"; + record.time = 0; + record.iKey = "o:0000"; + record.baseType = "custom"; + record.extProtocol.push_back(CsProtocol::Protocol{}); + record.extUser.push_back(CsProtocol::User{}); + record.extDevice.push_back(CsProtocol::Device{}); + record.extOs.push_back(CsProtocol::Os{}); + record.extApp.push_back(CsProtocol::App{}); + record.extNet.push_back(CsProtocol::Net{}); + record.extSdk.push_back(CsProtocol::Sdk{}); + return record; + } +} + +// 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) +{ + CsProtocol::Record record = MakeMinimalRecord(); + // 0xFF is never a valid UTF-8 byte. + record.name = std::string("Bad\xFFName"); + + std::string out; + bool decoded = false; + EXPECT_NO_THROW({ decoded = exporters::DecodeRecord(record, out); }); + + // 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) + { + 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"; + } +} + +TEST(PayloadDecoderTests, DecodeRecord_ValidUtf8_IsPreserved) +{ + CsProtocol::Record record = MakeMinimalRecord(); + record.name = "Valid.Event"; + + std::string out; + bool decoded = false; + EXPECT_NO_THROW({ decoded = exporters::DecodeRecord(record, out); }); + + 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"; + } +} From d99fdff96ad242fdaaca197993a0b784c528a24c Mon Sep 17 00:00:00 2001 From: manaswi karra Date: Tue, 28 Jul 2026 15:49:57 -0700 Subject: [PATCH 3/3] Harden PayloadDecoder tests against -Werror char conversion Address review feedback on the regression tests: - Inject the invalid UTF-8 byte via push_back(static_cast(0xFF)) and search for it with static_cast(0xFF) instead of a '\xFF' string/char literal, which relies on implementation-defined char conversion and can trip -Werror constant-conversion on some toolchains. - Drop the product-specific phrasing from the test comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6ab3a16b-98f7-4fa7-a933-684b34196436 --- tests/unittests/PayloadDecoderTests.cpp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/tests/unittests/PayloadDecoderTests.cpp b/tests/unittests/PayloadDecoderTests.cpp index b71ed3033..8e8051cee 100644 --- a/tests/unittests/PayloadDecoderTests.cpp +++ b/tests/unittests/PayloadDecoderTests.cpp @@ -36,15 +36,20 @@ namespace // 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). +// on the decode path inside the hosting process, an unhandled throw terminates +// that process. 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) { CsProtocol::Record record = MakeMinimalRecord(); - // 0xFF is never a valid UTF-8 byte. - record.name = std::string("Bad\xFFName"); + // Build the field with an explicit 0xFF byte (never valid UTF-8). A string + // literal escape ("...\xFF...") would rely on implementation-defined char + // conversion and can trip -Werror constant-conversion on some toolchains. + std::string name = "Bad"; + name.push_back(static_cast(0xFF)); + name += "Name"; + record.name = name; std::string out; bool decoded = false; @@ -58,7 +63,7 @@ TEST(PayloadDecoderTests, DecodeRecord_InvalidUtf8_DoesNotThrow) { 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) + EXPECT_EQ(out.find(static_cast(0xFF)), std::string::npos) << "Raw invalid byte must not survive in the output"; } }