ASan Initialization order checking - compiler-rt

Initialization order checking for ASan - compilerrt half.

[samsonov, 2012-07-24 07:04:47]

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_flags.h
File lib/asan/asan_flags.h (right):

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_flags.h#newcode53
lib/asan/asan_flags.h:53: bool initialization_order;
I'd prefer "check_initialization_order"

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

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode37
lib/asan/asan_globals.cc:37: static uptr DynamicInitializersCount,
InitializersHandled;
DynamicInitializersHandled?

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode116
lib/asan/asan_globals.cc:116: PoisonShadow(g->beg, g->size_with_redzone,
kAsanInitializationOrderMagic);
You can call PoisonGlobal(g) here.

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode160
lib/asan/asan_globals.cc:160: PoisonRedZones(g_copy);
Where do you actually fill the shadow for global with zeroes? (I've probably
missed it).

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode209
lib/asan/asan_globals.cc:209: void __asan_poison_globals(__asan_dynamic
*globals, uptr n) {
The function name and its action are a bit contradictory :)
If __asan_poison_globals is run before dynamic initialization of globals, and
__asan_unpoison_globals if run after it, maybe we should use it for naming?

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode225
lib/asan/asan_globals.cc:225: UnpoisonGlobal((__asan_dynamic*)l->g);
If you rely on fact that __asan_global is reinterptet-castable to __asan_dynamic
(i.e. one should be very careful with adding new fields to these structures),
please add a comment about this.

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_interface.h
File lib/asan/asan_interface.h (right):

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_interface.h#newcode46
lib/asan/asan_interface.h:46: struct __asan_dynamic {
why not __asan_dynamic_init_global?

[kcc1, 2012-07-24 08:38:53]

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

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode189
lib/asan/asan_globals.cc:189: if (!has_dynamic_init &&
globals[i].has_dynamic_initializer) {
This could be written simpler, although I doubt you need this at all.

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode223
lib/asan/asan_globals.cc:223: if (InitializersHandled ==
DynamicInitializersCount) {
that's gross. I don't like it. 

Besides, it looks like you significantly changed the logic since we discussed it
last time. 
It would be nice to have a comment describing the logic.

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_interface.h
File lib/asan/asan_interface.h (right):

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_interface.h#newcode46
lib/asan/asan_interface.h:46: struct __asan_dynamic {
I don't think you need a separate structure at all, why not just use the
existing one?

[reidw, 2012-07-26 23:43:08]

Second Version

[reidw, 2012-07-26 23:51:18]

Thanks again for reviewing this.  You've given me great feedback, and I really
appreciate it.

I've responded to the comments in the last review.
In the second version of the patch I also tried to give a bit more detail about
the functions used for poisoning in asan_globals.

The one thing that might still be objectionable is the method of determining
when all dynamic initializers have run and it's time to unpoison all globals (so
main can run).  Other then that, I believe I addressed most of the issues
raised.

Thanks again,
Reid

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_flags.h
File lib/asan/asan_flags.h (right):

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_flags.h#newcode53
lib/asan/asan_flags.h:53: bool initialization_order;
On 2012/07/24 07:04:47, samsonov wrote:
> I'd prefer "check_initialization_order"

Done

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

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode37
lib/asan/asan_globals.cc:37: static uptr DynamicInitializersCount,
InitializersHandled;
On 2012/07/24 07:04:47, samsonov wrote:
> DynamicInitializersHandled?
Done

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode116
lib/asan/asan_globals.cc:116: PoisonShadow(g->beg, g->size_with_redzone,
kAsanInitializationOrderMagic);
On 2012/07/24 07:04:47, samsonov wrote:
> You can call PoisonGlobal(g) here.
Done

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode160
lib/asan/asan_globals.cc:160: PoisonRedZones(g_copy);
On 2012/07/24 07:04:47, samsonov wrote:
> Where do you actually fill the shadow for global with zeroes? (I've probably
> missed it).

PoisonRedZones currently also poisons shadow.  I've added in an explicit call to
poison with 0 to clarify.

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode189
lib/asan/asan_globals.cc:189: if (!has_dynamic_init &&
globals[i].has_dynamic_initializer) {
On 2012/07/24 08:38:53, kcc1 wrote:
> This could be written simpler, although I doubt you need this at all. 
> 

I now determine and emit this information at compile time

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode209
lib/asan/asan_globals.cc:209: void __asan_poison_globals(__asan_dynamic
*globals, uptr n) {
On 2012/07/24 07:04:47, samsonov wrote:
> The function name and its action are a bit contradictory :)
> If __asan_poison_globals is run before dynamic initialization of globals, and
> __asan_unpoison_globals if run after it, maybe we should use it for naming?

Changed to __asan_before_dynamic_init, __asan_after_dynamic_init

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode223
lib/asan/asan_globals.cc:223: if (InitializersHandled ==
DynamicInitializersCount) {
On 2012/07/24 08:38:53, kcc1 wrote:
> that's gross. I don't like it. 
> 
> Besides, it looks like you significantly changed the logic since we discussed
it
> last time. 
> It would be nice to have a comment describing the logic. 

I've documented the logic of it, but I think that a minor variation on this
would work well

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode225
lib/asan/asan_globals.cc:225: UnpoisonGlobal((__asan_dynamic*)l->g);
On 2012/07/24 07:04:47, samsonov wrote:
> If you rely on fact that __asan_global is reinterptet-castable to
__asan_dynamic
> (i.e. one should be very careful with adding new fields to these structures),
> please add a comment about this.

Resolved by merging structures

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_interface.h
File lib/asan/asan_interface.h (right):

http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_interface.h#newcode46
lib/asan/asan_interface.h:46: struct __asan_dynamic {
On 2012/07/24 08:38:53, kcc1 wrote:
> I don't think you need a separate structure at all, why not just use the
> existing one? 

Done.  Was initially done because I didn't need name or has_dynamic_initializer

[kcc1, 2012-07-27 08:46:39]

On Fri, Jul 27, 2012 at 3:51 AM, <reidw@google.com> wrote:

> Thanks again for reviewing this.  You've given me great feedback, and I
> really appreciate it.
>
> I've responded to the comments in the last review.
> In the second version of the patch I also tried to give a bit more
> detail about the functions used for poisoning in asan_globals.
>
> The one thing that might still be objectionable is the method of
> determining when all dynamic initializers have run and it's time to
> unpoison all globals (so main can run).  Other then that, I believe I
> addressed most of the issues raised.
>

This is objectionable indeed. e.g. the globals could be initialized after
the main has stared (dlopen).
I like the initial approach much more, where you poison all globals (except
for the current module) in __asan_before_dynamic_init and then unpoison
them in __asan_after_dynamic_init
This algorithm *will* be quadratic (number of modules * number of dyn-init
globals), but with careful engineering this can be made quite fast.
Note that the linker-init globals should not be part of the process at all.



>
> Thanks again,
> Reid
>
>
>
http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**flags.h<http:/...
> File lib/asan/asan_flags.h (right):
>
> http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**
>
flags.h#newcode53<http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_flags.h#newcode53>
> lib/asan/asan_flags.h:53: bool initialization_order;
> On 2012/07/24 07:04:47, samsonov wrote:
>
>> I'd prefer "check_initialization_order"
>>
>
> Done
>
>
>
http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**globals.cc<htt...
> File lib/asan/asan_globals.cc (right):
>
> http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**
>
globals.cc#newcode37<http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode37>
> lib/asan/asan_globals.cc:37: static uptr DynamicInitializersCount,
> InitializersHandled;
> On 2012/07/24 07:04:47, samsonov wrote:
>
>> DynamicInitializersHandled?
>>
> Done
>
> http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**
>
globals.cc#newcode116<http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode116>
> lib/asan/asan_globals.cc:116: PoisonShadow(g->beg, g->size_with_redzone,
> kAsanInitializationOrderMagic)**;
> On 2012/07/24 07:04:47, samsonov wrote:
>
>> You can call PoisonGlobal(g) here.
>>
> Done
>
> http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**
>
globals.cc#newcode160<http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode160>
> lib/asan/asan_globals.cc:160: PoisonRedZones(g_copy);
> On 2012/07/24 07:04:47, samsonov wrote:
>
>> Where do you actually fill the shadow for global with zeroes? (I've
>>
> probably
>
>> missed it).
>>
>
> PoisonRedZones currently also poisons shadow.  I've added in an explicit
> call to poison with 0 to clarify.
>
>
> http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**
>
globals.cc#newcode189<http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode189>
> lib/asan/asan_globals.cc:189: if (!has_dynamic_init &&
> globals[i].has_dynamic_**initializer) {
> On 2012/07/24 08:38:53, kcc1 wrote:
>
>> This could be written simpler, although I doubt you need this at all.
>>
>
>
> I now determine and emit this information at compile time
>
> http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**
>
globals.cc#newcode209<http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode209>
> lib/asan/asan_globals.cc:209: void __asan_poison_globals(__asan_**dynamic
> *globals, uptr n) {
> On 2012/07/24 07:04:47, samsonov wrote:
>
>> The function name and its action are a bit contradictory :)
>> If __asan_poison_globals is run before dynamic initialization of
>>
> globals, and
>
>> __asan_unpoison_globals if run after it, maybe we should use it for
>>
> naming?
>
> Changed to __asan_before_dynamic_init, __asan_after_dynamic_init
>
>
> http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**
>
globals.cc#newcode223<http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode223>
> lib/asan/asan_globals.cc:223: if (InitializersHandled ==
> DynamicInitializersCount) {
> On 2012/07/24 08:38:53, kcc1 wrote:
>
>> that's gross. I don't like it.
>>
>
>  Besides, it looks like you significantly changed the logic since we
>>
> discussed it
>
>> last time.
>> It would be nice to have a comment describing the logic.
>>
>
> I've documented the logic of it, but I think that a minor variation on
> this would work well
>
> http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**
>
globals.cc#newcode225<http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_globals.cc#newcode225>
> lib/asan/asan_globals.cc:225: UnpoisonGlobal((__asan_**dynamic*)l->g);
> On 2012/07/24 07:04:47, samsonov wrote:
>
>> If you rely on fact that __asan_global is reinterptet-castable to
>>
> __asan_dynamic
>
>> (i.e. one should be very careful with adding new fields to these
>>
> structures),
>
>> please add a comment about this.
>>
>
> Resolved by merging structures
>
>
>
http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**interface.h<ht...
> File lib/asan/asan_interface.h (right):
>
> http://codereview.appspot.com/**6419070/diff/1/lib/asan/asan_**
>
interface.h#newcode46<http://codereview.appspot.com/6419070/diff/1/lib/asan/asan_interface.h#newcode46>
> lib/asan/asan_interface.h:46: struct __asan_dynamic {
> On 2012/07/24 08:38:53, kcc1 wrote:
>
>> I don't think you need a separate structure at all, why not just use
>>
> the
>
>> existing one?
>>
>
> Done.  Was initially done because I didn't need name or
> has_dynamic_initializer
>
>
http://codereview.appspot.com/**6419070/<http://codereview.appspot.com/6419070/>
>

[reidw, 2012-07-31 01:31:28]

Version 3: fairly minor changes

[samsonov, 2012-08-01 16:09:30]

http://codereview.appspot.com/6419070/diff/14001/lib/asan/asan_globals.cc
File lib/asan/asan_globals.cc (right):

http://codereview.appspot.com/6419070/diff/14001/lib/asan/asan_globals.cc#new...
lib/asan/asan_globals.cc:106: // so we store the globals in a map.
Ouch, the comment seems broken here - there is no more map! I'm probably a bit
out of context, but do you know cases when we can register the same global
several times? Will it break you LIFO-assumption?

http://codereview.appspot.com/6419070/diff/14001/lib/asan/asan_globals.cc#new...
lib/asan/asan_globals.cc:137: static void PoisonGlobal(const Global *g) {
PoisonGlobalAndRedzones()

http://codereview.appspot.com/6419070/diff/14001/lib/asan/asan_globals.cc#new...
lib/asan/asan_globals.cc:215: for (; l->g->beg != last->beg; l = l->next)
The following methods will break if for some reason you call
__asan_before_dynamic_init on list of globals that is not in our list of globals
(or their order in the list_of_globals is not expected), right? Please add some
checks, so that we can diagnose this, or print warning messages instead of
crashing.

[reidw, 2012-08-01 19:29:35]

I've responded to all of the suggested changes, and I'll be uploading the patch
corresponding to these changes tomorrow.

All the best,
Reid

http://codereview.appspot.com/6419070/diff/14001/lib/asan/asan_globals.cc
File lib/asan/asan_globals.cc (right):

http://codereview.appspot.com/6419070/diff/14001/lib/asan/asan_globals.cc#new...
lib/asan/asan_globals.cc:106: // so we store the globals in a map.
On 2012/08/01 16:09:30, samsonov wrote:
> Ouch, the comment seems broken here - there is no more map! I'm probably a bit
> out of context, but do you know cases when we can register the same global
> several times? Will it break you LIFO-assumption?

This would cause us to do a bit of extra poisoning, but it shouldn't break
anything.  I made a minor fix to global poisoning just to be extra safe (case
that the first global in the TU was inserted multiple times).

http://codereview.appspot.com/6419070/diff/14001/lib/asan/asan_globals.cc#new...
lib/asan/asan_globals.cc:137: static void PoisonGlobal(const Global *g) {
On 2012/08/01 16:09:30, samsonov wrote:
> PoisonGlobalAndRedzones()

Done

http://codereview.appspot.com/6419070/diff/14001/lib/asan/asan_globals.cc#new...
lib/asan/asan_globals.cc:215: for (; l->g->beg != last->beg; l = l->next)
On 2012/08/01 16:09:30, samsonov wrote:
> The following methods will break if for some reason you call
> __asan_before_dynamic_init on list of globals that is not in our list of
globals
> (or their order in the list_of_globals is not expected), right? Please add
some
> checks, so that we can diagnose this, or print warning messages instead of
> crashing.

Done, added a few CHECKs and comments describing how they would come about.

[reidw, 2012-08-02 17:45:03]

More minor changes

[samsonov, 2012-08-03 07:55:29]

LGTM (modulo comments below).

http://codereview.appspot.com/6419070/diff/17005/lib/asan/asan_globals.cc
File lib/asan/asan_globals.cc (right):

http://codereview.appspot.com/6419070/diff/17005/lib/asan/asan_globals.cc#new...
lib/asan/asan_globals.cc:29: struct ListOfGlobals {
Please write your assertion on the structure of ListOfGlobals here.

http://codereview.appspot.com/6419070/diff/17005/lib/asan/asan_globals.cc#new...
lib/asan/asan_globals.cc:200: // Even if all global variables have been
optimized away, initializers may
In general, I think this approach is ok. You may want to simplify this function
and remove special case for n==0 by this structure (leaving this up to you):

Global *first = (n > 0) ? &globals[0] ? : 0;
Global *last = (n > 0) ? &globals[n-1] ? : 0;
ListOfGlobals *l = list_of_globals;
CHECK(l);
bool global_is_in_this_tu = false;
for (; l; l = l->next) {
  Global g = l->g;
  if (g->beg == last->beg) global_is_in_this_tu = true;
  if (!global_is_in_this_tu && g->has_dynamic_initializer)
    PoisonGlobalAndRedzones(g);
  if (g->beg == first->beg) global_is_in_this_tu = false;
  // Maybe check that there l->next->g->beg is not
  // first->beg as well.
}
CHECK(!global_is_in_this_tu);

http://codereview.appspot.com/6419070/diff/17005/lib/asan/asan_globals.cc#new...
lib/asan/asan_globals.cc:253: // If we've run all initializers, unpoison all
global variables.
Comment is obsolete.

http://codereview.appspot.com/6419070/diff/17005/lib/asan/asan_interface.h
File lib/asan/asan_interface.h (right):

http://codereview.appspot.com/6419070/diff/17005/lib/asan/asan_interface.h#ne...
lib/asan/asan_interface.h:41: // Whether or not the global has a dynamic
initializer.
uptr has_dynamic_initializer;  // Whether or not
                               // whatever.

[kcc1, 2012-08-16 07:35:00]

Looks almost good. 
Please fix the minor issues and commit the code. 
Please commit tests separately after discussing with samsonov

http://codereview.appspot.com/6419070/diff/23001/lib/asan/asan_interface.h
File lib/asan/asan_interface.h (right):

http://codereview.appspot.com/6419070/diff/23001/lib/asan/asan_interface.h#ne...
lib/asan/asan_interface.h:41: __asan_global *next_dynamic;
Please don't add a field here. Instead, use the existing structure ListOfGlobals

http://codereview.appspot.com/6419070/diff/23001/lib/asan/output_tests/test_o...
File lib/asan/output_tests/test_output.sh (right):

http://codereview.appspot.com/6419070/diff/23001/lib/asan/output_tests/test_o...
lib/asan/output_tests/test_output.sh:1: #!/bin/bash
This file is going to be deprecated. 
Please coordinate with samsonov@

[glider1, 2012-08-16 09:09:31]

I was unable to comment on asan_rtl.cc and asan_flags.h since Rietveld couldn't
apply your patch.
Please make sure the new feature is off by default for now.

[samsonov, 2012-08-16 15:17:05]

http://codereview.appspot.com/6419070/diff/23001/lib/asan/output_tests/test_o...
File lib/asan/output_tests/test_output.sh (right):

http://codereview.appspot.com/6419070/diff/23001/lib/asan/output_tests/test_o...
lib/asan/output_tests/test_output.sh:1: #!/bin/bash
On 2012/08/16 07:35:00, kcc1 wrote:
> This file is going to be deprecated. 
> Please coordinate with samsonov@

Yeah, we're in process of converting our tests to llvm-lit runner. You may take
a look at asan/lit_tests folder (you'll probably need to use same test layout as
used for shared libraries tests). Yes, it looks pretty ugly :( Anyway, feel free
to commit the code and/or old-style tests.