[llvm-commits] [ASan] Add back the support for /MT; intercept statically-linked functions (issue 5783070)

[glider at google.com]

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#newcode45
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#newcode47
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/interception_win.cc
File lib/asan/interception/interception_win.cc (right):

http://codereview.appspot.com/5783070/diff/1/lib/asan/interception/interception_win.cc#newcode35
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/interception_win.cc#newcode52
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/interception_win.cc#newcode62
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/interception_win.cc#newcode66
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/interception_win.cc#newcode85
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/interception_win.cc#newcode114
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/interception_win.cc#newcode120
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/interception_win.cc#newcode124
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/interception_win.cc#newcode134
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.

http://codereview.appspot.com/5783070/