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; }