From f79fcce10011d504d65b27cc1924a05551ad64f0 Mon Sep 17 00:00:00 2001 From: Hugo Date: Sat, 7 Nov 2026 16:24:20 +0100 Subject: [PATCH] fix(stack-buffer): check accesses through a local pointer to an array element Since a store of an element's address stopped counting as a write, `char* p = &buf[10]; *p = 42;` got no report at all. The access through `p` is now checked where it happens, at `*p = 42`, with `p` in the alias path. Only addresses computed from the array itself are followed (`&buf[i]`, `buf + i`, an array field of a struct): their index counts from the array's start. A load of a variable stands for such an address when its MemorySSA clobber is the store of the address to that variable, the contract tryGetConstFromValue already relies on. That store dominates the load, so the index still holds the value it had when the address was formed, and the pilot and the heuristics judge it at the access, under the conditions the access runs on. A load that may read another store, such as one in the next iteration of a loop or after a branch that assigns the variable, is not followed. Copies from one variable to another are followed the same way. access-through-pointer-variable.c covers a write, a read through a copy, `buf + 10`, a reassignment each way, an access on every iteration, a guarded one, one through the address the previous iteration formed, and one after the loop. A unit test checks that a step back from a pointer variable is not taken for an index of the array. --- CHANGELOG.md | 8 +- src/analysis/StackBufferAnalysis.cpp | 111 +++++++++++--- .../access-through-pointer-variable.c | 138 ++++++++++++++++++ test/unit/analyzer_module_unit_tests.cpp | 16 ++ 4 files changed, 249 insertions(+), 24 deletions(-) create mode 100644 test/bound-storage/access-through-pointer-variable.c diff --git a/CHANGELOG.md b/CHANGELOG.md index a401800..f7ede44 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/analysis/StackBufferAnalysis.cpp b/src/analysis/StackBufferAnalysis.cpp index 56027b8..4073bfb 100644 --- a/src/analysis/StackBufferAnalysis.cpp +++ b/src/analysis/StackBufferAnalysis.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -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& accesses, + std::unordered_map>& via) + { + using namespace llvm; + + SmallVector>, 4> pointers; + pointers.emplace_back(&address, std::vector{}); + 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(user); + if (isa(user) || (store && store->getPointerOperand() == pointer)) + { + accesses.push_back(cast(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(variableUser); + if (!load || !load->isSimple() || load->getType() != pointer->getType()) + continue; + const auto* def = + dyn_cast_or_null(facts.clobberingAccess(*load)); + if (!def || def->getMemoryInst() != store) + continue; + std::vector 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& out, const AnalysisComplexityBudgets& budgets, @@ -1074,15 +1136,22 @@ namespace ctrace::stack::analysis GV->hasName() ? GV->getName().str() : std::string(""); } - // 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(idxVal); + if (CIdx && CIdx->getSExtValue() >= 0 && + static_cast(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 accesses; - for (User* GU : GEP->users()) - { - const auto* store = dyn_cast(GU); - if (isa(GU) || (store && store->getPointerOperand() == GEP)) - accesses.push_back(cast(GU)); - } + std::unordered_map> via; + const Value* array = AI ? static_cast(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 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(idxVal)) + // 4) Constant index case, out of bounds: test[11] + if (CIdx) { - auto idxValue = CIdx->getSExtValue(); - if (idxValue < 0 || static_cast(idxValue) >= arraySize) + for (Instruction* access : accesses) { - for (Instruction* access : accesses) - { - StackBufferOverflowIssue report = newReport(*access); - report.indexOrUpperBound = static_cast(idxValue); - report.indexIsConstant = true; - out.push_back(std::move(report)); - } + StackBufferOverflowIssue report = newReport(*access); + report.indexOrUpperBound = static_cast(CIdx->getSExtValue()); + report.indexIsConstant = true; + out.push_back(std::move(report)); } continue; } diff --git a/test/bound-storage/access-through-pointer-variable.c b/test/bound-storage/access-through-pointer-variable.c new file mode 100644 index 0000000..3ec6f45 --- /dev/null +++ b/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) diff --git a/test/unit/analyzer_module_unit_tests.cpp b/test/unit/analyzer_module_unit_tests.cpp index c083231..a631e8d 100644 --- a/test/unit/analyzer_module_unit_tests.cpp +++ b/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; }