possible bug in src/windows/patch_functions.cc

[GoogleCodeExporter]

LibcInfo::PopulateWindowsFn() in src/windows/patch_functions.cc
has this code to deal with multiple modules using the same symbol:

  // There's always a chance that our module uses the same function
  // as another module that we've already loaded.  In that case, we
  // need to set our windows_fn to NULL, to avoid double-patching.
  for (int ifn = 0; ifn < kNumFunctions; ifn++) {
    for (int imod = 0;
         imod < sizeof(g_module_libcs)/sizeof(*g_module_libcs);  imod++) {
      if (g_module_libcs[imod]->is_valid() &&
          this->windows_fn(ifn) == g_module_libcs[imod]->windows_fn(ifn)) {
        windows_fn_[ifn] = NULL;
      }
    }
  }

I am not sure this works.  An earlier line sets windows_fn_:

      windows_fn_[i] = PreamblePatcher::ResolveTarget(fn);

The problem is that ResolveTarget() chases JMP instructions.  If the
function appeared in an earlier module, it has been patched, and its
first instruction is now a jump to the corresponding
LibcInfoWithPatchFunctions<T>::Perftools_XXX().  So windows_fn_[i]
in the current module gets set to the address of that, but in the
loop you are comparing it with the address of the original Windows
function.

This seems to be the cause of a crash in a program linked with with
libtcmalloc_minimal.  PopulateWindowsFn() gets called thrice during
initialization: for MSVCRT, then MSVCR100, and finally the main
executable.  In the ModuleEntryCopy argument for the last call, the
addresses of malloc(), free(), realloc(), calloc(), _msize(), and
_expand() are the same as for MSVCR100.  The last entry in the
address array, for k_CallocCrt, is calloc() again.  But the eight
new/delete function addresses are different and seem to be in the
executable rather than the DLL.

For the functions that are repeated,
PreamblePatcher::RawPatchWithStub() patches
LibcInfoWithPatchFunctions<2>::Perftools_XXX().  It aborts because
Perftools__expand() is too short to patch.

This is a 32-bit executable running on 64-bit Windows 7.  The
compiler is VS 2010 SP1.  Only the 32-bit debug build of the program
fails.  The 32-bit release build works, as do the 64-bit debug and
release builds.  All builds are statically linked with the release
build of libtcmalloc_minimal, which is compiled with /MD.  I am
using gperftools 2.2.1, but even in 2.3.90 the code in src/windows/
is not significantly different.

Original issue reported on code.google.com by manojkum...@gmail.com on 7 Jan 2015 at 3:38

[GoogleCodeExporter]

Your explanation looks plausible. Would be great if you could prepare a patch 
with fix.

E.g. we can detect if jmp leads to one of perftools functions and don't follow 
it.

Original comment by alkondratenko on 10 Jan 2015 at 8:14

[GoogleCodeExporter]

A proper fix will take some effort (and somebody who knows more
about Windows than I do: I am new to it).  It is easy to avoid
patching a function that has already been patched, but the problem
is that the patching code assumes that if any function in a module
is patched, then the original free() and realloc() for that module
can be called.  Hence these lines in PopulateWindowsFn():

    CHECK(windows_fn_[kFree]);
    CHECK(windows_fn_[kRealloc]);

In my program, malloc/free/calloc/realloc in the main executable
resolve to functions in MSVCR100, but the new/delete functions are
different.  If I simply NULL out the windows_fn_ entry for malloc()
and free() because they are already patched, then
LibcInfoWithPatchFunctions<T>::Perftools_delete() will fail because
it needs to be able to call the original free().

In my program, where all files are compiled with /MD, it turns out
that the addresses for new, delete etc. in static_fn_ for the main
executable are not actually the functions called for new, delete,
etc.  I have no idea what these functions do, but they do not seem
to be called.  Each is a jump to a short instruction sequence ending
with OUTS (a privileged instruction).  My workaround for the bug was
to avoid patching the main executable, but that is not a general
solution.

BTW, the code works accidentally in some builds because the Visual
Studio linker merges identical functions.  Some patch functions,
e.g. LibcInfoWithPatchFunctions<T>::Perftools_malloc(), do not
depend on the template parameter, so the linker merges them all.
For those functions the perftools_fn_ entry for all instantiations
of LibcInfoWithPatchFunctions end up pointing to the same function.

LibcInfoWithPatchFunctions<T>::Patch() has this test for whether a
function has already been patched:

    if (windows_fn_[i] && windows_fn_[i] != perftools_fn_[i]) {

Logically this tests whether the function has been patched in the
current module but, because of the linker optimization, for
functions like malloc() it accidentally checks whether the function
has been patched in any module.

Original comment by manojkum...@gmail.com on 14 Jan 2015 at 7:08

[GoogleCodeExporter]

You can certainly quite safely assume that nobody will fix this before you can. 
So you have time for "some effort".

Right now project lacks anybody with decent attention to windows issues.

Original comment by alkondratenko on 18 Jan 2015 at 3:47