diff --git a/CHANGELOG.md b/CHANGELOG.md index 50c326b..ad32fea 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 +- StackBufferOverflow and MultipleStoresToStackBuffer resolve each pointer variable to its + stack array once per function, by strongly connected components: pointer variables + assigned from one another no longer make the analysis walk every path between them, + which never finished on zlib's `crc32.c` and cppcheck's `simplecpp.cpp` (#215). Pointers + assigned from one another in a cycle that reaches two different arrays now resolve to no + array, as any pointer with several possible arrays does, where each used to be reported + against its own array. - Added an authors file and linked the repository documents from the README. - SARIF rules carry a title (`shortDescription`) and a description (`fullDescription`) instead of their id, and `describeRule` (`RuleCatalog.hpp`) gives them to programs linking the analyzer. diff --git a/src/analysis/StackBufferAnalysis.cpp b/src/analysis/StackBufferAnalysis.cpp index 2bf7841..b07b96c 100644 --- a/src/analysis/StackBufferAnalysis.cpp +++ b/src/analysis/StackBufferAnalysis.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -33,21 +34,6 @@ namespace ctrace::stack::analysis { using SmtFeasibility = smt::SmtFeasibility; - struct RecursionGuard - { - llvm::SmallPtrSetImpl& set; - const llvm::Value* value; - RecursionGuard(llvm::SmallPtrSetImpl& s, const llvm::Value* v) - : set(s), value(v) - { - set.insert(value); - } - ~RecursionGuard() - { - set.erase(value); - } - }; - struct AnalysisComplexityBudgets { std::size_t pointerStoreScanInstrThreshold = 0; @@ -224,159 +210,319 @@ namespace ctrace::stack::analysis return count; } - static const llvm::AllocaInst* resolveArrayAllocaFromPointerInternal( - const llvm::Value* V, llvm::Function& F, std::vector& path, - llvm::SmallPtrSetImpl& recursionStack, int depth, - bool allowPointerStoreScan) + /// Resolves a pointer to the stack array it indexes, through the function's local + /// pointer variables (`char* p = buf; p[i]`) and pointer PHIs. One instance serves one + /// function: it indexes the stores into each alloca once, and resolves every pointer + /// variable and PHI once, by strongly connected components, so that a node's result + /// depends on the function alone and not on the query that reached it (#215). The work is + /// proportional to the function's pointer variables, PHIs and stores: each is visited + /// once by the walk and once more to settle its alias path witness. A query then + /// copies its alias path once. + class ArrayAllocaResolver { - using namespace llvm; - - if (!V) - return nullptr; - if (depth > 64) - return nullptr; - if (recursionStack.contains(V)) - return nullptr; + public: + ArrayAllocaResolver(const llvm::Function& F, bool allowPointerStoreScan) + { + using namespace llvm; + // Without the scan, a pointer variable has no known source and resolves to nothing. + if (!allowPointerStoreScan) + return; + for (const BasicBlock& BB : F) + { + for (const Instruction& I : BB) + { + const auto* SI = dyn_cast(&I); + if (!SI) + continue; + if (const auto* AI = dyn_cast(SI->getPointerOperand())) + storedValues_[AI].push_back(SI->getValueOperand()); + } + } + } - RecursionGuard guard(recursionStack, V); + /// The array alloca `V` points into, or nullptr when none is known or several are. + /// Appends to `path` the names met on the way, the array's last. + const llvm::AllocaInst* resolve(const llvm::Value* V, std::vector& path) + { + const Target target = walk(V); + path.insert(path.end(), target.names.begin(), target.names.end()); + if (!target.node) + return target.array; + + resolveFrom(target.node); + const Node* node = &nodes_[target.node]; + if (!node->alloca) + return nullptr; + for (;; node = &nodes_[node->next]) + { + path.insert(path.end(), node->segment.begin(), node->segment.end()); + if (!node->next) + return node->alloca; + } + } - auto isArrayAlloca = [](const AllocaInst* AI) -> bool + private: + // Where following casts, GEPs and loads from a value ends: a stack array, a node (a + // local pointer variable or a PHI), or neither when the heuristic does not follow. + struct Target { - // Consider a "stack buffer" as: - // - real arrays, - // - array-typed allocas (VLA in IR), - // - structs containing array fields. - if (AI->isArrayAllocation()) - return true; - return isArrayBackedType(AI->getAllocatedType()); + const llvm::AllocaInst* array = nullptr; + const llvm::Value* node = nullptr; + std::vector names; // the names met on the way, the target's last }; - // Avoid weird aliasing loops - SmallPtrSet visited; - const Value* cur = V; + struct Node + { + const llvm::AllocaInst* alloca = nullptr; // the result, once done + // The witness of the result, for the alias path: the names from the chosen + // source to its target, and that target when it is a node. + const llvm::Value* next = nullptr; + std::vector segment; + std::vector targets; // what each source leads to, until done + std::uint64_t index : 31 = 0; // Tarjan's discovery index, 0 until visited + std::uint64_t low : 31 = 0; + std::uint64_t onStack : 1 = 0; + std::uint64_t done : 1 = 0; + }; - while (cur && !visited.contains(cur)) + Target walk(const llvm::Value* V) const { - visited.insert(cur); - if (cur->hasName()) - path.push_back(cur->getName().str()); + using namespace llvm; - // Case 1: we hit an alloca. - if (auto* AI = dyn_cast(cur)) + Target target; + SmallPtrSet visited; + const Value* cur = V; + while (cur && visited.insert(cur).second) { - if (isArrayAlloca(AI)) + if (cur->hasName()) + target.names.push_back(cur->getName().str()); + + if (const auto* AI = dyn_cast(cur)) { - // Stack buffer alloca (array): final target. - return AI; + // A stack buffer: a real array, an array alloca (VLA in IR) or a struct + // with an array field. Anything else is a local pointer variable + // (char* ptr; char** pp; ...): what is stored into it says where it points. + if (AI->isArrayAllocation() || isArrayBackedType(AI->getAllocatedType())) + target.array = AI; + else + target.node = AI; + return target; } - - // Otherwise, it's very likely a local pointer variable - // (char *ptr; char **pp; etc.). Walk stores into this variable - // to see what values get assigned, and try to trace back to - // a real array alloca. - if (!allowPointerStoreScan) - return nullptr; - const AllocaInst* foundAI = nullptr; - - for (BasicBlock& BB : F) + if (const auto* BC = dyn_cast(cur)) { - for (Instruction& I : BB) - { - auto* SI = dyn_cast(&I); - if (!SI) - continue; - if (SI->getPointerOperand() != AI) - continue; - - const Value* storedPtr = SI->getValueOperand(); - std::vector subPath; - const AllocaInst* cand = resolveArrayAllocaFromPointerInternal( - storedPtr, F, subPath, recursionStack, depth + 1, - allowPointerStoreScan); - if (!cand) - continue; - - if (!foundAI) - { - foundAI = cand; - // Append subPath to path - path.insert(path.end(), subPath.begin(), subPath.end()); - } - else if (foundAI != cand) - { - // Multiple different bases: ambiguous aliasing, - // prefer to stop rather than be wrong. - return nullptr; - } - } + cur = BC->getOperand(0); + continue; + } + if (const auto* GEP = dyn_cast(cur)) + { + cur = GEP->getPointerOperand(); + continue; + } + // A load of a pointer: walk up to its container (char* ptr = buf; (*pp)[i]). + if (const auto* LI = dyn_cast(cur)) + { + cur = LI->getPointerOperand(); + continue; + } + if (isa(cur)) + { + target.node = cur; + return target; } - return foundAI; + // Arguments, globals, calls: the heuristic stops here. + break; } + return target; + } - // Case 2: bitcast -> follow the operand. - if (auto* BC = dyn_cast(cur)) + // The values a node takes: those stored into the variable, or the PHI's incoming values. + llvm::SmallVector sourcesOf(const llvm::Value* node) const + { + using namespace llvm; + + SmallVector sources; + if (const auto* PN = dyn_cast(node)) { - cur = BC->getOperand(0); - continue; + for (const Use& incoming : PN->incoming_values()) + sources.push_back(incoming.get()); } - - // Case 3: GEP -> follow the base pointer. - if (auto* GEP = dyn_cast(cur)) + else if (auto stored = storedValues_.find(cast(node)); + stored != storedValues_.end()) { - cur = GEP->getPointerOperand(); - continue; + sources = stored->second; } + return sources; + } + + // Resolves `start` and every node it reaches, with Tarjan's algorithm: a strongly + // connected component is finished only once every component it reaches is, so its + // result is computed from final results and is final itself, whatever the query + // that started the walk. Iterative, so that a long chain of pointer variables costs + // no stack. + void resolveFrom(const llvm::Value* start) + { + using namespace llvm; + + if (nodes_[start].index != 0) + return; // done: no query starts while a walk is in progress - // Case 4: load of a pointer. Typical example: - // char *ptr = test; - // char *p2 = ptr; - // char **pp = &ptr; - // (*pp)[i] = ... - // - // Walk up to the pointer "container" (local variable, or other value) - // by following the load operand. - if (auto* LI = dyn_cast(cur)) + struct Frame { - cur = LI->getPointerOperand(); - continue; + const Value* node; + std::size_t nextTarget; + }; + SmallVector frames; + SmallVector component; // Tarjan's stack + const auto enter = [&](const Value* value) + { + Node& node = nodes_[value]; + node.index = node.low = ++counter_; + node.onStack = 1; + for (const Value* source : sourcesOf(value)) + node.targets.push_back(walk(source)); + component.push_back(value); + frames.push_back({value, 0}); + }; + + enter(start); + while (!frames.empty()) + { + Frame& frame = frames.back(); + Node& node = nodes_[frame.node]; + if (frame.nextTarget < node.targets.size()) + { + const Value* next = node.targets[frame.nextTarget++].node; + if (!next) + continue; + const Node& child = nodes_[next]; + if (child.index == 0) + enter(next); + else if (child.onStack) + node.low = std::min(node.low, child.index); + continue; + } + + if (node.low == node.index) + finishComponent(component, frame.node); + const std::uint64_t low = node.low; + frames.pop_back(); + if (!frames.empty()) + { + Node& parent = nodes_[frames.back().node]; + parent.low = std::min(parent.low, low); + } } + } + + // Pops the component rooted at `root` and gives every member its result: the one + // array the members' sources lead to, directly or through a finished node. A source + // that leads to nothing is ignored; two different arrays make the component ambiguous. + void finishComponent(llvm::SmallVectorImpl& stack, + const llvm::Value* root) + { + using namespace llvm; + + SmallVector members; + do + { + members.push_back(stack.back()); + stack.pop_back(); + } while (members.back() != root); - // Case 5: PHI of pointers (merge of aliases): - // try to resolve each incoming and ensure they - // all point to the same array alloca. - if (auto* PN = dyn_cast(cur)) + // A member of this component is not done yet: its sources speak for it. + const auto arrayOf = [&](const Target& target) -> const AllocaInst* + { + if (target.array || !target.node) + return target.array; + const Node& child = nodes_[target.node]; + return child.done ? child.alloca : nullptr; + }; + + const AllocaInst* found = nullptr; + bool ambiguous = false; + for (const Value* member : members) { - const AllocaInst* foundAI = nullptr; - std::vector phiPath; - for (unsigned i = 0; i < PN->getNumIncomingValues(); ++i) + for (const Target& target : nodes_[member].targets) { - const Value* inV = PN->getIncomingValue(i); - std::vector subPath; - const AllocaInst* cand = resolveArrayAllocaFromPointerInternal( - inV, F, subPath, recursionStack, depth + 1, allowPointerStoreScan); - if (!cand) + const AllocaInst* array = arrayOf(target); + if (!array) continue; - if (!foundAI) + if (!found) + found = array; + else if (found != array) + ambiguous = true; + } + } + if (ambiguous) + found = nullptr; + + // Witnesses, for the alias path. Level 0: the members with a source that leads + // to the array directly or through a node outside the component. Level k + 1: + // the members not settled yet with a source that leads to a member of level k. + // A member takes its first source among those settled at a lower level, so the + // path depends neither on the member order nor on the query. Each member is + // settled once and each edge between members is followed once. + std::unordered_map> dependents; + SmallVector level; + if (found) + { + for (const Value* member : members) + { + bool direct = false; + for (const Target& target : nodes_[member].targets) { - foundAI = cand; - phiPath = subPath; + if (target.node && nodes_[target.node].onStack) + dependents[target.node].push_back(member); + else if (arrayOf(target) == found) + direct = true; } - else if (foundAI != cand) + if (direct) + level.push_back(member); + } + } + SmallPtrSet queued(level.begin(), level.end()); + while (!level.empty()) + { + for (const Value* member : level) + { + Node& node = nodes_[member]; + for (const Target& target : node.targets) { - // PHI mixes multiple different bases: too ambiguous. - return nullptr; + if (arrayOf(target) != found) + continue; + node.segment = target.names; + node.next = target.node; + break; } } - path.insert(path.end(), phiPath.begin(), phiPath.end()); - return foundAI; + SmallVector next; + for (const Value* member : level) + { + nodes_[member].alloca = found; + nodes_[member].done = 1; + for (const Value* dependent : dependents[member]) + { + if (queued.insert(dependent).second) + next.push_back(dependent); + } + } + level = std::move(next); } - // Other cases (arguments, complex globals, etc.): stop the heuristic. - break; + for (const Value* member : members) + { + Node& node = nodes_[member]; + node.onStack = 0; + node.done = 1; + node.targets = {}; + } } - return nullptr; - } + std::unordered_map> + storedValues_; + std::unordered_map nodes_; + std::uint64_t counter_ = 0; + }; static std::optional isAllocaArrayByDebugInfo(const llvm::AllocaInst* AI, const llvm::Function& F) @@ -447,16 +593,6 @@ namespace ctrace::stack::analysis return false; } - static const llvm::AllocaInst* resolveArrayAllocaFromPointer(const llvm::Value* V, - llvm::Function& F, - std::vector& path, - bool allowPointerStoreScan) - { - llvm::SmallPtrSet recursionStack; - return resolveArrayAllocaFromPointerInternal(V, F, path, recursionStack, 0, - allowPointerStoreScan); - } - static const llvm::StoreInst* findUniqueStoreToKeyInBlock(const llvm::BasicBlock& block, const llvm::Value* key) { @@ -758,6 +894,7 @@ namespace ctrace::stack::analysis return; const bool allowPointerStoreScan = instructionCount <= budgets.pointerStoreScanInstrThreshold; + ArrayAllocaResolver resolver(F, allowPointerStoreScan); const FunctionFacts facts(F); const ProgramPointRanges ranges(F, facts); std::size_t analyzedGEPs = 0; @@ -785,8 +922,7 @@ namespace ctrace::stack::analysis cached.computed = true; std::vector resolvedPath; - cached.alloca = - resolveArrayAllocaFromPointer(basePtr, F, resolvedPath, allowPointerStoreScan); + cached.alloca = resolver.resolve(basePtr, resolvedPath); if (!cached.alloca) cached.global = resolveArrayGlobalFromPointer(basePtr); @@ -1173,6 +1309,7 @@ namespace ctrace::stack::analysis return; const bool allowPointerStoreScan = instructionCount <= budgets.pointerStoreScanInstrThreshold; + ArrayAllocaResolver resolver(F, allowPointerStoreScan); std::size_t analyzedStores = 0; struct Info @@ -1203,8 +1340,7 @@ namespace ctrace::stack::analysis // Walk back to the base to find a stack array alloca. const Value* basePtr = GEP->getPointerOperand(); std::vector dummyAliasPath; - const AllocaInst* AI = resolveArrayAllocaFromPointer(basePtr, F, dummyAliasPath, - allowPointerStoreScan); + const AllocaInst* AI = resolver.resolve(basePtr, dummyAliasPath); if (!AI) continue; diff --git a/test/bound-storage/alias-chain-deep.c b/test/bound-storage/alias-chain-deep.c new file mode 100644 index 0000000..50b3e17 --- /dev/null +++ b/test/bound-storage/alias-chain-deep.c @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// A chain of 66 pointers. The safe access through the last one is resolved first; the old +// resolver gave up past 64 pointers, and a resolver that remembered the results computed +// under that cut lost the report on the write through `p1` (#215, first review). + +void cycle(int n) +{ + char buf[16] = {0}; + char* p0 = buf; + char* p1 = p0; + char* p2 = p1; + char* p3 = p2; + char* p4 = p3; + char* p5 = p4; + char* p6 = p5; + char* p7 = p6; + char* p8 = p7; + char* p9 = p8; + char* p10 = p9; + char* p11 = p10; + char* p12 = p11; + char* p13 = p12; + char* p14 = p13; + char* p15 = p14; + char* p16 = p15; + char* p17 = p16; + char* p18 = p17; + char* p19 = p18; + char* p20 = p19; + char* p21 = p20; + char* p22 = p21; + char* p23 = p22; + char* p24 = p23; + char* p25 = p24; + char* p26 = p25; + char* p27 = p26; + char* p28 = p27; + char* p29 = p28; + char* p30 = p29; + char* p31 = p30; + char* p32 = p31; + char* p33 = p32; + char* p34 = p33; + char* p35 = p34; + char* p36 = p35; + char* p37 = p36; + char* p38 = p37; + char* p39 = p38; + char* p40 = p39; + char* p41 = p40; + char* p42 = p41; + char* p43 = p42; + char* p44 = p43; + char* p45 = p44; + char* p46 = p45; + char* p47 = p46; + char* p48 = p47; + char* p49 = p48; + char* p50 = p49; + char* p51 = p50; + char* p52 = p51; + char* p53 = p52; + char* p54 = p53; + char* p55 = p54; + char* p56 = p55; + char* p57 = p56; + char* p58 = p57; + char* p59 = p58; + char* p60 = p59; + char* p61 = p60; + char* p62 = p61; + char* p63 = p62; + char* p64 = p63; + char* p65 = p64; + p65[0] = n; + + // at line 84, column 15 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay -> p0 -> p1 + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + p1[i] = 1; +} + +int main(void) +{ + cycle(3); + return 0; +} + +// The safe access through `p65` counts as a store into `buf` too: the resolver no longer gives +// up past 64 pointers. +// at line 9, column 1 +// [ !Info! ] multiple stores to stack buffer 'buf' in this function (2 store instruction(s), 2 distinct index expression(s)) +// [ !Info! ] stores use different index expressions; verify indices are correct and non-overlapping diff --git a/test/bound-storage/alias-cycle-phi.c b/test/bound-storage/alias-cycle-phi.c new file mode 100644 index 0000000..be8c924 --- /dev/null +++ b/test/bound-storage/alias-cycle-phi.c @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// A cycle that goes through PHIs of pointers: `q` takes a conditional operator over `p` and the +// buffer, and `p` is reassigned in a loop from a conditional operator over `q` and itself. The +// component holds the two pointer variables and the two PHIs, and the alias path of `q` goes +// through a PHI (`cond`). Both writes must be reported and MultipleStores must see both stores. + +// strict-expectation-details: true + +void phi_cycle(int n) +{ + char buf[16] = {0}; + char* p = buf; + char* q = (n & 1) ? p : buf; + for (int i = 0; i < n; ++i) + p = (i & 1) ? q : p; + + // at line 24, column 14 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay -> p + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + p[i] = 1; + + // at line 32, column 14 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay1 -> cond -> q + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + q[i] = 2; +} + +int main(void) +{ + phi_cycle(3); + return 0; +} + +// MultipleStores sees both writes as stores into the buffer. +// at line 12, column 1 +// [ !Info! ] multiple stores to stack buffer 'buf' in this function (2 store instruction(s), 2 distinct index expression(s)) +// [ !Info! ] stores use different index expressions; verify indices are correct and non-overlapping diff --git a/test/bound-storage/alias-cycle-terminates.c b/test/bound-storage/alias-cycle-terminates.c new file mode 100644 index 0000000..99e6987 --- /dev/null +++ b/test/bound-storage/alias-cycle-terminates.c @@ -0,0 +1,155 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Eight pointers in a chain from one buffer, then each assigned from every other one under a +// condition: one strongly connected component where only `p0` points to the array directly. +// The paths between them number 7! = 5040, and each one used to cost a scan of the function: +// StackBufferOverflow and MultipleStores took hours on such functions (#215, zlib crc32.c). +// The last pointer is chosen by a conditional operator, which is a PHI of pointers in the IR. + +void cycle(int n) +{ + char buf[16] = {0}; + char* p0 = buf; + char* p1 = p0; + char* p2 = p1; + char* p3 = p2; + char* p4 = p3; + char* p5 = p4; + char* p6 = p5; + char* p7 = p6; + if (n == 1) + p0 = p1; + if (n == 2) + p0 = p2; + if (n == 3) + p0 = p3; + if (n == 4) + p0 = p4; + if (n == 5) + p0 = p5; + if (n == 6) + p0 = p6; + if (n == 7) + p0 = p7; + if (n == 8) + p1 = p0; + if (n == 10) + p1 = p2; + if (n == 11) + p1 = p3; + if (n == 12) + p1 = p4; + if (n == 13) + p1 = p5; + if (n == 14) + p1 = p6; + if (n == 15) + p1 = p7; + if (n == 16) + p2 = p0; + if (n == 17) + p2 = p1; + if (n == 19) + p2 = p3; + if (n == 20) + p2 = p4; + if (n == 21) + p2 = p5; + if (n == 22) + p2 = p6; + if (n == 23) + p2 = p7; + if (n == 24) + p3 = p0; + if (n == 25) + p3 = p1; + if (n == 26) + p3 = p2; + if (n == 28) + p3 = p4; + if (n == 29) + p3 = p5; + if (n == 30) + p3 = p6; + if (n == 31) + p3 = p7; + if (n == 32) + p4 = p0; + if (n == 33) + p4 = p1; + if (n == 34) + p4 = p2; + if (n == 35) + p4 = p3; + if (n == 37) + p4 = p5; + if (n == 38) + p4 = p6; + if (n == 39) + p4 = p7; + if (n == 40) + p5 = p0; + if (n == 41) + p5 = p1; + if (n == 42) + p5 = p2; + if (n == 43) + p5 = p3; + if (n == 44) + p5 = p4; + if (n == 46) + p5 = p6; + if (n == 47) + p5 = p7; + if (n == 48) + p6 = p0; + if (n == 49) + p6 = p1; + if (n == 50) + p6 = p2; + if (n == 51) + p6 = p3; + if (n == 52) + p6 = p4; + if (n == 53) + p6 = p5; + if (n == 55) + p6 = p7; + if (n == 56) + p7 = p0; + if (n == 57) + p7 = p1; + if (n == 58) + p7 = p2; + if (n == 59) + p7 = p3; + if (n == 60) + p7 = p4; + if (n == 61) + p7 = p5; + if (n == 62) + p7 = p6; + char* q = n < 32 ? p0 : p1; + + // at line 140, column 15 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay -> p0 + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + p0[i] = 1; + + // at line 148, column 14 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay -> p0 -> cond -> q + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + q[i] = 1; +} + +int main(void) +{ + cycle(3); + return 0; +} diff --git a/test/bound-storage/alias-cycle-three-pointers.c b/test/bound-storage/alias-cycle-three-pointers.c new file mode 100644 index 0000000..111a55a --- /dev/null +++ b/test/bound-storage/alias-cycle-three-pointers.c @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Three pointers assigned in a ring from one buffer: one strongly connected component where only +// `p` points to the array directly, `q` through `p` and `r` through `q`. Every pointer must find +// the array, and the order of the accesses must not matter: `ring_pqr` writes through p, q and +// r, `ring_rqp` through r, q and p, and both report the same overflows with the same alias +// paths. The three writes also make MultipleStores report the buffer of each function. + +// strict-expectation-details: true + +void ring_pqr(int n) +{ + char buf[16] = {0}; + char* p = buf; + char* q = p; + char* r = q; + if (n) + p = r; + + // at line 26, column 14 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay -> p + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + p[i] = 1; + + // at line 34, column 14 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay -> p -> q + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + q[i] = 2; + + // at line 42, column 14 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay -> p -> q -> r + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + r[i] = 3; +} + +void ring_rqp(int n) +{ + char buf[16] = {0}; + char* p = buf; + char* q = p; + char* r = q; + if (n) + p = r; + + // at line 60, column 14 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay -> p -> q -> r + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + r[i] = 1; + + // at line 68, column 14 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay -> p -> q + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + q[i] = 2; + + // at line 76, column 14 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay -> p + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + p[i] = 3; +} + +int main(void) +{ + ring_pqr(0); + ring_rqp(1); + return 0; +} + +// MultipleStores sees the three writes of each function as stores into its buffer. +// at line 13, column 1 +// [ !Info! ] multiple stores to stack buffer 'buf' in this function (3 store instruction(s), 3 distinct index expression(s)) +// [ !Info! ] stores use different index expressions; verify indices are correct and non-overlapping + +// at line 47, column 1 +// [ !Info! ] multiple stores to stack buffer 'buf' in this function (3 store instruction(s), 3 distinct index expression(s)) +// [ !Info! ] stores use different index expressions; verify indices are correct and non-overlapping diff --git a/test/bound-storage/alias-cycle-two-arrays.c b/test/bound-storage/alias-cycle-two-arrays.c new file mode 100644 index 0000000..1c4e03d --- /dev/null +++ b/test/bound-storage/alias-cycle-two-arrays.c @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Pointers assigned from each other under a condition form one strongly connected component. +// When the component reaches two arrays of different sizes, the resolver reports no array for +// it, as it does for any pointer that may point into several arrays, so neither write through +// `p` or `q` is reported even though both arrays overflow at index 39. This is a deliberate +// trade-off, not a gain in detection: the old resolver reported each pointer against its own +// array, which depended on the order of the walk. The control case `same_array` has the same +// shape with both pointers from one array, and its overflow must be reported. + +// strict-expectation-details: true +// not contains: overflow on variable 'a' (size 16) +// not contains: overflow on variable 'b' (size 32) + +void both_arrays(int n) +{ + char a[16] = {0}; + char b[32] = {0}; + char* p = a; + char* q = b; + if (n) + p = q; + else + q = p; + p[0] = 0; + for (int i = 0; i < 40; ++i) + q[i] = 1; +} + +void both_arrays_reverse_query(int n) +{ + char a[16] = {0}; + char b[32] = {0}; + char* p = a; + char* q = b; + if (n) + p = q; + else + q = p; + q[0] = 0; + for (int i = 0; i < 40; ++i) + p[i] = 1; +} + +void same_array(int n) +{ + char c[16] = {0}; + char* p = c; + char* q = c; + if (n) + p = q; + else + q = p; + p[0] = 0; + + // at line 62, column 14 + // [ !!Warn ] potential stack buffer overflow on variable 'c' (size 16) + // ↳ alias path: c -> arraydecay1 -> q + // ↳ index variable may go up to 39 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 40; ++i) + q[i] = 1; +} + +int main(void) +{ + both_arrays(0); + both_arrays_reverse_query(1); + same_array(0); + return 0; +} + +// In the control case, MultipleStores sees both writes as stores into the one array. +// at line 47, column 1 +// [ !Info! ] multiple stores to stack buffer 'c' in this function (2 store instruction(s), 2 distinct index expression(s)) +// [ !Info! ] stores use different index expressions; verify indices are correct and non-overlapping diff --git a/test/bound-storage/alias-cycle.c b/test/bound-storage/alias-cycle.c new file mode 100644 index 0000000..d944ce6 --- /dev/null +++ b/test/bound-storage/alias-cycle.c @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Two pointers assigned from each other. Resolving `p` reaches `q`, which reaches `p` again +// while `p` is still being resolved. A result remembered for `q` at that moment would say +// "nothing", and the write through `q` would go unreported (#215, first review). + +void cycle(int n) +{ + char buf[16] = {0}; + char* p = buf; + char* q = p; + p = q; + p[0] = n; + + // at line 21, column 14 + // [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 16) + // ↳ alias path: buf -> arraydecay -> p -> q + // ↳ index variable may go up to 19 (array last valid index: 15) + // ↳ (this is a write access) + for (int i = 0; i < 20; ++i) + q[i] = 1; +} + +int main(void) +{ + cycle(3); + return 0; +} diff --git a/test/unit/alias_phi_cycle_input.ll b/test/unit/alias_phi_cycle_input.ll new file mode 100644 index 0000000..cac32b2 --- /dev/null +++ b/test/unit/alias_phi_cycle_input.ll @@ -0,0 +1,26 @@ +; SPDX-License-Identifier: Apache-2.0 +; A loop whose two pointer PHIs take each other's value: the cycle p -> q -> p that mem2reg +; leaves in optimized code. Both PHIs must resolve to buf through its decay, and the two stores +; through them must count as stores into buf. + +define void @phi_cycle(i32 %n) { +entry: + %buf = alloca [16 x i8], align 1 + %start = getelementptr inbounds [16 x i8], ptr %buf, i64 0, i64 0 + br label %loop + +loop: + %i = phi i32 [ 0, %entry ], [ %i.next, %loop ] + %p = phi ptr [ %start, %entry ], [ %q, %loop ] + %q = phi ptr [ %start, %entry ], [ %p, %loop ] + %p.slot = getelementptr inbounds i8, ptr %p, i64 20 + store i8 0, ptr %p.slot, align 1 + %q.slot = getelementptr inbounds i8, ptr %q, i64 21 + store i8 1, ptr %q.slot, align 1 + %i.next = add nsw i32 %i, 1 + %more = icmp slt i32 %i.next, %n + br i1 %more, label %loop, label %exit + +exit: + ret void +} diff --git a/test/unit/analyzer_module_unit_tests.cpp b/test/unit/analyzer_module_unit_tests.cpp index 3a98dfe..698b961 100644 --- a/test/unit/analyzer_module_unit_tests.cpp +++ b/test/unit/analyzer_module_unit_tests.cpp @@ -45,6 +45,7 @@ #include #include #include +#include #include #include #include @@ -2752,6 +2753,131 @@ namespace } return report.failures == 0; } + /// A loop whose two pointer PHIs take each other's value, as mem2reg leaves optimized code: + /// a cycle through PHIs. Both analyses that use the resolver must find the array through it. + bool testStackBufferPhiCycle(const std::filesystem::path& repoRoot, TestReport& report) + { + using namespace ctrace::stack::analysis; + const ctrace::stack::AnalysisConfig config; + LoadedModule loaded; + std::string loadError; + if (!loadModuleFromSource(repoRoot / "test/unit/alias_phi_cycle_input.ll", config, loaded, + loadError)) + { + report.expect(false, + "stack-buffer PHI cycle setup: failed to load module: " + loadError); + return false; + } + const auto all = [](const llvm::Function&) { return true; }; + + std::map pathByIndex; + for (const StackBufferOverflowIssue& issue : + analyzeStackBufferOverflows(*loaded.module, all, config)) + { + if (issue.indexIsConstant && issue.varName == "buf") + pathByIndex[issue.indexOrUpperBound] = issue.aliasPath; + } + report.expect(pathByIndex.size() == 2 && pathByIndex[20] == "buf -> start -> p" && + pathByIndex[21] == "buf -> start -> q", + "StackBufferOverflow: each PHI of a cycle resolves to the array through " + "its decay"); + + const std::vector stores = + analyzeMultipleStores(*loaded.module, all, config); + report.expect(stores.size() == 1 && stores[0].varName == "buf" && + stores[0].storeCount == 2 && stores[0].distinctIndexCount == 2, + "MultipleStores: both stores through the PHI cycle reach the buffer"); + return report.failures == 0; + } + + /// A ring of pointer variables with one array source, as -O0 code leaves it: p0 = buf, + /// p1 = p0, ..., p0 = p(n-1). Every pointer must find the array, however far from it; the + /// order of the accesses must not change the alias paths; and the walk must finish on a + /// component of thousands of nodes. + bool testStackBufferPointerRing(TestReport& report) + { + using namespace llvm; + using namespace ctrace::stack::analysis; + constexpr unsigned kRing = 5000; + const unsigned accessed[] = {0, kRing / 2, kRing - 1}; + + LLVMContext context; + Module module("pointer_ring", context); + const auto build = [&](const char* name, bool reversed) + { + Function* F = Function::Create(FunctionType::get(Type::getVoidTy(context), false), + Function::ExternalLinkage, name, module); + IRBuilder<> builder(BasicBlock::Create(context, "entry", F)); + Type* bufType = ArrayType::get(builder.getInt8Ty(), 16); + AllocaInst* buf = builder.CreateAlloca(bufType, nullptr, "buf"); + std::vector pointers; + for (unsigned i = 0; i < kRing; ++i) + pointers.push_back( + builder.CreateAlloca(builder.getPtrTy(), nullptr, "p" + std::to_string(i))); + Value* decay = builder.CreateInBoundsGEP( + bufType, buf, {builder.getInt64(0), builder.getInt64(0)}, "arraydecay"); + builder.CreateStore(decay, pointers[0]); + for (unsigned i = 1; i < kRing; ++i) + builder.CreateStore(builder.CreateLoad(builder.getPtrTy(), pointers[i - 1]), + pointers[i]); + builder.CreateStore(builder.CreateLoad(builder.getPtrTy(), pointers[kRing - 1]), + pointers[0]); + for (unsigned a = 0; a < 3; ++a) + { + // One out-of-bounds constant index per access (16, 17, 18), so the accesses can + // be told apart and MultipleStores sees distinct index expressions. + const unsigned which = reversed ? 2 - a : a; + Value* pointer = builder.CreateLoad(builder.getPtrTy(), pointers[accessed[which]]); + Value* slot = builder.CreateInBoundsGEP(builder.getInt8Ty(), pointer, + builder.getInt64(16 + which)); + builder.CreateStore(builder.getInt8(1), slot); + } + builder.CreateRetVoid(); + }; + build("ring", false); + build("ring_reversed", true); + + const ctrace::stack::AnalysisConfig config; + const auto pathsOf = [&](const char* name) + { + std::map> paths; + for (const StackBufferOverflowIssue& issue : analyzeStackBufferOverflows( + module, [&](const Function& F) { return F.getName() == name; }, config)) + { + if (issue.indexIsConstant && issue.varName == "buf") + paths[issue.indexOrUpperBound] = issue.aliasPathVec; + } + return paths; + }; + const auto ring = pathsOf("ring"); + const auto reversed = pathsOf("ring_reversed"); + const auto reaches = [&](std::uint64_t index, unsigned k) + { + const auto path = ring.find(index); + return path != ring.end() && path->second.size() == k + 3 && + path->second.front() == "p" + std::to_string(k) && path->second.back() == "buf"; + }; + report.expect(ring.size() == 3 && reaches(16, 0) && reaches(17, kRing / 2) && + reaches(18, kRing - 1), + "StackBufferOverflow: every pointer of a ring of 5000 finds the array, " + "2500 and 4999 steps away included"); + report.expect(reversed == ring, "StackBufferOverflow: the alias paths of a ring do not " + "depend on the order of the accesses"); + + const std::vector stores = + analyzeMultipleStores(module, [](const Function&) { return true; }, config); + report.expect(stores.size() == 2 && std::all_of(stores.begin(), stores.end(), + [](const MultipleStoreIssue& issue) + { + return issue.varName == "buf" && + issue.storeCount == 3 && + issue.distinctIndexCount == 3; + }), + "MultipleStores: the three stores through the ring reach the buffer in " + "both functions"); + return report.failures == 0; + } + } // namespace int main(int argc, char** argv) @@ -2795,6 +2921,8 @@ int main(int argc, char** argv) (void)testZ3SymbolsWithOneNameStayApart(report); #endif (void)testDuplicateIfIndexWidths(repoRoot, report); + (void)testStackBufferPhiCycle(repoRoot, report); + (void)testStackBufferPointerRing(report); (void)testResourceModelConditions(report); (void)testOwnershipFactCollector(repoRoot, report); (void)testOwnershipCollectorExceptions(repoRoot, report);