Avoid occasional use-after-free when a variable watch is executed

Re-lookup a variable value when an associated VariableWatch is executed
in cmMakefile::GetDefinition.

This fixes a problem with 'def' sometimes becoming invalid due to memory
reallocation inside an std::vector. In this case, the problem was that
if the call to VariableAccessed actually executed a callback function,
the internal state of the makefile has changed due to the associated
function scope being pushed. This in turn implies that a new
cmDefinitions instance was pushed in cmMakefile::VarTree. As
cmLinkedTree is based on an std::vector, this push can have triggered
reallocation of its internal memory buffer. However, as the value of
'def', which was computed on method entry, actually points to a property
of one of the cmDefinitions instances in cmMakefile::VarTree,
reallocation can invalidate the value of 'def' so that it cannot simply
be returned at the end of the function. The solution implemented here is
to simply lookup the value of 'def' again.

3 files changed, 19 insertions, 10 deletions

diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx
index 950b247356..600c985f05 100644
--- a/Source/cmMakefile.cxx
+++ b/Source/cmMakefile.cxx
@@ -2531,15 +2531,22 @@ const char* cmMakefile::GetDefinition(const std::string& name) const
   cmVariableWatch* vv = this->GetVariableWatch();
   if ( vv && !this->SuppressWatches )
     {
-    if ( def )
-      {
-      vv->VariableAccessed(name, cmVariableWatch::VARIABLE_READ_ACCESS,
-        def, this);
-      }
-    else
-      {
+    bool const watch_function_executed =
       vv->VariableAccessed(name,
-          cmVariableWatch::UNKNOWN_VARIABLE_READ_ACCESS, def, this);
+                           def ? cmVariableWatch::VARIABLE_READ_ACCESS
+                           : cmVariableWatch::UNKNOWN_VARIABLE_READ_ACCESS,
+                           def, this);
+
+    if (watch_function_executed)
+      {
+      // A callback was executed and may have caused re-allocation of the
+      // variable storage.  Look it up again for now.
+      // FIXME: Refactor variable storage to avoid this problem.
+      def = this->StateSnapshot.GetDefinition(name);
+      if(!def)
+        {
+        def = this->GetState()->GetInitializedCacheValue(name);
+        }
       }
     }
 #endif
diff --git a/Source/cmVariableWatch.cxx b/Source/cmVariableWatch.cxx
index 57dde31395..a200718595 100644
--- a/Source/cmVariableWatch.cxx
+++ b/Source/cmVariableWatch.cxx
@@ -96,7 +96,7 @@ void cmVariableWatch::RemoveWatch(const std::string& variable,
     }
 }
 
-void  cmVariableWatch::VariableAccessed(const std::string& variable,
+bool  cmVariableWatch::VariableAccessed(const std::string& variable,
                                         int access_type,
                                         const char* newValue,
                                         const cmMakefile* mf) const
@@ -112,5 +112,7 @@ void  cmVariableWatch::VariableAccessed(const std::string& variable,
       (*it)->Method(variable, access_type, (*it)->ClientData,
         newValue, mf);
       }
+    return true;
     }
+  return false;
 }
diff --git a/Source/cmVariableWatch.h b/Source/cmVariableWatch.h
index 0ca4a5580e..2f082afe22 100644
--- a/Source/cmVariableWatch.h
+++ b/Source/cmVariableWatch.h
@@ -42,7 +42,7 @@ public:
   /**
    * This method is called when variable is accessed
    */
-  void VariableAccessed(const std::string& variable, int access_type,
+  bool VariableAccessed(const std::string& variable, int access_type,
     const char* newValue, const cmMakefile* mf) const;
 
   /**