[llvm-commits] [ASan] Replace CRT .dll malloc with our implementation at asan_init() time (issue 5708059)

[Aaron Ballman]

On Wed, Feb 29, 2012 at 5:31 AM,  <timurrrr at google.com> wrote:
> Reviewers: glider,
>
> Message:
> Hi Alexander,
>
> Can you please review this small CL?
>
> Thanks,
> Timur
>
> Description:
> Replace CRT .dll malloc with our implementation
>
> Please review this at http://codereview.appspot.com/5708059/
>
> Affected files:
>   M lib/asan/asan_malloc_win.cc
>
>
> Index: lib/asan/asan_malloc_win.cc
> diff --git a/lib/asan/asan_malloc_win.cc b/lib/asan/asan_malloc_win.cc
> index
> 40429f481e284221972754996284040f8ce9f7a3..42c54dc5d1818aeea6bddccf22df324a9a560a44
> 100644
> --- a/lib/asan/asan_malloc_win.cc
> +++ b/lib/asan/asan_malloc_win.cc
> @@ -18,12 +18,7 @@
>  #include "asan_internal.h"
>  #include "asan_stack.h"
>
> -namespace __asan {
> -void ReplaceSystemMalloc() {
> -  // FIXME: investigate whether any action is needed.
> -  // Currenlty, we fail to intercept malloc() called from intercepted
> _strdup().
> -}
> -}  // namespace __asan
> +#include "interception/interception.h"
>
>  // ---------------------- Replacement functions ---------------- {{{1
>  using namespace __asan;  // NOLINT
> @@ -55,4 +50,37 @@ void *realloc(void *ptr, size_t size) {
>  }
>  }  // extern "C"
>
> +using __interception::GetRealFunctionAddress;
> +
> +// We don't want to include "windows.h" in this file to avoid extra
> attributes
> +// set on malloc/free etc (e.g. dllimport), so declare a few things
> manually:
> +extern "C" int __stdcall VirtualProtect(void* addr, size_t size,
> +                                        DWORD prot, DWORD *old_prot);
> +const int PAGE_EXECUTE_READWRITE = 0x40;
> +
> +namespace __asan {
> +void ReplaceSystemMalloc() {
> +#ifdef _WIN64
> +# error ReplaceSystemMalloc was not tested on x64
> +#endif
> +  char *crt_malloc;
> +  if (GetRealFunctionAddress("malloc", (void**)&crt_malloc)) {
> +    // Replace malloc in the CRT dll with a jump to our malloc.
> +    DWORD old_prot, unused;
> +    CHECK(VirtualProtect(crt_malloc, 16, PAGE_EXECUTE_READWRITE,
> &old_prot));
> +    REAL(memset)(crt_malloc, 0xCC /* int 3 */, 16);  // just in case.
> +
> +    uintptr_t jmp_offset = (uintptr_t)malloc - (uintptr_t)crt_malloc - 5;
> +    crt_malloc[0] = 0xE9;  // jmp, should be followed by an offset.
> +    REAL(memcpy)(crt_malloc + 1, &jmp_offset, sizeof(jmp_offset));

Doing the memset to int 3s is not safe.  You've not written your
relative jump yet, so technically, something could be executing the
code you're turning into int 3s.

Then you're always doing an absolute jump, destroying whatever was in
the real malloc.  I believe malloc is hot-patchable already, so why
not use the usual mechanisms?  Is it acceptable to irretrievably
destroy malloc for the session?

> +
> +    CHECK(VirtualProtect(crt_malloc, 16, old_prot, &unused));
> +
> +    // FYI: FlushInstructionCache is needed on Itanium etc but not on
> x86/x64.
> +  }
> +
> +  // FIXME: investigate whether anything else is needed.

There's not, at least in terms of applying hot patches.

~Aaron