[cmake-developers] Storage reallocation inside std::vector causing memory problem when using variable_watch

[Yves Frederix]

Hi all,

Some time ago I reported an issue with using variable_watch in 3.4.1
on the user list (reproduced by David Cole using master at that time),
see https://cmake.org/pipermail/cmake/2015-December/062213.html.

I finally found some time to get back to this and after some debugging
I did find the cause of the problem. In short, what happens is that an
internal memory reallocation inside an std::vector instance
invalidates the char* that contained the actual value of the watched
variable. It's not clear to me what would be the best fix though,
hence this mail (and the lack of an attached patch ;))

It seems that for 3.4.1 I was "lucky" in that I was able to create a
minimal test case to reproduce the problem. I cannot, however,
reproduce the problem using the same test case with, e.g., current
master, probably due to the unpredictable nature of std::vector
reallocations, but when checking the CMake sources it seems that the
problem itself is not fixed (but please, correct me if I'm wrong).

Steps to reproduce:
 - CMake 3.4.1
 - use the CMakeLists.txt from
https://cmake.org/pipermail/cmake/2015-December/062213.html
 - Configure with VS2012 or VS2015 generator. This will fail and show
strange symbols in the full path to
FindPackageHandleStandardArgs.cmake that is being printed to stdout.

The cause can be seen in cmMakefile:

const char* cmMakefile::GetDefinition(const std::string& name) const
{
  const char* def = this->StateSnapshot.GetDefinition(name);
  if(!def)
    {
    def = this->GetState()->GetInitializedCacheValue(name);
    }
#ifdef CMAKE_BUILD_WITH_CMAKE
  cmVariableWatch* vv = this->GetVariableWatch();
  if ( vv && !this->SuppressWatches )
    {
    if ( def )
      {
      vv->VariableAccessed(name, cmVariableWatch::VARIABLE_READ_ACCESS,
        def, this);
      }
    else
      {
      vv->VariableAccessed(name,
          cmVariableWatch::UNKNOWN_VARIABLE_READ_ACCESS, def, this);
      }
    }
#endif
  return def;
}


What happens is that 'def' is assigned at the beginning of the
function, but during the call to  vv->VariableAccessed(...), there is
an internal memory re-allocation inside one of the member variables
inside the cmDefinitions class of type std::vector. This operation
then invalidates the originally assigned 'def'.  If I understand
correctly, this reallocation is actually made possible in the first
place by the const_cast in cmVariableWatchCommandVariableAccessed() in
cmVariableWatchCommand.cxx. For this particular example (using VS2012
generator) the actual reallocation seems to be triggered by the call
to cmMakefile::FunctionPushPop functionScope(...) in
cmFunctionCommand::InvokeInitialPass.

There are some possible solutions I see to fix the problem:
1. Avoid the const_cast? I'm not even sure it is possible in the
current design...
2. Recompute 'def' after the call to VariableAccessed(). This seems
cumbersome, maybe does not result in the desired behavior?
3. Define a move constructor in the cmDefinitions class. Probably that
is not an option as IIRC CMake uses C++98. I did, however, check this
solution locally, compiling CMake using VS2012 and can verify that it
fixes the problem.

Unfortunately, none of the above options seems optimal.

If somebody with more knowledge about the CMake source would be
willing to have a closer look at this, I can provide with any
assistance that might be necessary. Also, I can provide a patch once
the good solution is defined.

Regards,
Yves