Skip to content

Fix memory errors related to EventPipe setup with COMPlus_EventPipeConfig#44068

Merged
josalem merged 1 commit intodotnet:masterfrom
gbalykov:fix-etw-setup-memory-errors
Dec 17, 2020
Merged

Fix memory errors related to EventPipe setup with COMPlus_EventPipeConfig#44068
josalem merged 1 commit intodotnet:masterfrom
gbalykov:fix-etw-setup-memory-errors

Conversation

@gbalykov
Copy link
Member

@gbalykov gbalykov commented Oct 30, 2020

  • XplatEventLoggerConfiguration configuration owns strings passed to EventPipeProviderConfiguration pProviders, and configurations are freed before pProviders are copied to EventPipeSessionProvider
  • NewArrayHolder shoud be used for pProviders to fix memory leak (delete[] instead of delete)

Bug shows itself in release builds with

COMPlus_EnableEventPipe=1 COMPlus_EventPipeConfig="*:0xFFFFFFFFFFFFFFFF:5" COMPlus_EventPipeOutputPath=`pwd`/trace.nettrace COMPlus_EventPipeRundown=0 ./corerun ~/hwapp2.dll

Correct, passed in env variable:

m_pProviderName = 0x5555557cce10 u"*"

Incorrect, processed inside EventPipe::Enable:

m_pProviderName = 0x5555557cce10 u"츰啼啕"

cc @alpencolt

@ghost
Copy link

ghost commented Oct 30, 2020

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

@maryamariyan
Copy link
Contributor

maryamariyan commented Oct 30, 2020 •
edited
Loading

@alpencolt is this PR for area-Tizen? (Referring to area-owners page).

@alpencolt
Copy link

alpencolt commented Nov 2, 2020

@maryamariyan it's for all platforms

@gbalykov gbalykov mentioned this pull request Nov 19, 2020
Open
@jkotas jkotas requested review from noahfalk and sywhang November 19, 2020 16:36
@sywhang
Copy link
Contributor

sywhang commented Nov 19, 2020

Thanks, this may need to be backported to 5.0 - I will handle that once this PR is merged.

noahfalk
noahfalk approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM

@noahfalk
Copy link
Member

noahfalk commented Nov 20, 2020

cc @davmason @josalem @sdmaclea - it looked fine to me but I didn't get to scrutinize it while trying to take care of everything before going on vacation. If we decide it needs a backport its probably worth someone else giving it a more careful look.

josalem
josalem approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@josalem josalem left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Walked through the lifetime of the strings, and this should fix the described issue.

@sywhang I think this is the only place where XplatEventLoggerConfiguration is used. Should we subsume the Parse logic directly into EventPipeProviderConfiguration to reduce duplication and ownership transfers?

@ViktorHofer
Copy link
Member

ViktorHofer commented Dec 8, 2020

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

@gbalykov
Fix memory errors related to EventPipe setup with COMPlus_EventPipeCo…
57e8f84 
…nfig

  - XplatEventLoggerConfiguration configuration owns strings passed to EventPipeProviderConfiguration pProviders, and configurations are freed before pProviders are copied to EventPipeSessionProvider
  - NewArrayHolder shoud be used for pProviders to fix memory leak (delete[] instead of delete)
@gbalykov gbalykov force-pushed the fix-etw-setup-memory-errors branch from e0adb01 to 57e8f84 Compare December 8, 2020 18:54
@gbalykov
Copy link
Member Author

gbalykov commented Dec 8, 2020

Rebased on new master with removed src/coreclr/src/ dir

@gbalykov
Copy link
Member Author

gbalykov commented Dec 17, 2020

@josalem Can this be merged?

@josalem
Copy link
Contributor

josalem commented Dec 17, 2020

I believe so. We recently added a C implementation of EventPipe we are intending on switching to, so we might need to port this change to there as well. That doesn't need to be part of this PR though.

CC @lateralusX

@josalem josalem merged commit 6b92612 into dotnet:master Dec 17, 2020
@gbalykov
Copy link
Member Author

gbalykov commented Dec 17, 2020

Thanks!

@lateralusX lateralusX mentioned this pull request Dec 18, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jan 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@noahfalk noahfalk noahfalk approved these changes

+2 more reviewers

@josalem josalem josalem approved these changes

@sywhang sywhang sywhang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Tracing-coreclr EventPipe

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@gbalykov @maryamariyan @alpencolt @sywhang @noahfalk @ViktorHofer @josalem @jkotas @Dotnet-GitSync-Bot