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

Issue 5783070: [ASan] Add back the support for /MT; intercept statically-linked functions (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
12 years, 9 months ago by timurrrr_at_google_com

Modified:
12 years, 9 months ago

Reviewers:
glider

CC:
llvm-commits_cs.uiuc.edu

Visibility:
Public.

More Reviews

Description

[ASan] Add back the support for /MT; intercept statically-linked functions Committed as http://llvm.org/viewvc/llvm-project?view=rev&revision=152557

Patch Set 2 : A few improvements #

Total comments: 1

Patch Set 4 : a few more improvements #

Patch Set 6 : fixed lint warnings #

Created: 12 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+170 lines, -11 lines)			Patch
M	lib/asan/asan_interceptors.cc	View	1 2 3 4 5	1 chunk	+25 lines, -0 lines	0 comments	Download
M	lib/asan/asan_malloc_win.cc	View	1	4 chunks	+11 lines, -4 lines	0 comments	Download
M	lib/asan/asan_win.cc	View	1 2 3 4 5	1 chunk	+2 lines, -2 lines	0 comments	Download
M	lib/asan/interception/interception.h	View		1 chunk	+9 lines, -4 lines	0 comments	Download
M	lib/asan/interception/interception_win.h	View	1	1 chunk	+10 lines, -1 line	0 comments	Download
M	lib/asan/interception/interception_win.cc	View	1 2 3 4 5	1 chunk	+113 lines, -0 lines	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

timurrrr_at_google_com

Hello Alexander, Can you please review this patch? Thanks, Timur

12 years, 9 months ago (2012-03-11 16:43:20 UTC) #1

glider

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

12 years, 9 months ago (2012-03-11 17:14:06 UTC) #2

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

12 years, 9 months ago (2012-03-12 10:06:31 UTC) #3

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

http://codereview.appspot.com/5783070/diff/4002/lib/asan/interception/interception_win.cc File lib/asan/interception/interception_win.cc (right): http://codereview.appspot.com/5783070/diff/4002/lib/asan/interception/interception_win.cc#newcode138 lib/asan/interception/interception_win.cc:138: pool_used += head + 5; I think it's better ...

12 years, 9 months ago (2012-03-12 10:32:55 UTC) #4

timurrrr_at_google_com

12 years, 9 months ago (2012-03-12 11:50:08 UTC) #6

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

Thanks, r152557!

Expand All Messages | Collapse All Messages