fix(stack-buffer): check accesses through a local pointer to an array element

CHANGELOG.md

@@ -16,9 +16,11 @@ Notable changes are grouped by release. See [all releases](https://github.com/Co
   bound the index, the existing result stays.
 - Stack buffer overflow checks no longer take the store of an element's address, such as
   `char* end = &buf[10]` or the end of a range-based `for`, for a write to that element.
-  Forming a pointer one past the end is valid C and C++. A write through such a pointer kept in
-  a local variable is not reported either: it used to be reported only by that mistake, on the
-  line that formed the address.
+  Forming a pointer one past the end is valid C and C++. A read or write through a pointer
+  variable holding the address is checked where it happens instead: `char* p = &buf[10];
+  *p = 42;` is reported on `*p = 42`, with the variable in the alias path. The variable is
+  followed when the store of the address is its last write on every path to the access; one
+  assigned on a single branch is not.
 - VLA sizes, local stack sizes and the "this access appears unreachable" note on buffer
   overflows read a local variable from the store that last wrote it, as MemorySSA finds it, and
   apply casts to it. They used to take the last constant stored anywhere in the function. A

src/analysis/StackBufferAnalysis.cpp

@@ -10,6 +10,7 @@
 #include <llvm/ADT/APInt.h>
 #include <llvm/ADT/SmallPtrSet.h>
 #include <llvm/ADT/SmallVector.h>
+#include <llvm/Analysis/MemorySSA.h>
 #include <llvm/Analysis/ValueTracking.h>
 #include <llvm/BinaryFormat/Dwarf.h>
 #include <llvm/IR/Constants.h>
@@ -896,6 +897,67 @@ namespace ctrace::stack::analysis
             return first && first->isZero();
         }
 
+        /// The loads and stores that use @p address: directly, and with @p followVariables also
+        /// through the pointer variables it is stored in (`char* p = &buf[i]; *p = 0;`), from
+        /// one variable to the next. A store of the address is not an access to it.
+        ///
+        /// A load of a variable stands for the address only when its MemorySSA clobber is the
+        /// store of the address to that variable, with the same type. The load then reads that
+        /// address, and as the store dominates the load, the values the address was computed
+        /// from still hold the values they had at the store: a load in the next iteration of a
+        /// loop has a merge for clobber, and is left out. Each load has one clobber, so the walk
+        /// meets each load once.
+        ///
+        /// @p via receives, for each access through variables, the names that come before the
+        /// alias path of the address: the variables, the last one first, then the address.
+        static void
+        collectAccesses(llvm::GetElementPtrInst& address, bool followVariables,
+                        const FunctionFacts& facts,
+                        llvm::SmallVectorImpl<llvm::Instruction*>& accesses,
+                        std::unordered_map<const llvm::Instruction*, std::vector<std::string>>& via)
+        {
+            using namespace llvm;
+
+            SmallVector<std::pair<Value*, std::vector<std::string>>, 4> pointers;
+            pointers.emplace_back(&address, std::vector<std::string>{});
+            if (address.hasName())
+                pointers.back().second.push_back(address.getName().str());
+            while (!pointers.empty())
+            {
+                auto [pointer, names] = pointers.pop_back_val();
+                for (User* user : pointer->users())
+                {
+                    auto* store = dyn_cast<StoreInst>(user);
+                    if (isa<LoadInst>(user) || (store && store->getPointerOperand() == pointer))
+                    {
+                        accesses.push_back(cast<Instruction>(user));
+                        if (pointer != &address)
+                            via[accesses.back()] = names;
+                    }
+                    if (!followVariables || !store || !store->isSimple() ||
+                        store->getValueOperand() != pointer)
+                        continue;
+
+                    Value* variable = store->getPointerOperand();
+                    for (User* variableUser : variable->users())
+                    {
+                        auto* load = dyn_cast<LoadInst>(variableUser);
+                        if (!load || !load->isSimple() || load->getType() != pointer->getType())
+                            continue;
+                        const auto* def =
+                            dyn_cast_or_null<MemoryDef>(facts.clobberingAccess(*load));
+                        if (!def || def->getMemoryInst() != store)
+                            continue;
+                        std::vector<std::string> loadNames;
+                        if (variable->hasName())
+                            loadNames.push_back(variable->getName().str());
+                        loadNames.insert(loadNames.end(), names.begin(), names.end());
+                        pointers.emplace_back(load, std::move(loadNames));
+                    }
+                }
+            }
+        }
+
         static void analyzeStackBufferOverflowsInFunction(
             llvm::Function& F, std::vector<StackBufferOverflowIssue>& out,
             const AnalysisComplexityBudgets& budgets,
@@ -1074,15 +1136,22 @@ namespace ctrace::stack::analysis
                             GV->hasName() ? GV->getName().str() : std::string("<unnamed-global>");
                     }
 
-                    // The loads and stores through the GEP, each reported on its own. A store of
-                    // the address itself (char* p = &buf[i]) accesses nothing.
+                    // A constant index within bounds is safe wherever the address is used.
+                    auto* CIdx = dyn_cast<ConstantInt>(idxVal);
+                    if (CIdx && CIdx->getSExtValue() >= 0 &&
+                        static_cast<StackSize>(CIdx->getSExtValue()) < arraySize)
+                        continue;
+
+                    // The loads and stores that use the address, each reported on its own. The
+                    // pointer variables it is stored in are followed only when it is computed
+                    // from the array itself: the index then counts from the array's start.
                     SmallVector<Instruction*, 4> accesses;
-                    for (User* GU : GEP->users())
-                    {
-                        const auto* store = dyn_cast<StoreInst>(GU);
-                        if (isa<LoadInst>(GU) || (store && store->getPointerOperand() == GEP))
-                            accesses.push_back(cast<Instruction>(GU));
-                    }
+                    std::unordered_map<const Instruction*, std::vector<std::string>> via;
+                    const Value* array = AI ? static_cast<const Value*>(AI) : GV;
+                    collectAccesses(*GEP,
+                                    allowPointerStoreScan &&
+                                        GEP->getPointerOperand()->stripPointerCasts() == array,
+                                    facts, accesses, via);
                     const bool pilotAccess = isIndexBoundsPilotAccess(*GEP, AI);
                     const auto newReport = [&](Instruction& access)
                     {
@@ -1098,8 +1167,12 @@ namespace ctrace::stack::analysis
                         report.neverRuns =
                             pilotAccess && promotedValues().indexValuesAt(access, *idxVal).kind ==
                                                IndexValues::Kind::NeverRuns;
-                        report.aliasPathVec = aliasPath;
-                        report.aliasPath = buildAliasPathString(aliasPath);
+                        std::vector<std::string> path = aliasPath;
+                        if (const auto through = via.find(&access); through != via.end())
+                            path.insert(path.begin(), through->second.begin(),
+                                        through->second.end());
+                        report.aliasPath = buildAliasPathString(path);
+                        report.aliasPathVec = std::move(path);
                         return report;
                     };
 
@@ -1115,19 +1188,15 @@ namespace ctrace::stack::analysis
                         baseIdxVal = cast->getOperand(0);
                     }
 
-                    // 4) Constant index case: test[11]
-                    if (auto* CIdx = dyn_cast<ConstantInt>(idxVal))
+                    // 4) Constant index case, out of bounds: test[11]
+                    if (CIdx)
                     {
-                        auto idxValue = CIdx->getSExtValue();
-                        if (idxValue < 0 || static_cast<StackSize>(idxValue) >= arraySize)
+                        for (Instruction* access : accesses)
                         {
-                            for (Instruction* access : accesses)
-                            {
-                                StackBufferOverflowIssue report = newReport(*access);
-                                report.indexOrUpperBound = static_cast<StackSize>(idxValue);
-                                report.indexIsConstant = true;
-                                out.push_back(std::move(report));
-                            }
+                            StackBufferOverflowIssue report = newReport(*access);
+                            report.indexOrUpperBound = static_cast<StackSize>(CIdx->getSExtValue());
+                            report.indexIsConstant = true;
+                            out.push_back(std::move(report));
                         }
                         continue;
                     }

test/bound-storage/access-through-pointer-variable.c

@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: Apache-2.0
+// An element address kept in a pointer variable is checked where the pointer is dereferenced,
+// with the value the variable holds there.
+
+// The address one past the end is valid; the write through it is not.
+void write_one_past_end(void)
+{
+    char buf[10] = {0};
+    char* p = &buf[10];
+    *p = 42;
+}
+
+// The same address, written as an offset from the array.
+void write_past_end_by_offset(void)
+{
+    char buf[10] = {0};
+    char* p = buf + 10;
+    *p = 42;
+}
+
+// The address goes through a second variable before the read.
+char read_through_copy(void)
+{
+    char buf[10] = {0};
+    char* p = &buf[10];
+    char* q = p;
+    return *q;
+}
+
+// Only the value stored last counts.
+void reassigned_in_bounds(void)
+{
+    char buf[10] = {0};
+    char* p = &buf[10];
+    p = &buf[9];
+    *p = 1;
+}
+
+void reassigned_past_end(void)
+{
+    char buf[10] = {0};
+    char* p = &buf[0];
+    p = &buf[10];
+    *p = 1;
+}
+
+// Each iteration writes through the address it formed: buf[10] on the last one.
+void every_iteration(void)
+{
+    char buf[10] = {0};
+    for (int i = 0; i <= 10; ++i)
+    {
+        char* p = &buf[i];
+        *p = 0;
+    }
+}
+
+// The write only runs once i passes 4, but it still reaches i == 10.
+void guarded_past_end(void)
+{
+    char buf[10] = {0};
+    for (int i = 0; i <= 10; ++i)
+    {
+        char* p = &buf[i];
+        if (i > 4)
+            *p = 0;
+    }
+}
+
+// The write uses the address the previous iteration formed, so it never reaches buf[10]:
+// the index of that iteration is not the current one.
+void previous_iteration(void)
+{
+    char buf[10] = {0};
+    char* p = buf;
+    for (int i = 0; i < 10; ++i)
+    {
+        *p = 0;
+        p = &buf[i + 1];
+    }
+}
+
+// After the loop, the variable holds the address formed by the last iteration: &buf[10].
+void after_last_iteration(void)
+{
+    char buf[10] = {0};
+    char* p = buf;
+    int i = 0;
+    do
+    {
+        p = &buf[i];
+    } while (++i <= 10);
+    *p = 0;
+}
+
+// strict-expectation-details: true
+
+// at line 10, column 8
+// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 10)
+// ↳ alias path: buf -> arrayidx -> p
+// ↳ constant index 10 is out of bounds (0..9)
+// ↳ (this is a write access)
+
+// at line 18, column 8
+// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 10)
+// ↳ alias path: buf -> arraydecay -> add.ptr -> p
+// ↳ constant index 10 is out of bounds (0..9)
+// ↳ (this is a write access)
+
+// at line 27, column 12
+// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 10)
+// ↳ alias path: buf -> arrayidx -> p -> q
+// ↳ constant index 10 is out of bounds (0..9)
+// ↳ (this is a read access)
+
+// at line 44, column 8
+// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 10)
+// ↳ alias path: buf -> arrayidx1 -> p
+// ↳ constant index 10 is out of bounds (0..9)
+// ↳ (this is a write access)
+
+// at line 54, column 12
+// [ !!Warn ] stack buffer overflow on variable 'buf' (size 10)
+// ↳ alias path: buf -> arrayidx -> p
+// ↳ index reaches 10 on every run of the function that returns (array last valid index: 9)
+// ↳ (this is a write access)
+
+// at line 66, column 16
+// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 10)
+// ↳ alias path: buf -> arrayidx -> p
+// ↳ index variable may go up to 10 (array last valid index: 9)
+// ↳ (this is a write access)
+
+// at line 93, column 8
+// [ !!Warn ] stack buffer overflow on variable 'buf' (size 10)
+// ↳ alias path: buf -> arrayidx -> p
+// ↳ index reaches 10 on every run of the function that returns (array last valid index: 9)
+// ↳ (this is a write access)

test/unit/analyzer_module_unit_tests.cpp

@@ -1869,6 +1869,19 @@ define void @stored_address() {
 exit:
   ret void
 }
+define i8 @step_back() {
+entry:
+  %buf = alloca [10 x i8]
+  %end = alloca ptr
+  %past = getelementptr [10 x i8], ptr %buf, i64 0, i64 10
+  store ptr %past, ptr %end
+  %e0 = load ptr, ptr %end
+  %last = getelementptr i8, ptr %e0, i64 -1
+  store ptr %last, ptr %end
+  %e1 = load ptr, ptr %end
+  %v = load i8, ptr %e1
+  ret i8 %v
+}
 )",
                                                                                error, context);
         if (!module)
@@ -1907,6 +1920,9 @@ define void @stored_address() {
                       "below 10 is not reported");
         report.expect(issuesOf("stored_address").empty(),
                       "IndexBoundsPerAccess: storing the address &buf[10] does not access buf");
+        report.expect(issuesOf("step_back").empty(),
+                      "IndexBoundsPerAccess: --end from &buf[10] reads buf[9]; the step from a "
+                      "pointer variable is not an index of buf");
         return report.failures == 0;
     }