Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Aug 17, 2023

Conversation

florelis
Copy link
Member

@florelis florelis commented Aug 16, 2023

We have been getting reports of heap corruption crashes on AppInstaller::Logging::TelemetryTraceLogger::InitializeInternal in calls to the COM API, with a recent spike in number of hits.
This is probably due to trying to initialize the trace logger from two places at the same time , so I'm adding synchronization there.
I was unable to get a repro to confirm this was the issue and test that the change fixed it, so it may not actually do anything. See https://task.ms/38253096

Microsoft Reviewers: Open in CodeFlow

Edit: Changed per @JohnMcPMS suggestion.

  • Made m_isSettingEnabled a CopyConstructibleAtomic<bool>
  • Changed m_isSettingEnabled default value to false, so that we only set it to true after successfully readin the settings
  • Moved m_userProfile to be a static in AnonymizeString(). This function is only called when emitting an event, and that happens only if we finished the initialization, which ensures that the use of the settings when getting the user profile path is safe.

@florelis florelis requested a review from a team as a code owner August 16, 2023 23:40
@@ -111,6 +111,7 @@ namespace AppInstaller::Logging

void TelemetryTraceLogger::Initialize()
{
std::lock_guard<std::mutex> lockInitialization{ *m_initializationLock };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locks here are just reverting back to the deadlock potential that we previously had (when a "magic static" was used to synchronize the global telemetry object initialization). I think we need a design change.

I would suggest making m_isSettingEnabled a CopyConstructibleAtomic<bool> so that multiple initialization attempts are less impactful (last writer wins and the changes are more readily observable).

Then I would move m_userProfile to not be a member of this class. It can either just be resolved every time, or if we want to keep the optimization it can be a static in a standalone function. We obviously don't expect it to change over the life of a single telemetry object now, and I wouldn't expect it to change over the life of a process either (at least not observably). It does end up using settings in retrieving the path (😭), so the static could guard against that (and IsTelemetryEnabled could require that settings be prepared as well).

Finally, a comment somewhere to indicate just how everything here is balanced on a razor's edge in terms of potential deadlocking.

@github-actions

This comment has been minimized.

@florelis florelis changed the title Add synchronization to telemetry initialization Attempt to prevent crash in TelemetryTraceLogger::InitializeInternal() Aug 17, 2023
@florelis florelis merged commit 0073a6e into microsoft:master Aug 17, 2023
@florelis florelis deleted the initializeInternal branch August 17, 2023 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants