[cfe-commits] [llvm-commits] [PATCH] AddressSanitizer Initialization Order Checking

[Kostya Serebryany]

compiler-rt part is in: r162278.
I modified the code to use two lists (one for all globals and one for
dyninit globals) instead of using a weird structure with two links.
Simple tests work, I look forward to applying this to real worlds apps.

% head g1.cc g2.cc
==> g1.cc <==
extern int foo();
int x1 = foo();

==> g2.cc <==
extern int x1;
int bar() { return x1; }
int x2 = bar();
int foo() { return 2; }
int main() {
  return x1 + x2;
}

% clang -g -faddress-sanitizer -mllvm -asan-initialization-order g2.cc
g1.cc ; ./a.out 2>&1 | ./scripts/asan_symbolize.py / | c++filt
==14426== ERROR: AddressSanitizer initialization-order-fiasco ...
READ of size 4 at 0x0000006191c0 thread T0
    #0 0x40381c in bar() g2.cc:2
    #1 0x403a4c in __cxx_global_var_init g2.cc:3
...
0x0000006191c0 is located 0 bytes inside of global variable 'x1 (g1.cc)'
(0x6191c0) of size 4

I did not commit any of your tests because a) they will not work with cmake
and b) I'd like them not to depend on system libraries and be simpler in
general.
Please work with Alexey Samsonov on the tests and cmake.

Thanks!
--kcc

On Tue, Aug 21, 2012 at 12:35 PM, Kostya Serebryany <kcc at google.com> wrote:

>
>
> On Tue, Aug 21, 2012 at 11:36 AM, Kostya Serebryany <kcc at google.com>wrote:
>
>> submitted the clang part: r162259.
>> Looking at the rest (there are minor problems, I may be able to fix them
>> myself).
>>
>
> LLVM part went as r162268 with minor changes:
>
> 1. Don't call createInitializerPoisonCalls if there are no globals with
> dynamic init in the current TU.
> 2. Fix test instrument_initializer_metadata.ll to have correct metadata
> name
> 3. removed instrument_initializer.ll -- I did not understand why this test
> was needed. Please comment.
>
> Looking at the compiler-rt part, where I don't like some parts.
>
> --kcc
>
>
>>
>>
>> --kcc
>>
>>
>> On Tue, Aug 21, 2012 at 12:52 AM, Reid Watson <reidw at google.com> wrote:
>>
>>> OK, I re-reviewed what was contained in the patches.
>>> I had a few non-reviewed experiments accidentally included in the
>>> previous patches.
>>> I've attached the correct versions, containing only the properly
>>> reviewed changes.
>>> Sincere apologies for the trouble!
>>>
>>> On Mon, Aug 20, 2012 at 1:14 PM, Reid Watson <reidw at google.com> wrote:
>>> > Sorry, minor problem with the compiler-rt patch.
>>> >
>>> > Please commit the attached version of the compiler-rt patch instead of
>>> > the previous one.
>>> >
>>> > On Mon, Aug 20, 2012 at 10:57 AM, Reid Watson <reidw at google.com>
>>> wrote:
>>> >> I've incorporated all the review comments in the attached patches.
>>> >> The tool is off by default, and enabled by adding -mllvm
>>> >> -asan-initialization-order to clang.
>>> >>
>>> >> I don't have commit access, so I'd appreciate it if someone could
>>> >> commit these for me.
>>> >>
>>> >> All the best,
>>> >> Reid
>>> >>
>>> >> On Thu, Aug 16, 2012 at 6:09 PM, Reid Watson <reidw at google.com>
>>> wrote:
>>> >>> I'll add in the prefix before the final version for commit.
>>> >>>
>>> >>> The goal is to detect globals with dynamic initialization per the C++
>>> >>> standard, so that ASan knows which globals can only be accessed
>>> during
>>> >>> .  It's valid to access a statically initialized global inside of an
>>> >>> initializer, even if that global resides in a different TU.  Thus,
>>> >>> LLVM/ASan need to be able to distinguish between
>>> >>> dynamically/statically initialized globals.  I'm not certain this is
>>> >>> optimal, but there haven't been any unexpected false positives with
>>> >>> it.  We may be missing a few case, and the optimizer can certainly
>>> end
>>> >>> up leaving us with false negatives, but those are much better than
>>> >>> false positives.
>>> >>>
>>> >>> On Thu, Aug 16, 2012 at 5:27 PM, Eric Christopher <
>>> echristo at apple.com> wrote:
>>> >>>>
>>> >>>>
>>> >>>> On Aug 16, 2012, at 1:05 AM, Kostya Serebryany <kcc at google.com>
>>> wrote:
>>> >>>>
>>> >>>>> +llvm-commits
>>> >>>>>
>>> >>>>> Reid,
>>> >>>>>
>>> >>>>> The LLVM and compiler-rt patches look good.
>>> >>>>> Please fix the remaining small issues (see my code review
>>> comments) and commit.
>>> >>>>> Hold on with the output tests for a bit since Alexey Samsonov is
>>> migrating them to cmake (please coordinate with him and commit as a
>>> separate patch).
>>> >>>>>
>>> >>>>> The stress test should contain X files, Y linker initialized
>>> globals and Z dynamically initialized globals.
>>> >>>>> Such test only makes sense where all 3 numbers are large.
>>> >>>>> I guess you can commit a single .sh script into
>>> compiler-rt/lib/asan/scripts
>>> >>>>
>>> >>>> The metadata should at least be prefixed with something like
>>> llvm.asan.<whatever> instead of just the name. That way it's more
>>> identifiable.
>>> >>>>
>>> >>>> What's the idea behind the metadata use anyhow?
>>> >>>>
>>> >>>> -eric
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120821/d2ebb338/attachment.html>