Make EventPipeProviderCallbackData own the filter data (#42307)

Fixes a use after free issue in EventPipe, where the EventPipeProviderCallbackData would cache a pointer to the filter data string on EventPipeSessionProvider, but disabling the session would free all of the EventPipeSessionProviders.

diff --git a/src/coreclr/src/vm/eventpipe.cpp b/src/coreclr/src/vm/eventpipe.cpp
index 4c5918f..debdcdd 100644 (file)
--- a/src/coreclr/src/vm/eventpipe.cpp
+++ b/src/coreclr/src/vm/eventpipe.cpp
@@ -1041,9 +1041,9 @@ HANDLE EventPipe::GetWaitHandle(EventPipeSessionID sessionID)
     return pSession ? pSession->GetWaitEvent()->GetHandleUNHOSTED() : 0;
 }
 
-void EventPipe::InvokeCallback(EventPipeProviderCallbackData eventPipeProviderCallbackData)
+void EventPipe::InvokeCallback(EventPipeProviderCallbackData *pEventPipeProviderCallbackData)
 {
-    EventPipeProvider::InvokeCallback(eventPipeProviderCallbackData);
+    EventPipeProvider::InvokeCallback(pEventPipeProviderCallbackData);
 }
 
 EventPipeEventInstance *EventPipe::BuildEventMetadataEvent(EventPipeEventInstance &instance, unsigned int metadataId)

diff --git a/src/coreclr/src/vm/eventpipe.h b/src/coreclr/src/vm/eventpipe.h
index 52e7061..ae56b9c 100644 (file)
--- a/src/coreclr/src/vm/eventpipe.h
+++ b/src/coreclr/src/vm/eventpipe.h
@@ -139,7 +139,7 @@ public:
         }
 
         while (eventPipeProviderCallbackDataQueue.TryDequeue(&eventPipeProviderCallbackData))
-            InvokeCallback(eventPipeProviderCallbackData);
+            InvokeCallback(&eventPipeProviderCallbackData);
     }
 
     // Returns the a number 0...N representing the processor number this thread is currently
@@ -158,7 +158,7 @@ public:
     }
 
 private:
-    static void InvokeCallback(EventPipeProviderCallbackData eventPipeProviderCallbackData);
+    static void InvokeCallback(EventPipeProviderCallbackData *pEventPipeProviderCallbackData);
 
     // Get the event used to write metadata to the event stream.
     static EventPipeEventInstance *BuildEventMetadataEvent(EventPipeEventInstance &instance, unsigned int metadataId);

diff --git a/src/coreclr/src/vm/eventpipecommontypes.cpp b/src/coreclr/src/vm/eventpipecommontypes.cpp
index e668dc1..f43b487 100644 (file)
--- a/src/coreclr/src/vm/eventpipecommontypes.cpp
+++ b/src/coreclr/src/vm/eventpipecommontypes.cpp
@@ -8,8 +8,7 @@
 
 void EventPipeProviderCallbackDataQueue::Enqueue(EventPipeProviderCallbackData *pEventPipeProviderCallbackData)
 {
-    SListElem<EventPipeProviderCallbackData> *listnode = new SListElem<EventPipeProviderCallbackData>(); // throws
-    listnode->m_Value = *pEventPipeProviderCallbackData;
+    SListElem<EventPipeProviderCallbackData> *listnode = new SListElem<EventPipeProviderCallbackData>(std::move(*pEventPipeProviderCallbackData)); // throws
     list.InsertTail(listnode);
 }
 
@@ -19,7 +18,7 @@ bool EventPipeProviderCallbackDataQueue::TryDequeue(EventPipeProviderCallbackDat
         return false;
 
     SListElem<EventPipeProviderCallbackData> *listnode = list.RemoveHead();
-    *pEventPipeProviderCallbackData = listnode->m_Value;
+    *pEventPipeProviderCallbackData = std::move(listnode->m_Value);
     delete listnode;
     return true;
 }

diff --git a/src/coreclr/src/vm/eventpipecommontypes.h b/src/coreclr/src/vm/eventpipecommontypes.h
index 1a66034..5bb4a18 100644 (file)
--- a/src/coreclr/src/vm/eventpipecommontypes.h
+++ b/src/coreclr/src/vm/eventpipecommontypes.h
@@ -98,20 +98,129 @@ typedef void (*EventPipeCallback)(
     EventFilterDescriptor *FilterData,
     void *CallbackContext);
 
-struct EventPipeProviderCallbackData
+class EventPipeProviderCallbackData
 {
-    LPCWSTR pFilterData;
-    EventPipeCallback pCallbackFunction;
-    bool enabled;
-    INT64 keywords;
-    EventPipeEventLevel providerLevel;
-    void* pCallbackData;
+public:
+    EventPipeProviderCallbackData():
+        m_pFilterData(nullptr),
+        m_pCallbackFunction(nullptr),
+        m_enabled(false),
+        m_keywords(0),
+        m_providerLevel(EventPipeEventLevel::LogAlways),
+        m_pCallbackData(nullptr),
+        m_pProvider(nullptr)
+    {
+
+    }
+
+    EventPipeProviderCallbackData(LPCWSTR pFilterData,
+                                  EventPipeCallback pCallbackFunction,
+                                  bool enabled,
+                                  INT64 keywords,
+                                  EventPipeEventLevel providerLevel,
+                                  void* pCallbackData,
+                                  EventPipeProvider *pProvider) :
+        m_pFilterData(nullptr),
+        m_pCallbackFunction(pCallbackFunction),
+        m_enabled(enabled),
+        m_keywords(keywords),
+        m_providerLevel(providerLevel),
+        m_pCallbackData(pCallbackData),
+        m_pProvider(pProvider)
+    {
+        if (pFilterData != nullptr)
+        {
+            // This is the only way to create an EventPipeProviderCallbackData that will copy the
+            // filter data. The copying is intentional, because sessions die before callbacks happen
+            // so we cannot cache a pointer to the session's filter data.
+            size_t bufSize = wcslen(pFilterData) + 1;
+            m_pFilterData = new WCHAR[bufSize];
+            wcscpy_s(m_pFilterData, bufSize, pFilterData);
+        }
+    }
+
+    EventPipeProviderCallbackData(EventPipeProviderCallbackData &&other)
+        : EventPipeProviderCallbackData()
+    {
+        *this = std::move(other);
+    }
+
+    EventPipeProviderCallbackData &operator=(EventPipeProviderCallbackData &&other)
+    {
+        std::swap(m_pFilterData, other.m_pFilterData);
+        m_pCallbackFunction = other.m_pCallbackFunction;
+        m_enabled = other.m_enabled;
+        m_keywords = other.m_keywords;
+        m_providerLevel = other.m_providerLevel;
+        m_pCallbackData = other.m_pCallbackData;
+        m_pProvider = other.m_pProvider;
+
+        return *this;
+    }
+
+    // We don't want to be unintentionally copying and deleting the filter data any more
+    // than we have to. Moving (above) is fine, but copying should be avoided.
+    EventPipeProviderCallbackData(const EventPipeProviderCallbackData &other) = delete;
+    EventPipeProviderCallbackData &operator=(const EventPipeProviderCallbackData &other) = delete;
+
+    ~EventPipeProviderCallbackData()
+    {
+        if (m_pFilterData != nullptr)
+        {
+            delete[] m_pFilterData;
+            m_pFilterData = nullptr;
+        }
+    }
+
+    LPCWSTR GetFilterData() const
+    {
+        return m_pFilterData;
+    }
+
+    EventPipeCallback GetCallbackFunction() const
+    {
+        return m_pCallbackFunction;
+    }
+
+    bool GetEnabled() const
+    {
+        return m_enabled;
+    }
+
+    INT64 GetKeywords() const
+    {
+        return m_keywords;
+    }
+
+    EventPipeEventLevel GetProviderLevel() const
+    {
+        return m_providerLevel;
+    }
+
+    void *GetCallbackData() const
+    {
+        return m_pCallbackData;
+    }
+
+    EventPipeProvider *GetProvider() const
+    {
+        return m_pProvider;
+    }
+
+private:
+    WCHAR *m_pFilterData;
+    EventPipeCallback m_pCallbackFunction;
+    bool m_enabled;
+    INT64 m_keywords;
+    EventPipeEventLevel m_providerLevel;
+    void* m_pCallbackData;
+    EventPipeProvider *m_pProvider;
 };
 
 class EventPipeProviderCallbackDataQueue
 {
 public:
-    void Enqueue(EventPipeProviderCallbackData* pEventPipeProviderCallbackData);
+    void Enqueue(EventPipeProviderCallbackData *pEventPipeProviderCallbackData);
     bool TryDequeue(EventPipeProviderCallbackData* pEventPipeProviderCallbackData);
 
 private:

diff --git a/src/coreclr/src/vm/eventpipeprovider.cpp b/src/coreclr/src/vm/eventpipeprovider.cpp
index 7aaa31f..916d7c5 100644 (file)
--- a/src/coreclr/src/vm/eventpipeprovider.cpp
+++ b/src/coreclr/src/vm/eventpipeprovider.cpp
@@ -194,7 +194,7 @@ void EventPipeProvider::AddEvent(EventPipeEvent &event)
     event.RefreshState();
 }
 
-/* static */ void EventPipeProvider::InvokeCallback(EventPipeProviderCallbackData eventPipeProviderCallbackData)
+/* static */ void EventPipeProvider::InvokeCallback(EventPipeProviderCallbackData *pEventPipeProviderCallbackData)
 {
     CONTRACTL
     {
@@ -205,12 +205,12 @@ void EventPipeProvider::AddEvent(EventPipeEvent &event)
     }
     CONTRACTL_END;
 
-    LPCWSTR pFilterData = eventPipeProviderCallbackData.pFilterData;
-    EventPipeCallback pCallbackFunction = eventPipeProviderCallbackData.pCallbackFunction;
-    bool enabled = eventPipeProviderCallbackData.enabled;
-    INT64 keywords = eventPipeProviderCallbackData.keywords;
-    EventPipeEventLevel providerLevel = eventPipeProviderCallbackData.providerLevel;
-    void *pCallbackData = eventPipeProviderCallbackData.pCallbackData;
+    LPCWSTR pFilterData = pEventPipeProviderCallbackData->GetFilterData();
+    EventPipeCallback pCallbackFunction = pEventPipeProviderCallbackData->GetCallbackFunction();
+    bool enabled = pEventPipeProviderCallbackData->GetEnabled();
+    INT64 keywords = pEventPipeProviderCallbackData->GetKeywords();
+    EventPipeEventLevel providerLevel = pEventPipeProviderCallbackData->GetProviderLevel();
+    void *pCallbackData = pEventPipeProviderCallbackData->GetCallbackData();
 
     bool isEventFilterDescriptorInitialized = false;
     EventFilterDescriptor eventFilterDescriptor{};
@@ -286,13 +286,13 @@ EventPipeProviderCallbackData EventPipeProvider::PrepareCallbackData(
     }
     CONTRACTL_END;
 
-    EventPipeProviderCallbackData result;
-    result.pFilterData = pFilterData;
-    result.pCallbackFunction = m_pCallbackFunction;
-    result.enabled = (m_sessions != 0);
-    result.providerLevel = providerLevel;
-    result.keywords = keywords;
-    result.pCallbackData = m_pCallbackData;
+    EventPipeProviderCallbackData result(pFilterData,
+                                         m_pCallbackFunction,
+                                         (m_sessions != 0),
+                                         keywords,
+                                         providerLevel,
+                                         m_pCallbackData,
+                                         this);
     return result;
 }
 

diff --git a/src/coreclr/src/vm/eventpipeprovider.h b/src/coreclr/src/vm/eventpipeprovider.h
index fab3bc7..864c53d 100644 (file)
--- a/src/coreclr/src/vm/eventpipeprovider.h
+++ b/src/coreclr/src/vm/eventpipeprovider.h
@@ -113,7 +113,7 @@ private:
         LPCWSTR pFilterData);
 
     // Invoke the provider callback.
-    static void InvokeCallback(EventPipeProviderCallbackData eventPipeProviderCallbackData);
+    static void InvokeCallback(EventPipeProviderCallbackData *pEventPipeProviderCallbackData);
 
     // Specifies whether or not the provider was deleted, but that deletion
     // was deferred until after tracing is stopped.