From 9afec42ce662117543e3cabb4fb8b5739accd429 Mon Sep 17 00:00:00 2001 From: star-du Date: Fri, 30 Oct 2026 16:11:25 +0800 Subject: [PATCH 1/2] Fix missing instance ticket token JNI binding Implement the existing Java native contract through the instance auth tokens controller while preserving status and conversion failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- lib/jni/LogManager_jni.cpp | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lib/jni/LogManager_jni.cpp b/lib/jni/LogManager_jni.cpp index e400b6f83..cab9d7535 100644 --- a/lib/jni/LogManager_jni.cpp +++ b/lib/jni/LogManager_jni.cpp @@ -1242,6 +1242,36 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na } return logManager->ResumeTransmission(); } + +extern "C" JNIEXPORT jint JNICALL +Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_nativeSetIntTicketToken( + JNIEnv* env, + jobject /* thiz */, + jlong nativeLogManager, + jint type, + jstring tokenValue) +{ + auto logManager = getLogManager(nativeLogManager); + if (!logManager) + { + return STATUS_EFAIL; + } + + auto ticketValue = JStringToStdString(env, tokenValue); + if (env->ExceptionCheck()) + { + return STATUS_EFAIL; + } + + auto controller = logManager->GetAuthTokensController(); + if (!controller) + { + return STATUS_EFAIL; + } + return static_cast(controller->SetTicketToken( + static_cast(type), ticketValue.c_str())); +} + extern "C" JNIEXPORT jint JNICALL Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_nativeSetTransmitProfileTP( JNIEnv* env, From 8e3101a02159e676cf6800678ee34849e503f5a3 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 5 Nov 2026 00:26:57 -0600 Subject: [PATCH 2/2] Prevent token refresh races and duplicate ticket growth Prevent token refresh and clear operations from racing with HTTP header encoding and event decoration. Use an internal SDK-wide lock to preserve the public API and object layout, and document the synchronization requirement for legacy live-reference getters. Register each user ticket key only once so repeated refreshes do not grow memory and every subsequent event payload. Cover refresh, clear/re-registration, and concurrent writers with active SDK readers. Validation: 51 targeted tests passed with VS 2026; 42 token/encoder/decorator tests passed under WSL Clang ThreadSanitizer. Public-header gates, changed-lines clang-format, diff whitespace, and CI misspell passed. The original code fails the new deduplication test and reports a token race under ThreadSanitizer. A separate pre-existing LogManagerImpl construction vptr race was reproduced on the original code and is not suppressed or modified here. Files changed: - lib/api/AuthTokensController.cpp - lib/api/AuthTokensController.hpp - lib/decorators/BaseDecorator.cpp - lib/http/HttpRequestEncoder.cpp - lib/include/public/IAuthTokensController.hpp - tests/unittests/HttpRequestEncoderTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d8996b4-37b3-4519-870a-4ce3be753d08 --- lib/api/AuthTokensController.cpp | 15 +- lib/api/AuthTokensController.hpp | 6 + lib/decorators/BaseDecorator.cpp | 4 +- lib/http/HttpRequestEncoder.cpp | 7 +- lib/include/public/IAuthTokensController.hpp | 4 + tests/unittests/HttpRequestEncoderTests.cpp | 177 ++++++++++++++++++- 6 files changed, 208 insertions(+), 5 deletions(-) diff --git a/lib/api/AuthTokensController.cpp b/lib/api/AuthTokensController.cpp index 14ccc70f6..4a7778aaa 100644 --- a/lib/api/AuthTokensController.cpp +++ b/lib/api/AuthTokensController.cpp @@ -22,10 +22,17 @@ namespace MAT_NS_BEGIN { LOG_TRACE("destructor"); } + std::mutex& AuthTokensController::GetMutex() + { + static std::mutex mutex; + return mutex; + } + status_t AuthTokensController::SetTicketToken(TicketType type, char const* tokenValue) { if (nullptr != tokenValue) { + std::lock_guard lock(GetMutex()); if (type == TicketType::TicketType_MSA_Device || type == TicketType::TicketType_AAD || type == TicketType::TicketType_XAuth_Device || @@ -35,7 +42,10 @@ namespace MAT_NS_BEGIN { } else { - m_tickets.push_back(TICKETS_PREPEND_STRING + std::to_string(type)); + if (m_userTokens.find(type) == m_userTokens.end()) + { + m_tickets.push_back(TICKETS_PREPEND_STRING + std::to_string(type)); + } m_userTokens[type] = std::string(tokenValue); } return STATUS_SUCCESS; @@ -45,6 +55,7 @@ namespace MAT_NS_BEGIN { status_t AuthTokensController::Clear() { + std::lock_guard lock(GetMutex()); m_deviceTokens.clear(); m_userTokens.clear(); m_tickets.clear(); @@ -53,12 +64,14 @@ namespace MAT_NS_BEGIN { status_t AuthTokensController::SetStrictMode(bool value) { + std::lock_guard lock(GetMutex()); m_IsStrictModeEnabled = value; return STATUS_EFAIL; } bool AuthTokensController::GetStrictMode() { + std::lock_guard lock(GetMutex()); return m_IsStrictModeEnabled; } diff --git a/lib/api/AuthTokensController.hpp b/lib/api/AuthTokensController.hpp index 48a67dc98..e222293b2 100644 --- a/lib/api/AuthTokensController.hpp +++ b/lib/api/AuthTokensController.hpp @@ -11,6 +11,7 @@ #include "IAuthTokensController.hpp" #include +#include #include // *INDENT-OFF* @@ -24,6 +25,11 @@ namespace MAT_NS_BEGIN public: AuthTokensController(); + + // Hold this lock throughout SDK reads of the reference-returning public getters. + // Sharing it across controllers preserves the public interface and object layout. + static std::mutex& GetMutex(); + /// /// Destroy the telemetry logging system instance. Calls `FlushAndTeardown()` implicitly. /// diff --git a/lib/decorators/BaseDecorator.cpp b/lib/decorators/BaseDecorator.cpp index ead86f3ab..92ab3aa10 100644 --- a/lib/decorators/BaseDecorator.cpp +++ b/lib/decorators/BaseDecorator.cpp @@ -2,8 +2,9 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // SPDX-License-Identifier: Apache-2.0 // -#include "mat/config.h" #include "BaseDecorator.hpp" +#include "api/AuthTokensController.hpp" +#include "mat/config.h" namespace MAT_NS_BEGIN { @@ -54,6 +55,7 @@ namespace MAT_NS_BEGIN { } //set Tickets + std::lock_guard lock(AuthTokensController::GetMutex()); if ((m_owner.GetAuthTokensController()) && (m_owner.GetAuthTokensController()->GetTickets().size() > 0)) { diff --git a/lib/http/HttpRequestEncoder.cpp b/lib/http/HttpRequestEncoder.cpp index b612b66dd..00713c76a 100644 --- a/lib/http/HttpRequestEncoder.cpp +++ b/lib/http/HttpRequestEncoder.cpp @@ -4,8 +4,9 @@ // #include "HttpRequestEncoder.hpp" -#include "utils/StringUtils.hpp" +#include "api/AuthTokensController.hpp" #include "pal/PAL.hpp" +#include "utils/StringUtils.hpp" #include #include @@ -55,7 +56,7 @@ namespace MAT_NS_BEGIN { ctx->httpRequest->GetHeaders().set("Content-Type", "application/bond-compact-binary"); ctx->httpRequest->GetHeaders().set("Upload-Time", toString(PAL::getUtcSystemTimeMs())); - + std::unique_lock tokensLock(AuthTokensController::GetMutex()); if (GetAuthTokensController() != nullptr && GetAuthTokensController()->GetDeviceTokens().size() > 0) { std::map& map = GetAuthTokensController()->GetDeviceTokens(); @@ -134,6 +135,8 @@ namespace MAT_NS_BEGIN { ctx->httpRequest->GetHeaders().set("Tickets", ticketHeader); } } + tokensLock.unlock(); + //strict mode if (GetAuthTokensController() != nullptr && true == GetAuthTokensController()->GetStrictMode()) { diff --git a/lib/include/public/IAuthTokensController.hpp b/lib/include/public/IAuthTokensController.hpp index 9140580ef..81d787733 100644 --- a/lib/include/public/IAuthTokensController.hpp +++ b/lib/include/public/IAuthTokensController.hpp @@ -15,6 +15,10 @@ namespace MAT_NS_BEGIN /// /// This class is used to manage the Events logging system /// + /// + /// The reference-returning getters expose live state. Application code must not + /// access these references concurrently with token updates or Clear(). + /// class MATSDK_LIBABI IAuthTokensController { public: diff --git a/tests/unittests/HttpRequestEncoderTests.cpp b/tests/unittests/HttpRequestEncoderTests.cpp index 5ee190821..531fe9a23 100644 --- a/tests/unittests/HttpRequestEncoderTests.cpp +++ b/tests/unittests/HttpRequestEncoderTests.cpp @@ -1,9 +1,14 @@ // Copyright (c) Microsoft Corporation. All rights reserved. +#include "api/AuthTokensController.hpp" #include "common/Common.hpp" #include "common/MockIHttpClient.hpp" -#include "http/HttpRequestEncoder.hpp" +#include "common/MockITelemetrySystem.hpp" #include "config/RuntimeConfig_Default.hpp" +#include "decorators/BaseDecorator.hpp" +#include "http/HttpRequestEncoder.hpp" + +#include using namespace testing; using namespace MAT; @@ -126,3 +131,173 @@ TEST_F(HttpRequestEncoderTests, DispatchDataViewerEventCorrectly) EXPECT_THAT(mockEncoder.dataPacket, Eq(std::vector{1, 127, 255})); } + +namespace +{ + class TokenLogManager : public NullLogManager + { + public: + AuthTokensController tokens; + + IAuthTokensController* GetAuthTokensController() override + { + return &tokens; + } + }; + + class TokenTelemetrySystem : public MockITelemetrySystem + { + public: + TokenLogManager manager; + + ILogManager& getLogManager() override + { + return manager; + } + }; +} + +TEST(AuthTokensControllerTests, RefreshingUserTokensDoesNotDuplicateTicketKeys) +{ + TokenLogManager manager; + auto& tokens = manager.tokens; + const TicketType userTypes[] = {TicketType_MSA_User, TicketType_XAuth_User, TicketType_AAD_User}; + for (int refresh = 0; refresh < 100; ++refresh) + { + const auto value = std::to_string(refresh); + for (auto type : userTypes) + { + ASSERT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(type, value.c_str())); + } + ASSERT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(TicketType_AAD, value.c_str())); + } + + const std::vector expectedTickets{"10001", "10003", "10005"}; + EXPECT_EQ(expectedTickets, tokens.GetTickets()); + for (auto type : userTypes) + { + EXPECT_EQ("99", tokens.GetUserTokens().at(type)); + } + EXPECT_EQ("99", tokens.GetDeviceTokens().at(TicketType_AAD)); + + BaseDecorator decorator(manager); + CsProtocol::Record record; + ASSERT_TRUE(decorator.decorate(record)); + ASSERT_EQ(1u, record.extProtocol.size()); + ASSERT_EQ(1u, record.extProtocol[0].ticketKeys.size()); + EXPECT_EQ(expectedTickets, record.extProtocol[0].ticketKeys[0]); + + ASSERT_EQ(STATUS_SUCCESS, tokens.Clear()); + EXPECT_TRUE(tokens.GetTickets().empty()); + EXPECT_TRUE(tokens.GetUserTokens().empty()); + EXPECT_TRUE(tokens.GetDeviceTokens().empty()); + ASSERT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(TicketType_MSA_User, "after-clear")); + ASSERT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(TicketType_MSA_User, "refreshed")); + EXPECT_THAT(tokens.GetTickets(), ElementsAre("10001")); + EXPECT_EQ("refreshed", tokens.GetUserTokens().at(TicketType_MSA_User)); +} + +TEST_F(HttpRequestEncoderTests, TokenRefreshIsSafeDuringEncodingAndDecoration) +{ + TokenTelemetrySystem tokenSystem; + auto& tokens = tokenSystem.manager.tokens; + const std::string firstValue(256, 'a'); + const std::string secondValue(256, 'b'); + ASSERT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(TicketType_MSA_User, firstValue.c_str())); + ASSERT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(TicketType_XAuth_User, firstValue.c_str())); + ASSERT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(TicketType_AAD, firstValue.c_str())); + + HttpRequestEncoder tokenEncoder(tokenSystem, mockHttpClient); + BaseDecorator decorator(tokenSystem.manager); + std::promise start; + auto ready = start.get_future().share(); + auto firstWriter = std::async(std::launch::async, [&]() + { + ready.wait(); + for (int refresh = 0; refresh < 2000; ++refresh) + { + const auto& value = refresh % 2 == 0 ? firstValue : secondValue; + EXPECT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(TicketType_MSA_User, value.c_str())); + EXPECT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(TicketType_AAD, value.c_str())); + } }); + auto secondWriter = std::async(std::launch::async, [&]() + { + ready.wait(); + for (int refresh = 0; refresh < 2000; ++refresh) + { + const auto& value = refresh % 2 == 0 ? firstValue : secondValue; + EXPECT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(TicketType_XAuth_User, value.c_str())); + } }); + + const auto ticketHeader = [](const std::string& msa, const std::string& xauth) + { + return "\"10001\"=\"p:" + msa + "\";\"10003\"=\"x:XBL3.0 x=" + xauth + "\""; + }; + start.set_value(); + for (int event = 0; event < 1000; ++event) + { + auto ctx = std::make_shared(); + tokenEncoder.encode(ctx); + const auto* request = static_cast(ctx->httpRequest); + EXPECT_THAT(request->m_headers, Contains(Pair("Aad-Token", AnyOf(Eq(firstValue), Eq(secondValue))))); + EXPECT_THAT(request->m_headers, Contains(Pair("Tickets", + AnyOf(Eq(ticketHeader(firstValue, firstValue)), Eq(ticketHeader(firstValue, secondValue)), + Eq(ticketHeader(secondValue, firstValue)), Eq(ticketHeader(secondValue, secondValue)))))); + + CsProtocol::Record record; + ASSERT_TRUE(decorator.decorate(record)); + ASSERT_EQ(1u, record.extProtocol.size()); + ASSERT_EQ(1u, record.extProtocol[0].ticketKeys.size()); + EXPECT_THAT(record.extProtocol[0].ticketKeys[0], ElementsAre("10001", "10003")); + } + firstWriter.get(); + secondWriter.get(); +} + +TEST_F(HttpRequestEncoderTests, TokenClearIsSafeDuringEncodingAndDecoration) +{ + TokenTelemetrySystem tokenSystem; + auto& tokens = tokenSystem.manager.tokens; + HttpRequestEncoder tokenEncoder(tokenSystem, mockHttpClient); + BaseDecorator decorator(tokenSystem.manager); + const std::string value(256, 'a'); + std::promise start; + auto ready = start.get_future().share(); + auto writer = std::async(std::launch::async, [&]() + { + ready.wait(); + for (int refresh = 0; refresh < 2000; ++refresh) + { + EXPECT_EQ(STATUS_SUCCESS, tokens.Clear()); + EXPECT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(TicketType_MSA_User, value.c_str())); + EXPECT_EQ(STATUS_SUCCESS, tokens.SetTicketToken(TicketType_AAD, value.c_str())); + tokens.SetStrictMode(refresh % 2 == 0); + } }); + + start.set_value(); + for (int event = 0; event < 1000; ++event) + { + auto ctx = std::make_shared(); + tokenEncoder.encode(ctx); + const auto* request = static_cast(ctx->httpRequest); + auto deviceHeader = request->m_headers.find("Aad-Token"); + if (deviceHeader != request->m_headers.end()) + { + EXPECT_EQ(value, deviceHeader->second); + } + auto userHeader = request->m_headers.find("Tickets"); + if (userHeader != request->m_headers.end()) + { + EXPECT_EQ("\"10001\"=\"p:" + value + "\"", userHeader->second); + } + + CsProtocol::Record record; + ASSERT_TRUE(decorator.decorate(record)); + if (!record.extProtocol.empty()) + { + ASSERT_EQ(1u, record.extProtocol[0].ticketKeys.size()); + EXPECT_THAT(record.extProtocol[0].ticketKeys[0], ElementsAre("10001")); + } + } + writer.get(); +}