Fix SIGSEGV in EventPipe on Shutdown #14123
Conversation
…guration object has been destroyed.
src/vm/eventpipeconfiguration.cpp
Outdated
| } | ||
| CONTRACTL_END; | ||
|
|
||
| if(pProvider == NULL) |
There was a problem hiding this comment.
A nit - missing space between the "if" and the "(".
src/vm/eventpipe.cpp
Outdated
| // Delete the provider now. | ||
| // NOTE: This will remove it from all of the EventPipe data structures. | ||
| delete(pProvider); | ||
| if(s_pConfig != NULL) |
There was a problem hiding this comment.
A nit - missing space between the "if" and the "(".
src/vm/eventpipe.cpp
Outdated
|
|
||
| return new EventPipeProvider(providerName, pCallbackFunction, pCallbackData); | ||
| EventPipeProvider *pProvider = NULL; | ||
| if(s_pConfig != NULL) |
There was a problem hiding this comment.
A nit - missing space between the "if" and the "(".
|
Thanks @janvorli. Fixed. |
There was a problem hiding this comment.
Should EventPipeConfiguration::DeleteDeferredProviders be modified as well?
| void Initialize(); | ||
|
|
||
| // Create a new provider. | ||
| EventPipeProvider* CreateProvider(const SString &providerName, EventPipeCallback pCallbackFunction, void *pCallbackData); |
There was a problem hiding this comment.
Maybe change this signature to use default arguments of NULL for the callback functions to match the style of eventpipeprovider.h:67
There was a problem hiding this comment.
This was actually intentional - the reason is that for the most part, no one should call this version of CreateProvider because they should always go through EventPipe::CreateProvider.
* Fix a crash that occurs when a provider is registered after the configuration object has been destroyed. * Code review feedback.
* Fix a crash that occurs when a provider is registered after the configuration object has been destroyed. * Code review feedback.
|
Any chance this can be backported to the 2.0 runtime? |
|
@joshgarnett yes, this will be ported to 2.0. You can watch #15444 for merge. Once the release branch is open, I will merge it. |
|
Actually, I take it back, this is a different issue. Can you tell me more about how you're seeing this and how often it occurs? |
|
Hey @brianrob, I was seeing it consistently on an app I just upgraded from mono to dotnet. I think I narrowed the cause down to NLog. After adding an explicit call to LogManager.Shutdown() at the end of the main method, the problem went away. Without that change I confirmed it also didn't seg fault with the latest daily build. (edit) The crash was in EventPipeConfiguration::RegisterProvider |
|
@brianrob @joshgarnett I believe the backport for this is being tracked in #15765. I've been eagerly waiting for it to make it into a servicing release :). As far as I can tell, the merge has been approved and simply has to be merged + released at this point. |
|
Ok, thanks @joshgarnett. @nategraf is #15765 approved or does this still need to go through shiproom? #15444 is another related AV and it is approved and just needs to be merged (soon). |
|
It still needs to go through shiproom. |
* Fix SIGSEGV in EventPipe on Shutdown (#14123) * Fix a crash that occurs when a provider is registered after the configuration object has been destroyed. * Code review feedback. * Use DeleteProvider to avoid phantom registers providers
This change does two things:
Fixes #13689.