AddressSanitizer: start factoring out interception machinery

[timurrrr_at_google_com, 2012-02-07 12:39:16]

http://codereview.appspot.com/5642046/diff/1/interception/interception.h
File interception/interception.h (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:38: //           DECLARE_REAL(...); will be located
inside namespaces.
"if ... are",
not
"if ... will be ..."

[glider, 2012-02-07 13:14:28]

2 general comments:

1. You'll need to get rid of all the ASanish stuff in these files. This is a bit
tricky on Mac, so I'm not insisting on factoring out the branch island allocator
now.
2. Please keep interception.h clean. You don't need to put anything but the
macros intended to be used by the clients there.

http://codereview.appspot.com/5642046/diff/1/interception/interception.h
File interception/interception.h (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:33: // asan_interceptors.cc, you'll instead need to:
Please rewrite this comment without mentioning ASan.

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:87: // foo with interceptor for other function.
"with an interceptor"

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:100: bool OverrideFunction(void *old_func, void
*new_func, void **orig_old_func);
You should not need these functions in the interface header.
Moreover, you don't need to define them for each platform.

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:102: bool GetRealFunctionAddress(const char
*func_name, void **func_addr);
Any reason not to return the address instead of bool here?

http://codereview.appspot.com/5642046/diff/1/interception/interception_linux.cc
File interception/interception_linux.cc (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception_linux....
interception/interception_linux.cc:10: bool OverrideFunction(void *old_func,
void *new_func, void **orig_old_func) {
Please remove this function.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc
File interception/interception_mac.cc (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc...
interception/interception_mac.cc:18: #include "../mach_override/mach_override.h"
Please put mach_override/ under interception/ as the former should now become a
part of the latter.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc...
interception/interception_mac.cc:34: # define kIslandEnd (0xffdf0000 -
kPageSize)
This stuff is in fact ASan-dependent and may not work with TSan. We need to
implement those __asan_[de]allocate_island routines in each tool. And they
certainly should not have the "__asan" prefix.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc...
interception/interception_mac.cc:71: orig_old_func,
mach_override_ptr_custom should have a custom prefix, too.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc...
interception/interception_mac.cc:79: // not implemented
Please remove this function.

[samsonov, 2012-02-07 14:31:32]

http://codereview.appspot.com/5642046/diff/1/interception/interception.h
File interception/interception.h (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:33: // asan_interceptors.cc, you'll instead need to:
On 2012/02/07 13:14:29, glider wrote:
> Please rewrite this comment without mentioning ASan.

Done.

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:38: //           DECLARE_REAL(...); will be located
inside namespaces.
On 2012/02/07 12:39:16, timurrrr_at_google_com wrote:
> "if ... are",
> not
> "if ... will be ..."

Done.

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:87: // foo with interceptor for other function.
On 2012/02/07 13:14:29, glider wrote:
> "with an interceptor"

Done.

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:100: bool OverrideFunction(void *old_func, void
*new_func, void **orig_old_func);
On 2012/02/07 13:14:29, glider wrote:
> You should not need these functions in the interface header.
Not sure. When the macros expand, the actual code in ASan RTL will have calls to
OverrideFunction(...), so we need to add them to some headers anyway. 
> Moreover, you don't need to define them for each platform.
Done, although I don't like the idea of different sets of functions present in
header for each platform.

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:102: bool GetRealFunctionAddress(const char
*func_name, void **func_addr);
On 2012/02/07 13:14:29, glider wrote:
> Any reason not to return the address instead of bool here?

To simplify a macro to a plain function call and avoid casts.

http://codereview.appspot.com/5642046/diff/1/interception/interception_linux.cc
File interception/interception_linux.cc (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception_linux....
interception/interception_linux.cc:10: bool OverrideFunction(void *old_func,
void *new_func, void **orig_old_func) {
On 2012/02/07 13:14:29, glider wrote:
> Please remove this function.

Done.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc
File interception/interception_mac.cc (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc...
interception/interception_mac.cc:18: #include "../mach_override/mach_override.h"
On 2012/02/07 13:14:29, glider wrote:
> Please put mach_override/ under interception/ as the former should now become
a
> part of the latter.

Added a FIXME. I'd rather do this in other commit.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc...
interception/interception_mac.cc:34: # define kIslandEnd (0xffdf0000 -
kPageSize)
On 2012/02/07 13:14:29, glider wrote:
> This stuff is in fact ASan-dependent and may not work with TSan. We need to
> implement those __asan_[de]allocate_island routines in each tool. And they
> certainly should not have the "__asan" prefix.

Right. Returned island allocation to asan_mac.cc and did a re-naming.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc...
interception/interception_mac.cc:71: orig_old_func,
On 2012/02/07 13:14:29, glider wrote:
> mach_override_ptr_custom should have a custom prefix, too.

So, you'd need different mach_override_ptr for each tool? (disregarding custom
allocate/deallocate island)

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc...
interception/interception_mac.cc:79: // not implemented
On 2012/02/07 13:14:29, glider wrote:
> Please remove this function.

Done.

[glider, 2012-02-07 16:42:36]

http://codereview.appspot.com/5642046/diff/1/interception/interception.h
File interception/interception.h (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:100: bool OverrideFunction(void *old_func, void
*new_func, void **orig_old_func);
#define INTERCEPT_FUNCTION(func)
#ifdef __linux__
INTERCEPT_FUNCTION_LINUX(func)
#elif __APPLE__
...

WDYT?

This won't dictate unnecessary interfaces to the systems that don't need them
and will hide this stuff from the user.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc
File interception/interception_mac.cc (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc...
interception/interception_mac.cc:71: orig_old_func,
On 2012/02/07 14:31:32, samsonov wrote:
> On 2012/02/07 13:14:29, glider wrote:
> > mach_override_ptr_custom should have a custom prefix, too.
> 
> So, you'd need different mach_override_ptr for each tool? (disregarding custom
> allocate/deallocate island)

We'll need an option to prepend a custom prefix to it.
I'll do that later in mach_override (I had asked Jonathan about that, but he
hasn't responded yet)

[kcc1, 2012-02-07 18:01:05]

In general, this is what we need.

- Please make sure that the real_* functions reside in a proper namespace.
Maybe you will need one more parameter to the macros (namespace name).
- The functions from the interception interfaces should reside in some
namspace  or have a __ prefix (__interception?)
- try to resolve FIXME's

--kcc


On Tue, Feb 7, 2012 at 4:30 AM, <samsonov@google.com> wrote:

> Reviewers: kcc, ramosian.glider, timurrrr_at_google_com,
>
>
>
> Please review this at
http://codereview.appspot.com/**5642046/<http://codereview.appspot.com/5642046/>
>
> Affected files:
>  M     Makefile.mk
>  M     Makefile.old
>  M     asan_interceptors.cc
>  M     asan_interceptors.h
>  M     asan_mac.cc
>  M     asan_malloc_mac.cc
>  A     interception/Makefile.mk
>  A     interception/interception.h
>  A     interception/interception_**linux.cc
>  A     interception/interception_mac.**cc
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>

[samsonov, 2012-02-08 07:35:54]

http://codereview.appspot.com/5642046/diff/1/interception/interception.h
File interception/interception.h (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception.h#newc...
interception/interception.h:100: bool OverrideFunction(void *old_func, void
*new_func, void **orig_old_func);
On 2012/02/07 16:42:36, glider wrote:
> #define INTERCEPT_FUNCTION(func)
> #ifdef __linux__
> INTERCEPT_FUNCTION_LINUX(func)
> #elif __APPLE__
> ...
> 
> WDYT?
> 
> This won't dictate unnecessary interfaces to the systems that don't need them
> and will hide this stuff from the user.

Where will you define INTERCEPT_FUNCTION_LINUX()? Anyway, the functions that are
called INTERCEPT_FUNCTION_LINUX will be inserted into user's code, and thus user
should have access to their definition. We can discourage to use these functions
themselves, though.

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc
File interception/interception_mac.cc (right):

http://codereview.appspot.com/5642046/diff/1/interception/interception_mac.cc...
interception/interception_mac.cc:71: orig_old_func,
On 2012/02/07 16:42:36, glider wrote:
> On 2012/02/07 14:31:32, samsonov wrote:
> > On 2012/02/07 13:14:29, glider wrote:
> > > mach_override_ptr_custom should have a custom prefix, too.
> > 
> > So, you'd need different mach_override_ptr for each tool? (disregarding
custom
> > allocate/deallocate island)
> 
> We'll need an option to prepend a custom prefix to it.
> I'll do that later in mach_override (I had asked Jonathan about that, but he
> hasn't responded yet)

OK, I guess we can deal with it a bit later.

[glider, 2012-02-08 08:19:17]

LGTM with nit.

http://codereview.appspot.com/5642046/diff/1012/asan_interceptors.h
File asan_interceptors.h (right):

http://codereview.appspot.com/5642046/diff/1012/asan_interceptors.h#newcode34
asan_interceptors.h:34: size_t internal_strlen(const char *s);
BTW aren't these functions already declared in asan_internal.h?

[samsonov, 2012-02-08 15:38:11]

kcc@:
1) Moved auxiliary functions under __interception namespace.
2) Why can't we for simplicity declare real_f inside __interception namespace as
well? This may break if two different libraries using interception are linked
together, but it's a mess anyway, as they'll have interceptors for the same
functions.
3) This CL is large already, so I'd prefer to resolve some FIXMEs after this CL
is submitted, if you don't mind.

glider@ suggested we should remove all OS-specific
details from the header "interception.h", so I moved them
to OS-specific headers. In this way the user code will not include <dlfcn.h> or
mach_override.h

http://codereview.appspot.com/5642046/diff/1012/asan_interceptors.h
File asan_interceptors.h (right):

http://codereview.appspot.com/5642046/diff/1012/asan_interceptors.h#newcode34
asan_interceptors.h:34: size_t internal_strlen(const char *s);
On 2012/02/08 08:19:17, glider wrote:
> BTW aren't these functions already declared in asan_internal.h?

Not yet.

[glider, 2012-02-08 15:43:11]

LGTM

[kcc1, 2012-02-08 17:30:39]

Looks good!

On Wed, Feb 8, 2012 at 7:38 AM, <samsonov@google.com> wrote:

> kcc@:
> 1) Moved auxiliary functions under __interception namespace.
> 2) Why can't we for simplicity declare real_f inside __interception
> namespace as well?

ok

> This may break if two different libraries using
> interception are linked together, but it's a mess anyway, as they'll
> have interceptors for the same functions.
> 3) This CL is large already, so I'd prefer to resolve some FIXMEs after
> this CL is submitted, if you don't mind.
>
ok

>
> glider@ suggested we should remove all OS-specific
> details from the header "interception.h", so I moved them
> to OS-specific headers. In this way the user code will not include
> <dlfcn.h> or mach_override.h
>
>
>
>
http://codereview.appspot.com/**5642046/diff/1012/asan_**interceptors.h<http:...
> File asan_interceptors.h (right):
>
> http://codereview.appspot.com/**5642046/diff/1012/asan_**
>
interceptors.h#newcode34<http://codereview.appspot.com/5642046/diff/1012/asan_interceptors.h#newcode34>
> asan_interceptors.h:34: size_t internal_strlen(const char *s);
> On 2012/02/08 08:19:17, glider wrote:
>
>> BTW aren't these functions already declared in asan_internal.h?
>>
>
> Not yet.
>
>
http://codereview.appspot.com/**5642046/<http://codereview.appspot.com/5642046/>
>

[samsonov, 2012-02-08 20:05:40]

r150083

> Looks good!
> 
> On Wed, Feb 8, 2012 at 7:38 AM, <mailto:samsonov@google.com> wrote:
> 
> > kcc@:
> > 1) Moved auxiliary functions under __interception namespace.
> > 2) Why can't we for simplicity declare real_f inside __interception
> > namespace as well?
> 
> ok
> 
> > This may break if two different libraries using
> > interception are linked together, but it's a mess anyway, as they'll
> > have interceptors for the same functions.
> > 3) This CL is large already, so I'd prefer to resolve some FIXMEs after
> > this CL is submitted, if you don't mind.
> >
> ok
> 
> >
> > glider@ suggested we should remove all OS-specific
> > details from the header "interception.h", so I moved them
> > to OS-specific headers. In this way the user code will not include
> > <dlfcn.h> or mach_override.h
> >
> >
> >
> >
>
http://codereview.appspot.com/**5642046/diff/1012/asan_**interceptors.h%3Chtt...>
> > File asan_interceptors.h (right):
> >
> > http://codereview.appspot.com/**5642046/diff/1012/asan_**
> >
>
interceptors.h#newcode34<http://codereview.appspot.com/5642046/diff/1012/asan_interceptors.h#newcode34>
> > asan_interceptors.h:34: size_t internal_strlen(const char *s);
> > On 2012/02/08 08:19:17, glider wrote:
> >
> >> BTW aren't these functions already declared in asan_internal.h?
> >>
> >
> > Not yet.
> >
> >
>
http://codereview.appspot.com/**5642046/%3Chttp://codereview.appspot.com/5642...>
> >