Re: [asan] WIP protection of globals

[Xinliang David Li <davidxl at google dot com>]

On Tue, Oct 16, 2012 at 3:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Oct 16, 2012 at 02:41:42PM -0700, Xinliang David Li wrote:
>> > +bool
>> > +asan_protect_global (tree decl)
>> > +{
>> > +  rtx rtl, symbol;
>> > +  section *sect;
>> > +
>> > +  if (TREE_CODE (decl) != VAR_DECL
>> > +      || DECL_THREAD_LOCAL_P (decl)
>> > +      || DECL_EXTERNAL (decl)
>> > +      || !TREE_ASM_WRITTEN (decl)
>> > +      || !DECL_RTL_SET_P (decl)
>> > +      || DECL_ONE_ONLY (decl)
>> > +      || DECL_COMMON (decl)
>>
>> Why the above two condition? If the linker picks the larger size one,
>> it is ok to do the instrumentation.
>
> For DECL_ONE_ONLY, what LLVM does is plain wrong, it puts address of
> even vars exported from shared libraries (which can be overridden) into
> the array passed to __asan_*register_globals.  That can't work, you don't
> know if the var that is found first was compiled with -fasan or not.
> We need to use a local alias in that case (yeah, my WIP patch doesn't do
> that yet, but I wanted to post at least something), and I believe local

Does that mean that all globals defined in shared libraries can not be
protected as long as they are not protected or hidden? This sounds
like a big limitation.  We need to answer the following two questions:

1) How often are exported variables get preempted?
2) Is it a common use case to mix -fasan and -fno-asan ?

If the answer is no for either of the questions, we should allow the
above at the risk of some possible false positives -- as the goal is
to find as many bugs as possible (without too much noise). In short,
false negatives are worse.


> aliases might be an issue with DECL_ONE_ONLY (and anyway, if a -fno-asan
> DECL_ONE_ONLY var wins over -fasan one, there is no padding after it).
>
> LLVM does other things wrong, it increases the size of the vars which is
> IMHO a big no no, say for copy relocs etc., and last I'd say the relocations
> in the description variable are unnecessary, would be better if it e.g. used
> PC relative relocations and could made the array passed to
> __asan_*register_globals read-only.
>

I like the GCC way better too.


> And for DECL_COMMON, you can't put any padding after a common variable
> without making the size of the common var larger (and increasing its
> alignment), both are undesirable for -fasan/-fno-asan mixing.

If the linker picks the large one (which I believe it does), is that
still a problem?

>
>> > +      || (DECL_SECTION_NAME (decl) != NULL_TREE
>> > +         && !DECL_HAS_IMPLICIT_SECTION_NAME_P (decl))
>>
>> Why is this condition? Is it related to -fdata-sections ?
>
> -fdata-sections will have non-NULL DECL_SECTION_NAME, but still
> DECL_HAS_IMPLICIT_SECTION_NAME_P.  The above condition is not to break
> various packages that put say a struct into some user section and expect
> the section then to contain an array of those structs.
> E.g. Linux kernel does this, systemtap probes, prelink, ...
> If padding is inserted, all those would break.
>

Ok -- not common scenarios to be of a concern.

thanks,

David


>         Jakub