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

Kostya Serebryany kcc at google.com
Thu Jul 19 02:28:39 PDT 2012 

On Thu, Jul 19, 2012 at 3:16 AM, Reid Watson <reidw at google.com> wrote:

> Thanks again for reviewing this -- I've implemented the suggested
> changes, uploaded to Rietveld(http://codereview.appspot.com/6425049),
>
Thanks! (although you've uploaded only half of the patch).

Comments on the LLVM part:

+static bool HasDynamicInitializer(GlobalVariable *G) {



I am not sure how reliable this function is.
Can we get this information from clang in a form of metadata?
I don't insists to fix this now, but this will need a fix eventually.

+    if (CurrentFunction->getName().find("__cxx_global_var_init") == 0) {


Shouldn't this be _GLOBAL__I_a?


+  //   size_t poison_during_initialization;


I'd call this has_dynamic_initializer


Please end the comments with a period.

Comments on the compiler-rt part:

+  ScopedLock lock(&mu_for_globals);


+  if (n == 0)


+    return;

Move if (n == 0)  before the lock.



+  while (l->g->beg != last->beg) {

Do you really need these 3 loops?
At the point where you call this, the globals of the current TU are in the
head of the loop.
So, you probably can do just one loop starting from 'first'.

+  while (l) {


+    if (l->g->poison_during_initialization)


I am worried about the performance of this thing.
I would prefer to have two different lists of globals, one containing only
those globals that "has_dynamic_initializer".
You can do this change later if you prefer, but before enabling the option
by default.

Same for tests: you will need to add tests, not necessary in the first
commit, but before flipping the asan-initialization-order flag.
I will ask you for at least these kinds of tests:
 - llvm-style tests in the form of .ll file with CHECK statements.
 - "output" style tests (see projects/compiler-rt/lib/asan/output_tests)
with the actual bugs.
 - stress test: a shell script that auto-generates a program with N C++
modules each containing M1 globals with dynamic init and M2 globals with
static init.
   I want to see how O(N*M1) looks like in practice. We should not see
O(N*(M1+M2)).



> and attached the new patches to this email.
>
> I left in the extra check which forces access to global scalar
> variables to be implemented.  It remains necessary to catch the first
> case in my previous email,


Please remind me that case.
I think we need something like this:

if (ClOpt && ClOptGlobals)
   if(GlobalVariable *G = dyn_cast<GlobalVariable>(Addr)
&& !HasDynamicInitializer(G)) { ...

where HasDynamicInitializer is fast (e.g. reads metadata)



> and all initialization order problems in
> which the value being initialized is some simple type.  If the
> performance hit is unacceptable though,


Performance drop might be acceptable, but I don't want to disable a useful
optimization for no reason.

--kcc


> then I'd be open to adding an
> extra flag to control whether or not to check initialization order on
> these types of globals.
>
> All the best,
> Reid
>
> On Tue, Jul 17, 2012 at 6:31 AM, Kostya Serebryany <kcc at google.com> wrote:
> >
> >
> > On Sat, Jul 14, 2012 at 12:50 AM, Reid Watson <reidw at google.com> wrote:
> >>
> >> Ok, I've updated the code to reflect the suggested changes, except for
> >> two issues still up in the air.  Once these are solved I'll email out
> >> an updated patch.
> >>
> >> 1. Disabling ClOptGlobals, detecting linker initialized globals
> >> The code which disables the optimization of access to global scalar
> >> values is necessary for catching cases where the initialization order
> >> happens on a dynamically initialized global scalar value.  The
> >> following example prints different output depending on the order the
> >> input files are given to the compiler, and requires ClOptGlobals to be
> >> ignored at one point for this patch to catch it:
> >
> >
> > Consider you have a C++ program
> >
> >    int A;  // linker-initialized by zero
> >    int B = foo();  // initialized at start-up.
> >
> >    int bar() { return A + B; }
> >
> > Currently, with ClOptGlobals=1 (i.e. by default) both accesses in bar()
> are
> > ignored.
> > With your change, both accesses will be instrumented.
> > I think we still need to ignore the access to A.
> >
> >
> >
> >>
> >>
> >> //File x.cpp
> >> #incldue <cstdio>
> >> extern int y;
> >> int f() {
> >>     printf("using 'y' (which is %d)\n", y);
> >>     return 3*y+7;
> >> }
> >> int x = f();
> >> int main(){}
> >>
> >> //File y.cpp
> >> #include <cstdio>
> >> int g() {
> >>     puts("initializing 'y'");
> >>     return 5;
> >> }
> >> int y = g();
> >>
> >> Access to linker initialized globals can still result in
> >> initialization order problems.
> >> Consider this example, which outputs 42 or 84 depending on the order
> >> of arguments to the compiler.
> >> //FIle setcode.cpp
> >> static int code;
> >> int getCode(){ return code;}
> >> void setCode(int _code){  code = _code;}
> >>
> >> //File x.cpp
> >> #include <cstdio>
> >> void setCode(int);
> >> int getCode();
> >>
> >> int initializeX(){
> >>   setCode(42);
> >>   return 0;
> >> }
> >> int x = initializeX();
> >>
> >> int main() {
> >>   printf("getCode returns %d\n", getCode());
> >> }
> >>
> >> //File y.cpp
> >> void setCode(int);
> >>
> >> int initializeY(){
> >>   setCode(84);
> >>   return 0;
> >> }
> >>
> >> int y = initializeY();
> >>
> >>
> >> 2. Finding the global init function
> >> Right now, _GLOBAL__I_a is the hardcoded name of the function which
> >> will begin dynamic initialization.  It's possible to add metadata
> >> identifying it as that function, but the same hard-coding would still
> >> be in place.
> >
> >
> > Ok. Let this name be hardcoded, unless someone else has a better idea.
> >
> > --kcc
> >
> >>
> >> Thanks,
> >> Reid
> >>
> >> On Fri, Jul 13, 2012 at 10:02 AM, Reid Watson <reidw at google.com> wrote:
> >> > Kostya,
> >> >
> >> > I completely missed that (caught in a poorly written email filter).
> >> > Thanks for reviewing it for me, and I'll use rietveld when submitting
> >> > the update d version.
> >> > I'm looking at the code now, and I'll update when I've implemented the
> >> > suggested changes.
> >> >
> >> > --Reid
> >> >
> >> > On Fri, Jul 13, 2012 at 4:41 AM, Kostya Serebryany <kcc at google.com>
> >> > wrote:
> >> >> Reid,
> >> >>
> >> >> Have you seen my replies to your previous patches?
> >> >> http://comments.gmane.org/gmane.comp.compilers.clang.scm/53381
> >> >> http://permalink.gmane.org/gmane.comp.compilers.llvm.cvs/116782
> >> >>
> >> >> --kcc
> >> >>
> >> >> On Thu, Jul 12, 2012 at 9:19 PM, Reid Watson <reidw at google.com>
> wrote:
> >> >>>
> >> >>> Hello,
> >> >>>
> >> >>> Attached are two patches which add support for detecting
> >> >>> initialization order problems in AddressSanitizer.
> >> >>> Issues with initialization order are checked by poisoning the shadow
> >> >>> memory of global variables which are not guaranteed to have been
> >> >>> initialized before each modules initializers run.
> >> >>> This approach catches the most obvious issues with initialization
> >> >>> order, and correctly ignores access to global variables which should
> >> >>> be accessible during initializers (function local statics, etc.).
> >> >>>
> >> >>> All the best,
> >> >>> Reid
> >> >>>
> >> >>> _______________________________________________
> >> >>> llvm-commits mailing list
> >> >>> llvm-commits at cs.uiuc.edu
> >> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >> >>>
> >> >>
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120719/448b7710/attachment.html>

More information about the llvm-commits mailing list