[PATCH] D75695: [StackProtector] Catch direct out-of-bounds when checking address-takenness

[John Brawn via Phabricator via llvm-commits]

john.brawn marked an inline comment as done.
john.brawn added inline comments.


================
Comment at: llvm/lib/CodeGen/StackProtector.cpp:202
+      APInt MaxOffset(TypeSize, DL.getTypeStoreSize(
+                        AI->getType()->getPointerElementType()));
+      if (!GEP->accumulateConstantOffset(DL, Offset) || Offset.uge(MaxOffset))
----------------
efriedma wrote:
> john.brawn wrote:
> > efriedma wrote:
> > > I think AllocaInst has a getAllocatedType, or something like that?
> > > 
> > > Should use getTypeAllocSize, since that's what actually determines the number of bytes allocated.
> > > I think AllocaInst has a getAllocatedType, or something like that?
> > AI isn't necessarily an AllocaInst, e.g. we could be looking at a GEP of a GEP or a GEP of a PHI.
> > 
> > > Should use getTypeAllocSize, since that's what actually determines the number of bytes allocated.
> > Will do.
> > 
> Oh, I see, the function recurses.
> 
> I don't think the recursion works correctly in general.  The values that actually matter are the current offset, determined recursively, the size of the original alloca, and the size of any memory operations using the result of the GEP.  Using the size of the GEP's operand type doesn't work correctly in general; in particular, if there's another GEP or bitcast in the recursive chain of uses, you can come up with the wrong result.
I think adding a check that bitcast doesn't bitcast to a larger type should be enough to handle this. If we have a GEP of a GEP then each one will make sure that the offset is within the bounds of the specific type it's handling. Doing it like this does mean that for something like

```
int fn() {
  int arr[4][4];
  return arr[0][5];
}
```
we say it's an out-of-bounds access when it doesn't actually access outside of the underlying object, but I don't think handling this kind of case is worth the added complexity.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75695/new/

https://reviews.llvm.org/D75695