[ASan] Add back the support for /MT; intercept statically-linked functions

[ASan] Add back the support for /MT; intercept statically-linked functions

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

[timurrrr_at_google_com, 2012-03-11 16:43:20]

Hello Alexander,

Can you please review this patch?

Thanks,
Timur

[glider, 2012-03-11 17:14:06]

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

http://codereview.appspot.com/5783070/diff/1/lib/asan/asan_interceptors.cc#ne...
lib/asan/asan_interceptors.cc:45: char* __cdecl strchr(const char *s, char c);
We definitely don't want __cdecl on Mac.
Also, can't this be put into some *_win.h?

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

http://codereview.appspot.com/5783070/diff/1/lib/asan/asan_malloc_win.cc#newc...
lib/asan/asan_malloc_win.cc:47: void *_calloc_impl(size_t nmemb, size_t size,
int *errno_tmp) {
Please add a comment about why you need to declare _calloc_impl

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
File lib/asan/interception/interception_win.cc (right):

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:35: static void _memset(unsigned char
*p, int value, size_t sz) {
It's better to use internal_memset and internal_memcpy here (please define them
if they aren't defined yet)

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:52: * from the wrapper. These pos+5
bytes of instructions are called "island".
I'd better move this comment before the function, but this is up to you.
Please do not use /* */, because it's inconsistent with the comment at the top
of the file and also makes it less convenient to grep for comments.

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:62: unsigned char *island = (unsigned
char*)VirtualAlloc(NULL, ISLAND_SIZE,
Don't you have any ASan-style function to use instead of VirtualAlloc?
(e.g. AsanMmapOrDie...)

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:66: return false;
You may want to consider deallocating the branch island, although I don't
insist.

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:85: } else if (instr_2b == 0xFF8B) {
It's probably more elegant to have three switch statements:

switch (instr_1b) {
  case 0x55:
  case 0x56:
    pos += 1;
    break;
}
switch (instr_2b) {
  case 0xff8b:
  ...
}

, but this may be of the same length (still worth trying)

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:114: if (pos > ISLAND_SIZE)
This should be a CHECK -- it's unlikely that pos is greater than ISLAND_SIZE
upon the loop exit.

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:120: uintptr_t jmp_offset =
(intptr_t)old_func - (intptr_t)island - 5;
Be careful with |old_func| and |island| having the rightmost bit set.
Those should probably be uintptr_t.

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:124: DWORD unused;
Let it be unused_oldprotect or something like that.
You can even reuse old_prot from below.

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:134: old_bytes[0] = 0xE9;  // jmp,
should be followed by an offset.
I see a repeating pattern in this code.

[timurrrr_at_google_com, 2012-03-12 10:06:31]

Thanks, PTAL!

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

http://codereview.appspot.com/5783070/diff/1/lib/asan/asan_interceptors.cc#ne...
lib/asan/asan_interceptors.cc:45: char* __cdecl strchr(const char *s, char c);
On 2012/03/11 17:14:06, glider wrote:
> We definitely don't want __cdecl on Mac.
> Also, can't this be put into some *_win.h?
Good catch, it's not actually needed on Win too!

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

http://codereview.appspot.com/5783070/diff/1/lib/asan/asan_malloc_win.cc#newc...
lib/asan/asan_malloc_win.cc:47: void *_calloc_impl(size_t nmemb, size_t size,
int *errno_tmp) {
On 2012/03/11 17:14:06, glider wrote:
> Please add a comment about why you need to declare _calloc_impl
[per offline chat] the comment is not needed

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
File lib/asan/interception/interception_win.cc (right):

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:35: static void _memset(unsigned char
*p, int value, size_t sz) {
On 2012/03/11 17:14:06, glider wrote:
> It's better to use internal_memset and internal_memcpy here (please define
them
> if they aren't defined yet)
(per offline chat)
they are defined in ASan sources whilst we want to keep the interception/
library work with any tool.
Added a FIXME to move the functions into interception/

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:52: * from the wrapper. These pos+5
bytes of instructions are called "island".
On 2012/03/11 17:14:06, glider wrote:
> I'd better move this comment before the function, but this is up to you.
[per offline discussion]
this is "how it works", not "what it does". "What it does" should be in the .h
file and it is there.
> Please do not use /* */, because it's inconsistent with the comment at the top
> of the file and also makes it less convenient to grep for comments.

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:62: unsigned char *island = (unsigned
char*)VirtualAlloc(NULL, ISLAND_SIZE,
On 2012/03/11 17:14:06, glider wrote:
> Don't you have any ASan-style function to use instead of VirtualAlloc?
> (e.g. AsanMmapOrDie...)
No - at least while we're on Win-only with this code.
We can't depend on anything from ASan in the interception/ library.

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:66: return false;
On 2012/03/11 17:14:06, glider wrote:
> You may want to consider deallocating the branch island, although I don't
> insist.
I've created a common pool instead.

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:85: } else if (instr_2b == 0xFF8B) {
On 2012/03/11 17:14:06, glider wrote:
> It's probably more elegant to have three switch statements:
> 
> switch (instr_1b) {
>   case 0x55:
>   case 0x56:
>     pos += 1;
>     break;
> }
> switch (instr_2b) {
>   case 0xff8b:
>   ...
> }
> 
> , but this may be of the same length (still worth trying)

Done.

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:114: if (pos > ISLAND_SIZE)
On 2012/03/11 17:14:06, glider wrote:
> This should be a CHECK -- it's unlikely that pos is greater than ISLAND_SIZE
> upon the loop exit.
Ditto, we don't have CHECK in the interception/ library :(

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:120: uintptr_t jmp_offset =
(intptr_t)old_func - (intptr_t)island - 5;
On 2012/03/11 17:14:06, glider wrote:
> Be careful with |old_func| and |island| having the rightmost bit set.
> Those should probably be uintptr_t.

Done.

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:124: DWORD unused;
On 2012/03/11 17:14:06, glider wrote:
> Let it be unused_oldprotect or something like that.
done

> You can even reuse old_prot from below.
that's not very true.

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/intercepti...
lib/asan/interception/interception_win.cc:134: old_bytes[0] = 0xE9;  // jmp,
should be followed by an offset.
On 2012/03/11 17:14:06, glider wrote:
> I see a repeating pattern in this code.

Done.

[glider, 2012-03-12 10:32:55]

http://codereview.appspot.com/5783070/diff/4002/lib/asan/interception/interce...
File lib/asan/interception/interception_win.cc (right):

http://codereview.appspot.com/5783070/diff/4002/lib/asan/interception/interce...
lib/asan/interception/interception_win.cc:138: pool_used += head + 5;
I think it's better to align the jump targets on 32 bytes, but this is up to
you.

[glider, 2012-03-12 10:39:29]

LGTM

[timurrrr_at_google_com, 2012-03-12 11:50:08]

On 2012/03/12 10:39:29, glider wrote:
> LGTM

Thanks, r152557!