[PATCH] D13403: Better handling of function calls in SafeStack

[Peter Collingbourne via llvm-commits]

pcc added a comment.

I suppose a higher level point is whether it is sufficient to rely only on the `readonly` attribute. Consider this function:

  void *f(void **a, int x) {
    return a[x];
  }

The `a` argument would be `readonly` and `nocapture`, but it could leak the address of the safe stack given an appropriate `x`. We already protect against this kind of thing within a function (via the gep case in `SafeStack::IsSafeStackAlloca`), but not across function call boundaries.

Maybe we do in fact need something like a global analysis like `FunctionAttrs` that attaches `safestack` attributes to parameters.


================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:238
@@ -109,3 +237,3 @@
         // We assume that GEP on static alloca with constant indices is safe,
         // otherwise a compiler would detect it and warn during compilation.
 
----------------
It occurs to me that this isn't necessarily a valid assumption (consider for example an access with out-of-bounds indices that were constant folded by the optimizer from something that the frontend wasn't smart enough to warn about).

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:272
@@ -137,3 +271,3 @@
         // We assume that passing a pointer to an object as a 'nocapture'
         // argument is safe.
         // FIXME: a more precise solution would require an interprocedural
----------------
This comment should be updated.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:281
@@ +280,3 @@
+              return false;
+            if (!AA->onlyReadsMemory(CS) && !CS.onlyReadsMemory(A - B))
+              return false;
----------------
I would just check for the attribute here. The `FunctionAttrs` pass should have already set attributes according to AA results.


Repository:
  rL LLVM

http://reviews.llvm.org/D13403