Fixing some StackProtector issues

PR43308 describes a case where StackProtector fails to protect against
a fairly simple smash. This problem started after r363169, which
removed StackProtector's own analysis function HasAddressTaken, and
used CaptureTracking's PointerMayBeCaptured instead. The problem here
is that "pointer is captured" and "pointer could be used to smash the
stack" are not equivalent queries. The header of CaptureTracking.cpp
says in part:
  A pointer value is captured if the function makes a copy of any part
  of the pointer that outlives the call.
Clearly, stacks can be smashed without a "capture" occurring; a smash
simply uses a pointer in a normal-looking way, but just oversteps the
bounds of the pointed-to data.

r363169 was intended to fix PR42238, so undoing the patch will have to
come up with some other fix for PR42238. Also, there's PR40436, which
identified an infinite recursion in HasAddressTaken; there's D57149 to
fix that, which was abandoned as irrelevant after r363169.

Finally, aside from just fixing PR42238, the commit log for r363169
also notes that it "fixes some infrastructure issues to allow running
just the IR pass." That fix should be preserved.

Overall, I think the appropriate measures are:

1. Revert r363169, which fixes PR43308 but re-introduces PR42238 and
   PR40436.
2. Re-add the "infrastructure" fix from r363169.
3. Fix PR42238 by enhancing HasAddressTaken to look at new instructions.
4. Fix PR40436, by reviving D57149.

I'll start working on a patch series for the first 3 items; the
original author of D57149 can decide about reviving that one.

--paulr

From: llvm-dev [mailto:llvm-dev-bounces@lists.llvm.org] On Behalf Of via
llvm-dev
Sent: Thursday, September 19, 2019 10:37 AM
To: llvm-dev@lists.llvm.org
Subject: [llvm-dev] Fixing some StackProtector issues

PR43308 describes a case where StackProtector fails to protect against
a fairly simple smash. This problem started after r363169, which
removed StackProtector's own analysis function HasAddressTaken, and
used CaptureTracking's PointerMayBeCaptured instead. The problem here
is that "pointer is captured" and "pointer could be used to smash the
stack" are not equivalent queries. The header of CaptureTracking.cpp
says in part:
  A pointer value is captured if the function makes a copy of any part
  of the pointer that outlives the call.
Clearly, stacks can be smashed without a "capture" occurring; a smash
simply uses a pointer in a normal-looking way, but just oversteps the
bounds of the pointed-to data.

r363169 was intended to fix PR42238, so undoing the patch will have to
come up with some other fix for PR42238. Also, there's PR40436, which
identified an infinite recursion in HasAddressTaken; there's D57149 to
fix that, which was abandoned as irrelevant after r363169.

Finally, aside from just fixing PR42238, the commit log for r363169
also notes that it "fixes some infrastructure issues to allow running
just the IR pass." That fix should be preserved.

Overall, I think the appropriate measures are:

1. Revert r363169, which fixes PR43308 but re-introduces PR42238 and
   PR40436.

See https://reviews.llvm.org/D67842
Ordinarily a revert wouldn't go through review, but as it's a base for
the other patches, I figured putting it on Phab couldn't hurt. Also
it's two commits (the second was just a whitespace cleanup) making it
slightly awkward for reviewers to do themselves if they wish; this way
there's a single diff to download.

2. Re-add the "infrastructure" fix from r363169.

On second thought, no. This change was to allow running the StackProtector
pass from opt; but it's a CodeGen pass, not a Transform, and it can already
be run in isolation by llc. So this isn't needed.

3. Fix PR42238 by enhancing HasAddressTaken to look at new instructions.

See https://reviews.llvm.org/D67845 for a preliminary refactoring.
See https://reviews.llvm.org/D67846 for the functional change.

Not sure what you mean. llc can’t be used for running IR->IR passes

-Matt

On second thought, no. This change was to allow running the StackProtector
pass from opt; but it's a CodeGen pass, not a Transform, and it can already
be run in isolation by llc. So this isn't needed.

Not sure what you mean. llc can’t be used for running IR->IR passes

-Matt

Not sure why you think that. llc can run any CodeGen pass. See other
examples of llc used to run StackProtector:
  test/CodeGen/ARM/fpoffset_overflow.mir
  test/CodeGen/X86/vector-truncate-combine.ll
  test/CodeGen/X86/stack-protector-remarks.ll

--paulr