Skip to content

Commit

Permalink
EventPipeProvider destructor hardening (dotnet#13958)
Browse files Browse the repository at this point in the history 
* Take lock before manipulating list in desctructor

* Add NULL checks for provider list

* Remove asserts that duplicate conditional check
  • Loading branch information
@nategraf
nategraf authored Sep 14, 2017
1 parent 1f746be commit b5bcc13
Showing 1 changed file with 76 additions and 50 deletions.
126 changes: 76 additions & 50 deletions src/vm/eventpipeconfiguration.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ EventPipeConfiguration::~EventPipeConfiguration()

if(m_pProviderList != NULL)
{
// Take the lock before manipulating the provider list.
CrstHolder _crst(EventPipe::GetLock());

SListElem<EventPipeProvider*> *pElem = m_pProviderList->GetHead();
while(pElem != NULL)
{
Expand Down Expand Up @@ -103,8 +106,12 @@ bool EventPipeConfiguration::RegisterProvider(EventPipeProvider &provider)
return false;
}

// The provider has not been registered, so register it.
m_pProviderList->InsertTail(new SListElem<EventPipeProvider*>(&provider));
// The provider list should be non-NULL, but can be NULL on shutdown.
if (m_pProviderList != NULL)
{
// The provider has not been registered, so register it.
m_pProviderList->InsertTail(new SListElem<EventPipeProvider*>(&provider));
}

// Set the provider configuration and enable it if we know anything about the provider before it is registered.
if(m_pEnabledProviderList != NULL)
Expand Down Expand Up @@ -135,25 +142,29 @@ bool EventPipeConfiguration::UnregisterProvider(EventPipeProvider &provider)
// Take the lock before manipulating the provider list.
CrstHolder _crst(EventPipe::GetLock());

// Find the provider.
SListElem<EventPipeProvider*> *pElem = m_pProviderList->GetHead();
while(pElem != NULL)
// The provider list should be non-NULL, but can be NULL on shutdown.
if (m_pProviderList != NULL)
{
if(pElem->GetValue() == &provider)
// Find the provider.
SListElem<EventPipeProvider*> *pElem = m_pProviderList->GetHead();
while(pElem != NULL)
{
break;
}
if(pElem->GetValue() == &provider)
{
break;
}

pElem = m_pProviderList->GetNext(pElem);
}
pElem = m_pProviderList->GetNext(pElem);
}

// If we found the provider, remove it.
if(pElem != NULL)
{
if(m_pProviderList->FindAndRemove(pElem) != NULL)
// If we found the provider, remove it.
if(pElem != NULL)
{
delete(pElem);
return true;
if(m_pProviderList->FindAndRemove(pElem) != NULL)
{
delete(pElem);
return true;
}
}
}

Expand Down Expand Up @@ -188,16 +199,20 @@ EventPipeProvider* EventPipeConfiguration::GetProviderNoLock(const SString &prov
}
CONTRACTL_END;

SListElem<EventPipeProvider*> *pElem = m_pProviderList->GetHead();
while(pElem != NULL)
// The provider list should be non-NULL, but can be NULL on shutdown.
if (m_pProviderList != NULL)
{
EventPipeProvider *pProvider = pElem->GetValue();
if(pProvider->GetProviderName().Equals(providerName))
SListElem<EventPipeProvider*> *pElem = m_pProviderList->GetHead();
while(pElem != NULL)
{
return pProvider;
}
EventPipeProvider *pProvider = pElem->GetValue();
if(pProvider->GetProviderName().Equals(providerName))
{
return pProvider;
}

pElem = m_pProviderList->GetNext(pElem);
pElem = m_pProviderList->GetNext(pElem);
}
}

return NULL;
Expand Down Expand Up @@ -239,24 +254,27 @@ void EventPipeConfiguration::Enable(
m_pEnabledProviderList = new EventPipeEnabledProviderList(pProviders, static_cast<unsigned int>(numProviders));
m_enabled = true;

SListElem<EventPipeProvider*> *pElem = m_pProviderList->GetHead();
while(pElem != NULL)
// The provider list should be non-NULL, but can be NULL on shutdown.
if (m_pProviderList != NULL)
{
EventPipeProvider *pProvider = pElem->GetValue();

// Enable the provider if it has been configured.
EventPipeEnabledProvider *pEnabledProvider = m_pEnabledProviderList->GetEnabledProvider(pProvider);
if(pEnabledProvider != NULL)
SListElem<EventPipeProvider*> *pElem = m_pProviderList->GetHead();
while(pElem != NULL)
{
pProvider->SetConfiguration(
true /* providerEnabled */,
pEnabledProvider->GetKeywords(),
pEnabledProvider->GetLevel());
}
EventPipeProvider *pProvider = pElem->GetValue();

pElem = m_pProviderList->GetNext(pElem);
}
// Enable the provider if it has been configured.
EventPipeEnabledProvider *pEnabledProvider = m_pEnabledProviderList->GetEnabledProvider(pProvider);
if(pEnabledProvider != NULL)
{
pProvider->SetConfiguration(
true /* providerEnabled */,
pEnabledProvider->GetKeywords(),
pEnabledProvider->GetLevel());
}

pElem = m_pProviderList->GetNext(pElem);
}
}
}

void EventPipeConfiguration::Disable()
Expand All @@ -271,13 +289,17 @@ void EventPipeConfiguration::Disable()
}
CONTRACTL_END;

SListElem<EventPipeProvider*> *pElem = m_pProviderList->GetHead();
while(pElem != NULL)
// The provider list should be non-NULL, but can be NULL on shutdown.
if (m_pProviderList != NULL)
{
EventPipeProvider *pProvider = pElem->GetValue();
pProvider->SetConfiguration(false /* providerEnabled */, 0 /* keywords */, EventPipeEventLevel::Critical /* level */);
SListElem<EventPipeProvider*> *pElem = m_pProviderList->GetHead();
while(pElem != NULL)
{
EventPipeProvider *pProvider = pElem->GetValue();
pProvider->SetConfiguration(false /* providerEnabled */, 0 /* keywords */, EventPipeEventLevel::Critical /* level */);

pElem = m_pProviderList->GetNext(pElem);
pElem = m_pProviderList->GetNext(pElem);
}
}

m_enabled = false;
Expand Down Expand Up @@ -409,16 +431,20 @@ void EventPipeConfiguration::DeleteDeferredProviders()
}
CONTRACTL_END;

SListElem<EventPipeProvider*> *pElem = m_pProviderList->GetHead();
while(pElem != NULL)
// The provider list should be non-NULL, but can be NULL on shutdown.
if (m_pProviderList != NULL)
{
EventPipeProvider *pProvider = pElem->GetValue();
pElem = m_pProviderList->GetNext(pElem);
if(pProvider->GetDeleteDeferred())
SListElem<EventPipeProvider*> *pElem = m_pProviderList->GetHead();
while(pElem != NULL)
{
// The act of deleting the provider unregisters it,
// removes it from the list, and deletes the list element
delete(pProvider);
EventPipeProvider *pProvider = pElem->GetValue();
pElem = m_pProviderList->GetNext(pElem);
if(pProvider->GetDeleteDeferred())
{
// The act of deleting the provider unregisters it,
// removes it from the list, and deletes the list element
delete(pProvider);
}
}
}
}
Expand Down

0 comments on commit b5bcc13

Please sign in to comment.