winget-cli

commit 0073a6e184ca4dd00facfe53236237627707e26a
parent 4fda2dab3d5296f2e0cf131170d04b15d874880b
Author: Flor Chacón <14323496+florelis@users.noreply.github.com>
Date:   Thu, 17 Aug 2023 14:02:26 -0700

Attempt to prevent crash in `TelemetryTraceLogger::InitializeInternal()` (#3527)


Diffstat:
M
src/AppInstallerCommonCore/AppInstallerTelemetry.cpp
 | 
13
+++++++++++--
M
src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h
 | 
18
++++++++++++++----

2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp
@@ -31,6 +31,7 @@ namespace AppInstaller::Logging
         static const uint32_t s_RootExecutionId = 0;
         static std::atomic_uint32_t s_subExecutionId{ s_RootExecutionId };
 
+        // Data that is needed by AnonymizeString
         constexpr std::wstring_view s_UserProfileReplacement = L"%USERPROFILE%"sv;
 
         // TODO: Temporary code to keep existing telemetry behavior
@@ -121,6 +122,8 @@ namespace AppInstaller::Logging
     {
         if (!m_isInitialized)
         {
+            // Only initialize if we already have the user settings, so that we can respect the telemetry setting.
+            // We may not yet have the user settings if we are trying to report an error while reading them.
             auto userSettings = Settings::TryGetUser();
             if (userSettings)
             {
@@ -809,7 +812,6 @@ namespace AppInstaller::Logging
     void TelemetryTraceLogger::InitializeInternal(const AppInstaller::Settings::UserSettings& userSettings)
     {
         m_isSettingEnabled = !userSettings.Get<Settings::Setting::TelemetryDisable>();
-        m_userProfile = Runtime::GetPathTo(Runtime::PathName::UserProfile).wstring();
         m_isInitialized = true;
     }
 
@@ -820,7 +822,11 @@ namespace AppInstaller::Logging
 
     std::wstring TelemetryTraceLogger::AnonymizeString(std::wstring_view input) const noexcept try
     {
-        return Utility::ReplaceWhileCopying(input, m_userProfile, s_UserProfileReplacement);
+        // GetPathTo() may need to read the settings, so this function should only be called after settings are initialized.
+        // To ensure that, this function is only called when emitting an event, and we disable the telemetry until settings are ready.
+        static const std::wstring s_UserProfile = Runtime::GetPathTo(Runtime::PathName::UserProfile).wstring();
+
+        return Utility::ReplaceWhileCopying(input, s_UserProfile, s_UserProfileReplacement);
     }
     catch (...) { return std::wstring{ input }; }
 
@@ -843,6 +849,9 @@ namespace AppInstaller::Logging
         }
         else
         {
+            // For the global telemetry object, we may not have yet read the settings file.
+            // In that case, we will not be able to initialize it, so we need to try it
+            // each time we get the object.
             static TelemetryTraceLogger processGlobalTelemetry(/* useSummary */ false);
             processGlobalTelemetry.TryInitialize();
             return processGlobalTelemetry;
diff --git a/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h b/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h
@@ -154,6 +154,10 @@ namespace AppInstaller::Logging
         void Initialize();
 
         // Try to capture if UserSettings is enabled and set user profile path, returns whether the action is successfully completed.
+        // There is a possible circular dependency with the user settings. When initializing the telemetry, we need to read the settings
+        // to make sure it's not disabled, but a failure when reading the settings would trigger a telemetry event. We work around that
+        // by avoiding initialization (and thus disabling telemetry) until we have successfully read the settings. Subsequent calls to
+        // TryInitialize() would finish the initialization.
         bool TryInitialize();
 
         // Store the passed in name of the Caller for COM calls
@@ -255,6 +259,7 @@ namespace AppInstaller::Logging
     protected:
         bool IsTelemetryEnabled() const noexcept;
 
+        // Initializes flags that determine whether telemetry is enabled.
         void InitializeInternal(const AppInstaller::Settings::UserSettings& userSettings);
 
         // Used to anonymize a string to the best of our ability.
@@ -262,8 +267,16 @@ namespace AppInstaller::Logging
         std::wstring AnonymizeString(const wchar_t* input) const noexcept;
         std::wstring AnonymizeString(std::wstring_view input) const noexcept;
 
-        bool m_isSettingEnabled = true;
+        // Flags used to determine whether to send telemetry. All of them are set during initialization and
+        // are CopyConstructibleAtomic to minimize the impact of multiple simultaneous initialization attempts.
+        // m_isSettingEnabled starts as false so we can don't send telemetry until we have read the
+        // settings and confirmed that it is enabled.
+        CopyConstructibleAtomic<bool> m_isSettingEnabled{ false };
+
+        // We may decide to disable telemetry at runtime, for example, for command line completion.
         CopyConstructibleAtomic<bool> m_isRuntimeEnabled{ true };
+
+        // We wait for initialization of the other flags before sending any events.
         CopyConstructibleAtomic<bool> m_isInitialized{ false };
 
         CopyConstructibleAtomic<uint32_t> m_executionStage{ 0 };
@@ -273,9 +286,6 @@ namespace AppInstaller::Logging
         std::wstring m_telemetryCorrelationJsonW = L"{}";
         std::string m_caller;
 
-        // Data that is needed by AnonymizeString
-        std::wstring m_userProfile;
-
         bool m_useSummary = true;
         mutable TelemetrySummary m_summary;