commit 4c0edbd725976f947398f1513cef3f36ec05cc5f
author Justin Berger <j.david.berger@gmail.com> Sat Jul 22 17:23:11 2017 -0600
committer Brad King <brad.king@kitware.com> Thu Sep 21 11:50:57 2017 -0400
tree 804b60e16591750cc6d270a6206646df3f34c5a5
parent 28d2c6ef7e807c53a8bee76c86525f254fc4c54c

variable_watch: Made it safe to add/remove watches in access callbacks

Source/cmVariableWatch.cxx

diff --git a/Source/cmVariableWatch.cxx b/Source/cmVariableWatch.cxx
index 0e072b8..bd5d19c 100644
--- a/Source/cmVariableWatch.cxx
+++ b/Source/cmVariableWatch.cxx
@@ -2,11 +2,9 @@
    file Copyright.txt or https://cmake.org/licensing for details.  */
 #include "cmVariableWatch.h"
 
-#include "cmAlgorithms.h"
-
-#include "cm_auto_ptr.hxx"
-#include <algorithm>
+#include <memory>
 #include <utility>
+#include <vector>
 
 static const char* const cmVariableWatchAccessStrings[] = {
   "READ_ACCESS",     "UNKNOWN_READ_ACCESS", "UNKNOWN_DEFINED_ACCESS",
@@ -25,35 +23,27 @@
 {
 }
 
-template <typename C>
-void deleteAllSecond(typename C::value_type it)
-{
-  cmDeleteAll(it.second);
-}
-
 cmVariableWatch::~cmVariableWatch()
 {
-  std::for_each(this->WatchMap.begin(), this->WatchMap.end(),
-                deleteAllSecond<cmVariableWatch::StringToVectorOfPairs>);
 }
 
 bool cmVariableWatch::AddWatch(const std::string& variable, WatchMethod method,
                                void* client_data /*=0*/,
                                DeleteData delete_data /*=0*/)
 {
-  CM_AUTO_PTR<cmVariableWatch::Pair> p(new cmVariableWatch::Pair);
+  auto p = std::make_shared<cmVariableWatch::Pair>();
   p->Method = method;
   p->ClientData = client_data;
   p->DeleteDataCall = delete_data;
   cmVariableWatch::VectorOfPairs& vp = this->WatchMap[variable];
-  for (cmVariableWatch::Pair* pair : vp) {
+  for (auto& pair : vp) {
     if (pair->Method == method && client_data &&
         client_data == pair->ClientData) {
       // Callback already exists
       return false;
     }
   }
-  vp.push_back(p.release());
+  vp.push_back(std::move(p));
   return true;
 }
 
@@ -70,7 +60,6 @@
         // If client_data is NULL, we want to disconnect all watches against
         // the given method; otherwise match ClientData as well.
         (!client_data || (client_data == (*it)->ClientData))) {
-      delete *it;
       vp->erase(it);
       return;
     }
@@ -84,9 +73,17 @@
   cmVariableWatch::StringToVectorOfPairs::const_iterator mit =
     this->WatchMap.find(variable);
   if (mit != this->WatchMap.end()) {
-    const cmVariableWatch::VectorOfPairs* vp = &mit->second;
-    for (cmVariableWatch::Pair* it : *vp) {
-      it->Method(variable, access_type, it->ClientData, newValue, mf);
+    // The strategy here is to copy the list of callbacks, and ignore
+    // new callbacks that existing ones may add.
+    std::vector<std::weak_ptr<Pair>> vp(mit->second.begin(),
+                                        mit->second.end());
+    for (auto& weak_it : vp) {
+      // In the case where a callback was removed, the weak_ptr will not be
+      // lockable, and so this ensures we don't attempt to call into freed
+      // memory
+      if (auto it = weak_it.lock()) {
+        it->Method(variable, access_type, it->ClientData, newValue, mf);
+      }
     }
     return true;
   }

Source/cmVariableWatch.h

diff --git a/Source/cmVariableWatch.h b/Source/cmVariableWatch.h
index 05a0a56..27d1b12 100644
--- a/Source/cmVariableWatch.h
+++ b/Source/cmVariableWatch.h
@@ -6,6 +6,7 @@
 #include "cmConfigure.h" // IWYU pragma: keep
 
 #include <map>
+#include <memory> // IWYU pragma: keep
 #include <string>
 #include <vector>
 
@@ -79,7 +80,7 @@
     }
   };
 
-  typedef std::vector<Pair*> VectorOfPairs;
+  typedef std::vector<std::shared_ptr<Pair>> VectorOfPairs;
   typedef std::map<std::string, VectorOfPairs> StringToVectorOfPairs;
 
   StringToVectorOfPairs WatchMap;