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

[Timur Iskhodzhanov]

Hi Aaron,

On Wed, Feb 29, 2012 at 5:45 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 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.
You mean from some other thread?
I assume there's only one thread at the time this ReplaceSystemMalloc
is called [asan_init should be called before that moment].
Anyways, if something could execute "int 3s" while I'm writing jmp
then we have a dangerous data race and it's not safe to use not-atomic
memcpy either.

> Then you're always doing an absolute jump, destroying whatever was in
> the real malloc.
Yes, this was the intention.

> I believe malloc is hot-patchable already, so why not use the usual mechanisms?
I'm not sure I'm aware of hot-patching malloc.
Can you please elaborate?

> Is it acceptable to irretrievably destroy malloc for the session?
Ideally, we want "original" malloc/free to not be available at all
while running under ASan.

>> +
>> +    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

Thanks!

--
Timur