Skip to content

Make sure EventPipe streaming thread won't write session->streaming_thread after session free. - #58615

Merged
lateralusX merged 1 commit into
dotnet:mainfrom
lateralusX:lateralusX/fix-issue-57461
Sep 6, 2021
Merged

Make sure EventPipe streaming thread won't write session->streaming_thread after session free.#58615
lateralusX merged 1 commit into
dotnet:mainfrom
lateralusX:lateralusX/fix-issue-57461

Conversation

@lateralusX

@lateralusXlateralusX commented Sep 3, 2021

Copy link
Copy Markdown
Member

In case where ep_disable is called by a different thread (stop tracing IPC command) there was a race between streaming thread setting session->streaming_thread to NULL and IPC command triggering a call to disable_holding_lock and freeing session.

Resetting the session->streaming_thread in streaming thread must happens before it signals its shutdown event to prevent the race causing heap corruption, writing NULL into freed memory.

Probably root cause of #57461.

…hread after session free.
In case where ep_disable is called by a different thread (close IPC command)
there was a race between streaming threads setting
session->streaming_thread to NULL and IPC command triggering a call
to disable_holding_lock and freeing session.
Resetting the streaming_thread in streaming thread must happens before
it signals its shutdown event to prevent the race.
@lateralusX

Copy link
Copy Markdown
MemberAuthor

//CC @josalem

@josalemjosalem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm getting major déjà vu from this change, but I can't seem to find a previous patch that matches this change.

That aside, this LGTM. Thanks!

@josalem

Copy link
Copy Markdown
Contributor

We should try to backport this change to release/6.0 for rc2.

@lateralusX

lateralusX commented Sep 6, 2021

Copy link
Copy Markdown
MemberAuthor

I'm getting major déjà vu from this change, but I can't seem to find a previous patch that matches this change.

That aside, this LGTM. Thanks!

Yes, I fixed another incarnation of this issue in an error case I hit locally where we wrote to memory after free on the same thread, #53886 but missed that the same scenario might be hit when the streaming thread is racing with IPC stop tracing command on different thread.

@lateralusX

Copy link
Copy Markdown
MemberAuthor

/backport to release/6.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1204902529

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@lateralusX@josalem