diff --git a/CHANGELOG.md b/CHANGELOG.md index ac39bb97..36ea3489 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ Notable changes are grouped by release. See [all releases](https://github.com/Co ## Unreleased +- 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 + store after the read, a store on one branch only, or a store of another width changed the + result then, and `(unsigned char)300` read as 300. The note now also requires every edge into + the access to be dead. It no longer appears on accesses that run, such as an access after a + loop. - Building requires LLVM 20. CMake rejects an older LLVM when it configures the build, where LLVM 19 passed that check and then failed to compile. On macOS, `scripts/build.sh` looks for Homebrew `llvm@20` before `llvm`, a later version, and no longer for `llvm@19` or `llvm@18`. diff --git a/include/analysis/IRValueUtils.hpp b/include/analysis/IRValueUtils.hpp index 23ee3ab7..493066e9 100644 --- a/include/analysis/IRValueUtils.hpp +++ b/include/analysis/IRValueUtils.hpp @@ -16,6 +16,8 @@ namespace llvm namespace ctrace::stack::analysis { + class FunctionFacts; + enum class LeadingUnderscores { One, @@ -48,7 +50,13 @@ namespace ctrace::stack::analysis AllocaOrigin classifyAllocaOrigin(const llvm::AllocaInst* AI); - const llvm::ConstantInt* tryGetConstFromValue(const llvm::Value* V, const llvm::Function& F); + /// The integer constant @p V holds wherever it is used, or null when that is not established. + /// + /// Follows casts, applying them to the constant, and loads whose MemorySSA clobber is a + /// store of the same type to the same pointer, so the store wrote exactly the bytes the load + /// reads. Any other clobber, such as a merge of several stores, a store that may only + /// overlap, a call or the function entry, gives null. + const llvm::ConstantInt* tryGetConstFromValue(const llvm::Value* V, const FunctionFacts& facts); /// The sole store to a slot whose other uses are loads, debug or lifetime intrinsics. /// Returns null for multiple stores, address escapes or unsupported uses. diff --git a/src/analysis/AllocaUsage.cpp b/src/analysis/AllocaUsage.cpp index dba0a07e..bfc6a188 100644 --- a/src/analysis/AllocaUsage.cpp +++ b/src/analysis/AllocaUsage.cpp @@ -171,7 +171,7 @@ namespace ctrace::stack::analysis issue.sizeIsConst = true; issue.sizeBytes = C->getZExtValue() * elemSize; } - else if (auto* C = tryGetConstFromValue(arraySizeVal, F)) + else if (auto* C = tryGetConstFromValue(arraySizeVal, facts)) { issue.sizeIsConst = true; issue.sizeBytes = C->getZExtValue() * elemSize; diff --git a/src/analysis/DynamicAlloca.cpp b/src/analysis/DynamicAlloca.cpp index 137b6406..86762769 100644 --- a/src/analysis/DynamicAlloca.cpp +++ b/src/analysis/DynamicAlloca.cpp @@ -8,8 +8,11 @@ #include #include +#include "analysis/FunctionFacts.hpp" #include "analysis/IRValueUtils.hpp" +#include + namespace ctrace::stack::analysis { namespace @@ -22,6 +25,9 @@ namespace ctrace::stack::analysis if (F.isDeclaration()) return; + // Built on the first non-constant size only: most functions have no VLA. + std::optional facts; + for (BasicBlock& BB : F) { for (Instruction& I : BB) @@ -40,15 +46,15 @@ namespace ctrace::stack::analysis if (llvm::isa(arraySizeVal)) continue; // compile-time known size, OK - // 2) "Smart" heuristic: try to trace back to a constant - // via stores into a local variable (tryGetConstFromValue). - // Typical example: + // 2) The size is a constant stored into a local variable, read back + // unchanged (tryGetConstFromValue). Typical example: // int n = 6; // char buf[n]; // in C: VLA, but here n is actually constant // // In this case we don't want to spam with a VLA warning: // treat it as an effectively constant size. - if (tryGetConstFromValue(arraySizeVal, F) != nullptr) + if (tryGetConstFromValue(arraySizeVal, facts ? *facts : facts.emplace(F)) != + nullptr) continue; // 3) Here we consider it a real VLA / dynamic alloca @@ -97,6 +103,7 @@ namespace ctrace::stack::analysis { std::vector out; using namespace llvm; + std::optional facts; for (const AllocaInst* AI : allocas) { @@ -105,7 +112,10 @@ namespace ctrace::stack::analysis if (llvm::isa(arraySizeVal)) continue; - if (tryGetConstFromValue(arraySizeVal, function) != nullptr) + // FunctionFacts needs a mutable function for its analyses; they do not change it. + if (!facts) + facts.emplace(const_cast(function)); + if (tryGetConstFromValue(arraySizeVal, *facts) != nullptr) continue; DynamicAllocaIssue issue; diff --git a/src/analysis/IRValueUtils.cpp b/src/analysis/IRValueUtils.cpp index c2d76f27..ec46cd29 100644 --- a/src/analysis/IRValueUtils.cpp +++ b/src/analysis/IRValueUtils.cpp @@ -1,8 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 #include "analysis/IRValueUtils.hpp" +#include "analysis/FunctionFacts.hpp" + #include #include +#include +#include #include #include #include @@ -299,46 +303,50 @@ namespace ctrace::stack::analysis return AllocaOrigin::Unknown; } - const llvm::ConstantInt* tryGetConstFromValue(const llvm::Value* V, const llvm::Function& F) + namespace { - using namespace llvm; + /// Bounds the walk; -O0 copy chains such as `int m = n;` need a few levels, and + /// unreachable code may hold a cycle of casts. + constexpr unsigned kMaxConstResolutionDepth = 16; - // First remove casts (sext/zext/trunc, etc.) to reach the real base value. - const Value* cur = V; - while (auto* cast = dyn_cast(cur)) + const llvm::ConstantInt* resolveConstant(const llvm::Value* value, + const FunctionFacts& facts, unsigned depth) { - cur = cast->getOperand(0); - } + using namespace llvm; - // Trivial case: already an integer constant. - if (auto* C = dyn_cast(cur)) - return C; - - // Typical -O0 case: comparing a load from a local variable. - auto* LI = dyn_cast(cur); - if (!LI) - return nullptr; + if (const auto* constant = dyn_cast(value)) + return constant; + if (depth == 0) + return nullptr; - const Value* ptr = LI->getPointerOperand(); - const ConstantInt* found = nullptr; + if (const auto* cast = dyn_cast(value)) + { + const ConstantInt* operand = resolveConstant(cast->getOperand(0), facts, depth - 1); + if (!operand) + return nullptr; + return dyn_cast_or_null( + ConstantFoldCastOperand(cast->getOpcode(), const_cast(operand), + cast->getType(), cast->getDataLayout())); + } - // Ultra-simple version: look for a constant store in the function. - for (const BasicBlock& BB : F) - { - for (const Instruction& I : BB) + const auto* load = dyn_cast(value); + if (!load || !load->isSimple()) + return nullptr; + const auto* def = dyn_cast_or_null(facts.clobberingAccess(*load)); + const auto* store = def ? dyn_cast_or_null(def->getMemoryInst()) : nullptr; + if (!store || !store->isSimple() || + store->getPointerOperand()->stripPointerCasts() != + load->getPointerOperand()->stripPointerCasts() || + store->getValueOperand()->getType() != load->getType()) { - auto* SI = dyn_cast(&I); - if (!SI) - continue; - if (SI->getPointerOperand() != ptr) - continue; - if (auto* C = dyn_cast(SI->getValueOperand())) - { - // Keep the last constant found (if multiple stores, this is naive). - found = C; - } + return nullptr; } + return resolveConstant(store->getValueOperand(), facts, depth - 1); } - return found; + } // namespace + + const llvm::ConstantInt* tryGetConstFromValue(const llvm::Value* V, const FunctionFacts& facts) + { + return V ? resolveConstant(V, facts, kMaxConstResolutionDepth) : nullptr; } } // namespace ctrace::stack::analysis diff --git a/src/analysis/Reachability.cpp b/src/analysis/Reachability.cpp index 6c394cab..5bbff3e3 100644 --- a/src/analysis/Reachability.cpp +++ b/src/analysis/Reachability.cpp @@ -1,8 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 #include "analysis/Reachability.hpp" +#include "analysis/FunctionFacts.hpp" #include "analysis/IRValueUtils.hpp" +#include + #include #include #include @@ -13,78 +16,38 @@ namespace ctrace::stack::analysis bool isStaticallyUnreachableStackAccess(const StackBufferOverflowIssue& issue) { - if (!issue.inst) - return false; + using namespace llvm; - auto* block = issue.inst->getParent(); - if (!block) + const BasicBlock* block = issue.inst ? issue.inst->getParent() : nullptr; + if (!block || pred_empty(block)) return false; - using namespace llvm; - - for (auto* predecessor : predecessors(block)) + std::optional facts; + // Unreachable only when no edge into the block can be taken. + for (const BasicBlock* predecessor : predecessors(block)) { - auto* branch = dyn_cast(predecessor->getTerminator()); - if (!branch || !branch->isConditional()) - continue; - - auto* compare = dyn_cast(branch->getCondition()); + const auto* branch = dyn_cast(predecessor->getTerminator()); + const auto* compare = branch && branch->isConditional() + ? dyn_cast(branch->getCondition()) + : nullptr; if (!compare) - continue; + return false; - const llvm::Function& function = *issue.inst->getFunction(); - auto* lhs = analysis::tryGetConstFromValue(compare->getOperand(0), function); - auto* rhs = analysis::tryGetConstFromValue(compare->getOperand(1), function); + // FunctionFacts needs a mutable function for its analyses; they do not change it. + if (!facts) + facts.emplace(const_cast(*block->getParent())); + const ConstantInt* lhs = tryGetConstFromValue(compare->getOperand(0), *facts); + const ConstantInt* rhs = tryGetConstFromValue(compare->getOperand(1), *facts); if (!lhs || !rhs) - continue; - - bool condTrue = false; - const auto& lhsValue = lhs->getValue(); - const auto& rhsValue = rhs->getValue(); - - switch (compare->getPredicate()) - { - case ICmpInst::ICMP_EQ: - condTrue = (lhsValue == rhsValue); - break; - case ICmpInst::ICMP_NE: - condTrue = (lhsValue != rhsValue); - break; - case ICmpInst::ICMP_SLT: - condTrue = lhsValue.slt(rhsValue); - break; - case ICmpInst::ICMP_SLE: - condTrue = lhsValue.sle(rhsValue); - break; - case ICmpInst::ICMP_SGT: - condTrue = lhsValue.sgt(rhsValue); - break; - case ICmpInst::ICMP_SGE: - condTrue = lhsValue.sge(rhsValue); - break; - case ICmpInst::ICMP_ULT: - condTrue = lhsValue.ult(rhsValue); - break; - case ICmpInst::ICMP_ULE: - condTrue = lhsValue.ule(rhsValue); - break; - case ICmpInst::ICMP_UGT: - condTrue = lhsValue.ugt(rhsValue); - break; - case ICmpInst::ICMP_UGE: - condTrue = lhsValue.uge(rhsValue); - break; - default: - continue; - } + return false; - if (block == branch->getSuccessor(0) && !condTrue) - return true; - if (block == branch->getSuccessor(1) && condTrue) - return true; + const bool holds = + ICmpInst::compare(lhs->getValue(), rhs->getValue(), compare->getPredicate()); + if (branch->getSuccessor(holds ? 0 : 1) == block) + return false; } - return false; + return true; } } // namespace ctrace::stack::analysis diff --git a/src/analysis/StackComputation.cpp b/src/analysis/StackComputation.cpp index 62972eac..67599f97 100644 --- a/src/analysis/StackComputation.cpp +++ b/src/analysis/StackComputation.cpp @@ -22,6 +22,7 @@ #include #include "analysis/AnalyzerUtils.hpp" +#include "analysis/FunctionFacts.hpp" #include "analysis/IntRanges.hpp" #include "analysis/IRValueUtils.hpp" #include "analysis/smt/SmtEncoding.hpp" @@ -90,6 +91,8 @@ namespace ctrace::stack::analysis const CallResolver* resolver) { LocalStackInfo info; + // Built on the first non-constant array size only: most functions have none. + std::optional facts; for (llvm::BasicBlock& BB : F) { @@ -113,7 +116,8 @@ namespace ctrace::stack::analysis { count = CI->getZExtValue(); } - else if (auto* C = analysis::tryGetConstFromValue(alloca->getArraySize(), F)) + else if (auto* C = analysis::tryGetConstFromValue( + alloca->getArraySize(), facts ? *facts : facts.emplace(F))) { count = C->getZExtValue(); } diff --git a/test/bound-storage/global-array-overflow.c b/test/bound-storage/global-array-overflow.c index 92fbf690..92dc40fb 100644 --- a/test/bound-storage/global-array-overflow.c +++ b/test/bound-storage/global-array-overflow.c @@ -14,8 +14,10 @@ int main(void) // ↳ alias path: gbuf // ↳ index variable may go up to 10 (array last valid index: 9) // ↳ (this is a write access) - // ↳ [info] this access appears unreachable at runtime (condition is always false for this branch) gbuf[i] = i; return gbuf[0]; } + +// The access runs after the loop with i == 10, so it is reachable. +// not contains: [info] this access appears unreachable at runtime (condition is always false for this branch) diff --git a/test/bound-storage/unreachable-branch-dependent-store.c b/test/bound-storage/unreachable-branch-dependent-store.c new file mode 100644 index 00000000..d46eee1a --- /dev/null +++ b/test/bound-storage/unreachable-branch-dependent-store.c @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: Apache-2.0 +// i is 0 or 11 depending on c: the condition is not constant, so the access may run. +void unreachable_branch_dependent_store(int c) +{ + int i = 0; + char test[10]; + + if (c) + i = 11; + if (i <= 10) + test[11] = 'a'; +} + +// not contains: [info] this access appears unreachable at runtime (condition is always false for this branch) + +// at line 11, column 18 +// [ !!Warn ] potential stack buffer overflow on variable 'test' (size 10) +// ↳ alias path: test +// ↳ constant index 11 is out of bounds (0..9) +// ↳ (this is a write access) diff --git a/test/bound-storage/unreachable-later-store-is-not-read.c b/test/bound-storage/unreachable-later-store-is-not-read.c new file mode 100644 index 00000000..23746adb --- /dev/null +++ b/test/bound-storage/unreachable-later-store-is-not-read.c @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: Apache-2.0 +// The condition reads i before i = 11: it holds, so the access is reachable. +char unreachable_later_store_is_not_read(void) +{ + int i = 5; + char test[10]; + + if (i <= 10) + test[11] = 'a'; + i = 11; + return (char)i; +} + +// not contains: [info] this access appears unreachable at runtime (condition is always false for this branch) + +// at line 9, column 18 +// [ !!Warn ] potential stack buffer overflow on variable 'test' (size 10) +// ↳ alias path: test +// ↳ constant index 11 is out of bounds (0..9) +// ↳ (this is a write access) diff --git a/test/bound-storage/unreachable-multi-pred-conditional.c b/test/bound-storage/unreachable-multi-pred-conditional.c new file mode 100644 index 00000000..ea2ff5ba --- /dev/null +++ b/test/bound-storage/unreachable-multi-pred-conditional.c @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: Apache-2.0 +// The `zero` edge into the access is dead, but the `x > 0` edge is not. +void unreachable_multi_pred_conditional(int x) +{ + int zero = 0; + char test[10]; + + if (x > 0 || zero) + test[11] = 'a'; +} + +// not contains: [info] this access appears unreachable at runtime (condition is always false for this branch) + +// at line 9, column 18 +// [ !!Warn ] potential stack buffer overflow on variable 'test' (size 10) +// ↳ alias path: test +// ↳ constant index 11 is out of bounds (0..9) +// ↳ (this is a write access) diff --git a/test/bound-storage/unreachable-truncated-constant.c b/test/bound-storage/unreachable-truncated-constant.c new file mode 100644 index 00000000..d9b92e6d --- /dev/null +++ b/test/bound-storage/unreachable-truncated-constant.c @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: Apache-2.0 +// (unsigned char)300 is 44: the condition holds, so the access is reachable. +void unreachable_truncated_constant(void) +{ + int big = 300; + char test[10]; + + if ((unsigned char)big == 44) + test[11] = 'a'; +} + +// not contains: [info] this access appears unreachable at runtime (condition is always false for this branch) + +// at line 9, column 18 +// [ !!Warn ] potential stack buffer overflow on variable 'test' (size 10) +// ↳ alias path: test +// ↳ constant index 11 is out of bounds (0..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 698b9615..003ead88 100644 --- a/test/unit/analyzer_module_unit_tests.cpp +++ b/test/unit/analyzer_module_unit_tests.cpp @@ -9,6 +9,7 @@ #include "analysis/InputPipeline.hpp" #include "analysis/InputPipelineRevision.hpp" #include "analysis/IntRanges.hpp" +#include "analysis/IRValueUtils.hpp" #include "analysis/ParameterDebugBinding.hpp" #include "analysis/Reachability.hpp" #include "analysis/ResourceLifetimeAnalysis.hpp" @@ -1580,6 +1581,117 @@ namespace return report.failures == 0; } + /// tryGetConstFromValue: a load is constant only when its clobber stored exactly what it reads. + bool testTryGetConstFromValue(TestReport& report) + { + using ctrace::stack::analysis::FunctionFacts; + using ctrace::stack::analysis::tryGetConstFromValue; + llvm::LLVMContext context; + llvm::SMDiagnostic error; + const std::unique_ptr module = llvm::parseAssemblyString(R"( +@g = global i32 0 +define i32 @exact() { + %p = alloca i32 + store i32 7, ptr %p + %v = load i32, ptr %p + ret i32 %v +} +define i8 @truncated() { + %p = alloca i32 + store i32 300, ptr %p + %v = load i32, ptr %p + %t = trunc i32 %v to i8 + ret i8 %t +} +define i64 @sign_extended() { + %p = alloca i8 + store i8 -1, ptr %p + %v = load i8, ptr %p + %s = sext i8 %v to i64 + ret i64 %s +} +define i32 @later_store() { + %p = alloca i32 + store i32 5, ptr %p + %v = load i32, ptr %p + store i32 11, ptr %p + ret i32 %v +} +define i8 @narrower_load() { + %p = alloca i32 + store i32 300, ptr %p + %v = load i8, ptr %p + ret i8 %v +} +define i32 @merged(i1 %c) { +entry: + %p = alloca i32 + store i32 0, ptr %p + br i1 %c, label %then, label %join +then: + store i32 11, ptr %p + br label %join +join: + %v = load i32, ptr %p + ret i32 %v +} +define i32 @may_alias(ptr %q) { + store i32 7, ptr @g + store i32 9, ptr %q + %v = load i32, ptr @g + ret i32 %v +} +define i32 @volatile_load() { + %p = alloca i32 + store i32 7, ptr %p + %v = load volatile i32, ptr %p + ret i32 %v +} +define i32 @entry_value(ptr %q) { + %v = load i32, ptr %q + ret i32 %v +} +)", + error, context); + if (!module) + { + report.expect(false, "tryGetConstFromValue setup: the IR does not parse"); + return false; + } + + // The constant the function returns, as tryGetConstFromValue sees it. + const auto resolve = [&](const char* name) -> const llvm::ConstantInt* + { + llvm::Function* fn = module->getFunction(name); + const auto* ret = llvm::cast(fn->back().getTerminator()); + const FunctionFacts facts(*fn); + return tryGetConstFromValue(ret->getReturnValue(), facts); + }; + const auto holds = [&](const char* name, std::int64_t expected, unsigned width) + { + const llvm::ConstantInt* constant = resolve(name); + return constant && constant->getBitWidth() == width && + constant->getSExtValue() == expected; + }; + + report.expect(holds("exact", 7, 32), "tryGetConstFromValue: a store read back unchanged"); + report.expect(holds("truncated", 44, 8), "tryGetConstFromValue: trunc of 300 to i8 is 44"); + report.expect(holds("sign_extended", -1, 64), "tryGetConstFromValue: sext of i8 -1 is -1"); + report.expect(holds("later_store", 5, 32), + "tryGetConstFromValue: a store after the load is not what it reads"); + report.expect(!resolve("narrower_load"), + "tryGetConstFromValue: a load narrower than the store is unknown"); + report.expect(!resolve("merged"), + "tryGetConstFromValue: stores merged from two paths are unknown"); + report.expect(!resolve("may_alias"), + "tryGetConstFromValue: a store that may overlap the slot is unknown"); + report.expect(!resolve("volatile_load"), + "tryGetConstFromValue: a volatile load is unknown"); + report.expect(!resolve("entry_value"), + "tryGetConstFromValue: memory as it was on entry is unknown"); + return report.failures == 0; + } + using ctrace::stack::analysis::smt::ConstraintIR; using ctrace::stack::analysis::smt::ExprId; using ctrace::stack::analysis::smt::ExprKind; @@ -2914,6 +3026,7 @@ int main(int argc, char** argv) (void)testProgramPointRangesUnsignedReadings(repoRoot, report); (void)testSmtEvaluatorEncodesLazily(report); (void)testFunctionFactsClobberingAccess(repoRoot, report); + (void)testTryGetConstFromValue(report); (void)testSmtEncoderMemoryModel(repoRoot, report); (void)testSmtEncoderPathCondition(repoRoot, report); #ifdef CTRACE_STACK_ENABLE_Z3_BACKEND diff --git a/test/vla/vla-size-assigned-on-one-branch.c b/test/vla/vla-size-assigned-on-one-branch.c new file mode 100644 index 00000000..56a7b942 --- /dev/null +++ b/test/vla/vla-size-assigned-on-one-branch.c @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: Apache-2.0 +// n is 4 or 4096 depending on c: the size is not one compile-time constant. +char vla_size_assigned_on_one_branch(int c) +{ + int n = 4; + if (c) + n = 4096; + char buf[n]; + buf[0] = 0; + return buf[0]; +} + +// not contains: requested stack size: 4096 bytes + +// at line 8, column 5 +// [ !!Warn ] dynamic stack allocation detected for variable 'vla' +// ↳ allocated type: i8 +// ↳ size of this allocation is not compile-time constant (VLA / variable alloca) and may lead to unbounded stack usage + +// at line 8, column 5 +// [ !!Warn ] dynamic alloca on the stack for variable 'vla' +// ↳ allocation performed via alloca/VLA; stack usage grows with runtime value +// ↳ size is unbounded at compile time +// ↳ size does not appear user-controlled but remains runtime-dependent diff --git a/test/vla/vla-size-later-store-is-not-read.c b/test/vla/vla-size-later-store-is-not-read.c new file mode 100644 index 00000000..3a9e02df --- /dev/null +++ b/test/vla/vla-size-later-store-is-not-read.c @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: Apache-2.0 +// The VLA reads n before n = 100: its size is 6 bytes. +char vla_size_later_store_is_not_read(void) +{ + int n = 6; + char buf[n]; + buf[0] = 0; + n = 100; + return buf[0] + (char)n; +} + +// strict-expectation-details: true + +// not contains: requested stack size: 100 bytes + +// at line 6, column 5 +// [ !!Warn ] dynamic alloca on the stack for variable 'vla' +// ↳ allocation performed via alloca/VLA; stack usage grows with runtime value +// ↳ requested stack size: 6 bytes +// ↳ size does not appear user-controlled but remains runtime-dependent