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.

1 files changed, 15 insertions, 8 deletions

diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx
index 950b247..600c985 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