[ASan] Replace CRT .dll malloc with our implementation at asan_init() time

Replace CRT .dll malloc with our implementation

Committed: http://llvm.org/viewvc/llvm-project?view=rev&revision=151715

[timurrrr_at_google_com, 2012-02-29 11:31:04]

Hi Alexander,

Can you please review this small CL?

Thanks,
Timur

[glider, 2012-02-29 11:43:42]

LGTM if the lint passes.

http://codereview.appspot.com/5708059/diff/1/lib/asan/asan_malloc_win.cc
File lib/asan/asan_malloc_win.cc (right):

http://codereview.appspot.com/5708059/diff/1/lib/asan/asan_malloc_win.cc#newc...
lib/asan/asan_malloc_win.cc:73: uintptr_t jmp_offset = (uintptr_t)malloc -
(uintptr_t)crt_malloc - 5;
This should be signed.

[timurrrr_at_google_com, 2012-02-29 11:48:02]

Thanks, r151715!

http://codereview.appspot.com/5708059/diff/1/lib/asan/asan_malloc_win.cc
File lib/asan/asan_malloc_win.cc (right):

http://codereview.appspot.com/5708059/diff/1/lib/asan/asan_malloc_win.cc#newc...
lib/asan/asan_malloc_win.cc:73: uintptr_t jmp_offset = (uintptr_t)malloc -
(uintptr_t)crt_malloc - 5;
On 2012/02/29 11:43:43, glider wrote:
> This should be signed.

Done.

[aaron_aaronballman.com, 2012-02-29 13:45:57]

On Wed, Feb 29, 2012 at 5:31 AM,  <timurrrr@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

[timurrrr_at_google_com, 2012-02-29 13:53:17]

Hi Aaron,

On Wed, Feb 29, 2012 at 5:45 PM, Aaron Ballman <aaron@aaronballman.com> wrote:
> On Wed, Feb 29, 2012 at 5:31 AM,  <timurrrr@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

[aaron_aaronballman.com, 2012-02-29 14:05:18]

On Wed, Feb 29, 2012 at 7:52 AM, Timur Iskhodzhanov <timurrrr@google.com> wrote:
> Hi Aaron,
>
> On Wed, Feb 29, 2012 at 5:45 PM, Aaron Ballman <aaron@aaronballman.com> wrote:
>> On Wed, Feb 29, 2012 at 5:31 AM,  <timurrrr@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].

Correct

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

Correct!  That's why hot patching was designed the way it was -- no data races.

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

Microsoft has hot patching functionality built in to a lot of their
APIs.  Basically, you'll see mov edi, edi as the first instruction in
a call, and the call will be preceded by 5 bytes of NOP instructions.
The basic idea is to write your absolute jump over the NOPs, then
write the single byte relative jmp over the mov edi, edi.  Et voila,
you've patched the function atomically.  And you can still call the
original by skipping over the relative jmp.

http://blogs.msdn.com/b/oldnewthing/archive/2011/09/21/10214405.aspx

I have code to do this, if interested/required.

~Aaron

[timurrrr_at_google_com, 2012-02-29 14:18:01]

On Wed, Feb 29, 2012 at 6:05 PM, Aaron Ballman <aaron@aaronballman.com> wrote:
> On Wed, Feb 29, 2012 at 7:52 AM, Timur Iskhodzhanov <timurrrr@google.com>
wrote:
>> Hi Aaron,
>>
>> On Wed, Feb 29, 2012 at 5:45 PM, Aaron Ballman <aaron@aaronballman.com>
wrote:
>>> On Wed, Feb 29, 2012 at 5:31 AM,  <timurrrr@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].
>
> Correct
>
>> 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.
>
> Correct!  That's why hot patching was designed the way it was -- no data
races.
>
>>> 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?
>
> Microsoft has hot patching functionality built in to a lot of their
> APIs.  Basically, you'll see mov edi, edi as the first instruction in
> a call, and the call will be preceded by 5 bytes of NOP instructions.
> The basic idea is to write your absolute jump over the NOPs, then
> write the single byte relative jmp over the mov edi, edi.  Et voila,
> you've patched the function atomically.  And you can still call the
> original by skipping over the relative jmp.
>
> http://blogs.msdn.com/b/oldnewthing/archive/2011/09/21/10214405.aspx
>
> I have code to do this, if interested/required.
I've read the article and loved it :)
It looks like the magic is not needed for ASan at this point [just one
thread at the asan_init time]
but anyways thanks a lot for making me know a bit more on patching functions :)

> ~Aaron