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

[timurrrr at google.com]

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#newcode45
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#newcode47
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/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) {
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/interception_win.cc#newcode52
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/interception_win.cc#newcode62
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/interception_win.cc#newcode66
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/interception_win.cc#newcode85
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/interception_win.cc#newcode114
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/interception_win.cc#newcode120
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/interception_win.cc#newcode124
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/interception_win.cc#newcode134
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.

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