[PATCH] Implement nonnull-attribute sanitizer

[Alexey Samsonov]

================
Comment at: tools/clang/lib/CodeGen/CGCall.cpp:2414-2415
@@ +2413,4 @@
+                                unsigned ParmNum) {
+  if (!CGF.SanOpts->NonnullAttribute || !FD || ParmNum >= FD->getNumParams())
+    return;
+  const NonNullAttr *NNAtt = FD->getAttr<NonNullAttr>();
----------------
rsmith wrote:
> samsonov wrote:
> > rsmith wrote:
> > > What should happen here:
> > > 
> > >   __attribute__((nonnull)) void f(const char *, ...);
> > >   int main() { void *p = 0; f("%s", p); }
> > > 
> > > (I have no idea if the attribute applies in this case.)
> > Neither am I :( GCC docs say that "If no argument index list is given to the nonnull attribute, all pointer arguments are marked as non-null", but it's not clear if it refers to call arguments, or formal parameters. What is worse, GCC and Clang seem to disagree on this:
> >   $ cat tmp/ubsan/bad.cpp
> >   __attribute__((nonnull)) void *foo2(int *a, ...);
> >   
> >   void bar() { foo2(0, (void*)0); }
> >   $ g++ -Wnonnull -Wall tmp/ubsan/bad.cpp -c -o /dev/null
> >   tmp/ubsan/bad.cpp: In function ‘void bar()’:
> >   tmp/ubsan/bad.cpp:3:30: warning: null argument where non-null required (argument 1) [-Wnonnull]
> >   void bar() { foo2(0, (void*)0); }
> >                               ^
> >   tmp/ubsan/bad.cpp:3:30: warning: null argument where non-null required (argument 2) [-Wnonnull]
> >   $ ./bin/clang++ -Wnonnull -Wall tmp/ubsan/bad.cpp -c -o /dev/null
> >   tmp/ubsan/bad.cpp:3:30: warning: null passed to a callee which requires a non-null argument [-Wnonnull]
> >   void bar() { foo2(0, (void*)0); }
> >                     ~          ^
> >   1 warning generated.
> > 
> > Sigh. Should Clang strive for GCC-compatible behavior, or you have the idea of which behavior is "more predictable" in this case?
> I think the GCC behavior makes more sense. We should fix the frontend to also understand this, and model it properly in the AST. Our current representation doesn't work.
Vararg sanitization should now work.

================
Comment at: tools/clang/lib/CodeGen/CGCall.cpp:2416-2420
@@ +2415,7 @@
+    return;
+  const NonNullAttr *NNAtt = FD->getAttr<NonNullAttr>();
+  auto PVD = FD->getParamDecl(ParmNum);
+  if (!(NNAtt && NNAtt->isNonNull(PVD->getFunctionScopeIndex())) &&
+      !PVD->hasAttr<NonNullAttr>())
+    return;
+  CodeGenFunction::SanitizerScope SanScope(&CGF);
----------------
samsonov wrote:
> rsmith wrote:
> > Can a function have multiple `__attribute__((nonnull(N)))`s on it?
> Nice catch! Both Clang and GCC accept this code and seem to calculate union of all arguments passed into nonnull().
> 
>   $ cat tmp/ubsan/bad.cpp
>   __attribute__((nonnull(1)))
>   __attribute__((nonnull(2)))
>   void foo(int *a, int *a2);
>   void bar() { foo(0, 0); }
>   $ g++ -Wnonnull -Wall tmp/ubsan/bad.cpp -c -o /dev/null
>   tmp/ubsan/bad.cpp: In function ‘void bar()’:
>   tmp/ubsan/bad.cpp:9:11: warning: null argument where non-null required (argument 1) [-Wnonnull]
>      foo(0, 0);
>              ^
>   tmp/ubsan/bad.cpp:9:11: warning: null argument where non-null required (argument 2) [-Wnonnull]
>   $ ./bin/clang++ -Wnonnull -Wall tmp/ubsan/bad.cpp -c -o /dev/null
>   tmp/ubsan/bad.cpp:9:11: warning: null passed to a callee which requires a non-null argument [-Wnonnull]
>     foo(0, 0);
>            ~^
>   tmp/ubsan/bad.cpp:9:11: warning: null passed to a callee which requires a non-null argument [-Wnonnull]
>     foo(0, 0);
>         ~   ^
>   2 warnings generated.
> 
> The code as written won't handle this case properly (the last nonnull() attribute in function declaration will win). Sadly, there is the same problem already present in EmitFunctionProlog() function. I will work on a fix in a separate change.
Fixed

================
Comment at: tools/clang/lib/CodeGen/CGCall.cpp:2422
@@ +2421,3 @@
+  CodeGenFunction::SanitizerScope SanScope(&CGF);
+  assert(RV.isScalar());
+  llvm::Value *V = RV.getScalarVal();
----------------
rsmith wrote:
> samsonov wrote:
> > rsmith wrote:
> > > What guarantees this? I don't see where you're checking that the parameter is of a pointer type.
> > This seems to be handled by Sema:
> > 
> >   tmp/ubsan/bad.cpp:1:33: warning: 'nonnull' attribute only applies to pointer arguments [-Wignored-attributes]
> >   void foo(int *a, __attribute__((nonnull)) int b);
> >                    ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
> > 
> I was thinking about this case:
> 
>   void f(non_scalar_type x) __attribute__((nonnull));
>   void g() { f(x); }
> 
> ... but it looks like the attribute handling flattens the attribute into a list of parameters when it's applied. That's broken in several ways, alas. For instance, we fail to diagnose this properly:
> 
>   template<typename T> __attribute__((nonnull)) void f(T t);
>   void g() { f((void*)0); }
> 
> ... because we can't preserve that we had a parameter-less nonnull attribute until we get to template instantiation.
Fixed

http://reviews.llvm.org/D5082