[ARM] Avoid reference into modified vector

[nikic]

FirstCand is a reference to RepeatedSequenceLocs[0]. However, that vector is being modified a lot throughout the function, including one place that reassigns the whole vector. I'm not sure whether this can really happen in practice, but it doesn't seem unlikely that this could lead to a use-after-free.

Avoid this by directly using RepeatedSequenceLocs[0] at the start of the function (as a lot of other places already do) and only creating FirstCand at the end where no more modifications take place.

[llvmbot]

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-backend-arm

Author: Nikita Popov (nikic)

Changes

FirstCand is a reference to RepeatedSequenceLocs[0]. However, that vector is being modified a lot throughout the function, including one place that reassigns the whole vector. I'm not sure whether this can really happen in practice, but it doesn't seem unlikely that this could lead to a use-after-free.

Avoid this by directly using RepeatedSequenceLocs[0] at the start of the function (as a lot of other places already do) and only creating FirstCand at the end where no more modifications take place.

---

Full diff: https://github.com/llvm/llvm-project/pull/93965.diff

1 Files Affected:

• (modified) llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp (+2-3)

diff --git a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
index 8f873bee484ac..627148b73c4f5 100644
--- a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -5873,10 +5873,8 @@ static bool isLRAvailable(const TargetRegisterInfo &TRI,
 std::optional<outliner::OutlinedFunction>
 ARMBaseInstrInfo::getOutliningCandidateInfo(
     std::vector<outliner::Candidate> &RepeatedSequenceLocs) const {
-  outliner::Candidate &FirstCand = RepeatedSequenceLocs[0];
-
   unsigned SequenceSize = 0;
-  for (auto &MI : FirstCand)
+  for (auto &MI : RepeatedSequenceLocs[0])
     SequenceSize += getInstSizeInBytes(MI);
 
   // Properties about candidate MBBs that hold for all of them.
@@ -6071,6 +6069,7 @@ ARMBaseInstrInfo::getOutliningCandidateInfo(
   if (FlagsSetInAll & MachineOutlinerMBBFlags::HasCalls) {
     // check if the range contains a call.  These require a save + restore of
     // the link register.
+    outliner::Candidate &FirstCand = RepeatedSequenceLocs[0];
     if (std::any_of(FirstCand.begin(), std::prev(FirstCand.end()),
                     [](const MachineInstr &MI) { return MI.isCall(); }))
       NumBytesToCreateFrame += Costs.SaveRestoreLROnStack;

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[yroux]

LGTM, thanks

[rgwott]

Hi. This PR broke some tests downstream for us, using the ARM backend.
From my debugging, it seems that the new &FirstCand in ARMBaseInstrInfo.cpp:6072 may refer to an empty container, thus breaking the subsequent access in line 6073. In the broken test case, this happens because CandidatesWithoutStackFixups in line 6061 can be empty.
I do not have a reproducer right now but will try to get one.
We did not observe problems with AArch64 yet, but we also did not investigate.
Thanks in advance for looking into this.

[nikic]

@rgwott I'd love to have a reproducer for this -- it actually sounds like exactly the case I was concerned about here: RepeatedSequenceLocs gets reassigned to CandidatesWithoutStackFixups, while FirstCand still points into the old vector memory. If you run your test case under valgrind (or address sanitizer), do you maybe get a use-after-free report? I'd expect that to happen, if I'm not misunderstanding the code.

In any case, I'd be happy to revert this change in the meantime if it's causing issues for you.

[rgwott]

@nikic here is a reproducer:

void a();
void b(int c, int d, int, void *, struct e *f) { a(2, c, d, f); }
void g(int c, int d, int, void *, struct e *f) { a(4, c, d, f); }

clang --target=arm -mcpu=cortex-m55 -Oz repro.c

I did not see a use-after-free report in valgrind, but we do see this STL assertion:
/usr/include/c++/11/bits/stl_vector.h:1045: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](std::vector<_Tp, _Alloc>::size_type) [with _Tp = llvm::outliner::Candidate; _Alloc = std::allocator<llvm::outliner::Candidate>; std::vector<_Tp, _Alloc>::reference = llvm::outliner::Candidate&; std::vector<_Tp, _Alloc>::size_type = long unsigned int]: Assertion '__n < this->size()' failed.

No rush to revert this change, as I reverted it downstream to keep on working and I can reapply it once a fix comes from upstream.
Thanks!

[nikic]

@rgwott Thanks for the reproducer! I believe that #95410 should fix this.