From aeb314cde2195537d22217967aa05d62e960b204 Mon Sep 17 00:00:00 2001 From: Hugo Date: Sat, 7 Nov 2026 19:27:41 +0100 Subject: [PATCH 1/2] feat(analysis): summarize what functions access through their pointer parameters A summary lists, for each pointer parameter of a function, the loads, stores and memory intrinsics of a constant length made through it, at a constant offset from the address it holds on entry, on every run of the function that returns: the block of the access post-dominates the entry. The walk follows GEPs with a constant offset, the variables the pointer is stored in, and the calls it is passed to, whose own accesses it shifts by the offset. For each parameter it keeps the reads and the writes that reach furthest either way, with the place of the access. An access on some runs only, at a variable offset, or through a function pointer is left out: a summary lists accesses that happen, and never shows that a call is safe. A byval parameter points to the callee's own copy and gets no accesses. Summaries are computed callees first, over each cycle of calls in rounds, and across modules in rounds. A symbol defined once, by a definition the linker cannot replace, publishes its summary. Offsets past 2^32 bytes, and rounds that do not settle within their budget, make a summary incomplete; the accesses it lists still happen. The variables are read back through FunctionFacts::loadsReadingStore, the loads that read exactly what a store wrote. A local slot written by that store alone, whose address is only loaded from, needs no MemorySSA: only the loads the store dominates read it. Any other slot takes the MemorySSA clobber, as in #265. Unit tests cover the slot shortcut against a call or an alias that rewrites the slot and against a load before the store, the accesses left out, the budgets, incomplete summaries, byval, and across modules a chain, a weak and a twice-defined function, and a cycle. Nothing reads the summaries yet. Refs #266 --- CMakeLists.txt | 1 + include/analysis/FunctionFacts.hpp | 12 + include/analysis/ParamAccessSummary.hpp | 82 +++++ src/analysis/FunctionFacts.cpp | 40 +++ src/analysis/ParamAccessSummary.cpp | 370 +++++++++++++++++++++++ test/unit/analyzer_module_unit_tests.cpp | 246 +++++++++++++++ 6 files changed, 751 insertions(+) create mode 100644 include/analysis/ParamAccessSummary.hpp create mode 100644 src/analysis/ParamAccessSummary.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 0b17b51..2c1597f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -91,6 +91,7 @@ set(STACK_ANALYZER_SOURCES src/analysis/MemIntrinsicOverflow.cpp src/analysis/NullDerefAnalysis.cpp src/analysis/OOBReadAnalysis.cpp + src/analysis/ParamAccessSummary.cpp src/analysis/ParameterDebugBinding.cpp src/analysis/PromotedValues.cpp src/analysis/ResourceLifetimeAnalysis.cpp diff --git a/include/analysis/FunctionFacts.hpp b/include/analysis/FunctionFacts.hpp index 7094d31..f75e743 100644 --- a/include/analysis/FunctionFacts.hpp +++ b/include/analysis/FunctionFacts.hpp @@ -15,6 +15,8 @@ namespace llvm class LazyValueInfo; class LoadInst; class MemoryAccess; + template class SmallVectorImpl; + class StoreInst; class TargetLibraryInfo; class Value; } // namespace llvm @@ -78,6 +80,16 @@ namespace ctrace::stack::analysis /// stores to that pointer is the value they read. [[nodiscard]] const llvm::MemoryAccess* clobberingAccess(const llvm::LoadInst& load) const; + /// @brief Appends to @p loads the simple loads, of the stored type and from the stored + /// pointer, that read exactly the value @p store writes. + /// + /// A load reads it when its clobber is @p store. A local slot that @p store alone + /// writes, whose address is only loaded from, needs no MemorySSA: no other store, call + /// or alias can change it, so a load reads the stored value wherever @p store dominates + /// it. A slot written elsewhere, or whose address goes anywhere else, takes the clobber. + void loadsReadingStore(const llvm::StoreInst& store, + llvm::SmallVectorImpl& loads) const; + private: struct Impl; std::unique_ptr impl_; diff --git a/include/analysis/ParamAccessSummary.hpp b/include/analysis/ParamAccessSummary.hpp new file mode 100644 index 0000000..c597ef9 --- /dev/null +++ b/include/analysis/ParamAccessSummary.hpp @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: Apache-2.0 +#pragma once + +#include +#include +#include +#include +#include + +namespace llvm +{ + class CallBase; + class Function; + class Module; +} // namespace llvm + +namespace ctrace::stack::analysis +{ + /// A load, a store or a memory intrinsic that a function makes through a pointer parameter, + /// at a constant offset from the address the parameter holds on entry, on every run of the + /// function that returns (#266). The location is the access itself, at the end of the calls. + struct ParamAccess + { + std::int64_t offset = 0; // bytes from the parameter's address + std::uint64_t size = 0; // bytes + std::string function; // linker symbol of the function that makes the access + std::string file; + unsigned line = 0; + unsigned column = 0; + }; + + /// The accesses through one parameter that reach furthest each way: the lowest offset and + /// the highest end, for reads and for writes. + struct ParamAccessBounds + { + std::optional lowestRead; + std::optional highestRead; + std::optional lowestWrite; + std::optional highestWrite; + }; + + /// What a function is known to access through its pointer parameters, one entry per + /// parameter. + /// + /// A summary lists accesses that happen. It never lists all of them: an access on some runs + /// only, at a variable offset, or through a pointer the walk loses is left out. No summary, + /// complete or not, shows that a call is safe. + /// + /// complete is false when a budget stopped the computation (offsets too large, rounds over + /// a cycle of calls), or when the summary used an incomplete one. Its accesses still happen. + struct ParamAccessSummary + { + std::vector params; + std::uint64_t complete : 1 = true; + std::uint64_t reservedFlags : 63 = 0; + }; + + /// Summaries keyed by linker symbol, for calls from other modules. + struct ParamAccessIndex + { + std::map functions; + }; + + using ParamAccessSummaries = std::map; + + /// The summaries of the functions @p mod defines. A call to a declaration takes the summary + /// @p elsewhere has for its symbol, if it has one parameter per argument. + ParamAccessSummaries computeParamAccessSummaries(llvm::Module& mod, + const ParamAccessIndex* elsewhere); + + /// The summary a call reaches: from @p summaries for a function of this module, from + /// @p elsewhere for a declaration. Null for an indirect call or a function without one. + const ParamAccessSummary* calleeParamAccesses(const llvm::CallBase& call, + const ParamAccessSummaries& summaries, + const ParamAccessIndex* elsewhere); + + /// The summaries of the external functions defined across @p modules, for each module to + /// use for the others. Rounds repeat until one changes nothing, or until a budget of rounds + /// stops them and marks every summary incomplete. A symbol with more than one definition, or + /// one that may be replaced at link time, gets none. + ParamAccessIndex paramAccessesAcrossModules(const std::vector& modules); +} // namespace ctrace::stack::analysis diff --git a/src/analysis/FunctionFacts.cpp b/src/analysis/FunctionFacts.cpp index 21c6cac..fca15e6 100644 --- a/src/analysis/FunctionFacts.cpp +++ b/src/analysis/FunctionFacts.cpp @@ -179,4 +179,44 @@ namespace ctrace::stack::analysis } return impl.memoryModel->memorySSA->getWalker()->getClobberingMemoryAccess(&load); } + + void FunctionFacts::loadsReadingStore(const llvm::StoreInst& store, + llvm::SmallVectorImpl& loads) const + { + using namespace llvm; + if (!store.isSimple()) + return; + const Value* slot = store.getPointerOperand(); + const Type* type = store.getValueOperand()->getType(); + + SmallVector candidates; + const auto* alloca = dyn_cast(slot); + bool onlyThisStore = alloca && alloca->getFunction() == &impl_->function; + for (const Use& use : slot->uses()) + { + if (auto* load = dyn_cast(use.getUser())) + { + if (load->isSimple() && load->getType() == type) + candidates.push_back(load); + continue; + } + // Any other use but this store writing the slot may write it, or let an alias or a + // call write it. + if (use.getUser() != &store || use.getOperandNo() != store.getPointerOperandIndex()) + onlyThisStore = false; + } + + for (LoadInst* load : candidates) + { + if (onlyThisStore) + { + if (impl_->dominatorTree.dominates(&store, load)) + loads.push_back(load); + continue; + } + const auto* def = dyn_cast_or_null(clobberingAccess(*load)); + if (def && def->getMemoryInst() == &store) + loads.push_back(load); + } + } } // namespace ctrace::stack::analysis diff --git a/src/analysis/ParamAccessSummary.cpp b/src/analysis/ParamAccessSummary.cpp new file mode 100644 index 0000000..2d0bcaa --- /dev/null +++ b/src/analysis/ParamAccessSummary.cpp @@ -0,0 +1,370 @@ +// SPDX-License-Identifier: Apache-2.0 +#include "analysis/ParamAccessSummary.hpp" + +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "analysis/AnalyzerUtils.hpp" +#include "analysis/FunctionFacts.hpp" +#include "analysis/IRValueUtils.hpp" + +namespace ctrace::stack::analysis +{ + namespace + { + using namespace llvm; + + // Budgets. Reaching one marks the summary incomplete; the accesses found still happen. + /// Offsets and sizes are followed up to this many bytes either way. + constexpr std::int64_t kMaxTrackedBytes = std::int64_t{1} << 32; + /// Rounds over the functions of one cycle of calls in a module. + constexpr unsigned kMaxRoundsPerCycle = 8; + /// Rounds over the modules analyzed together. + constexpr unsigned kMaxRoundsAcrossModules = 8; + + bool withinBudget(std::int64_t bytes) + { + return bytes >= -kMaxTrackedBytes && bytes <= kMaxTrackedBytes; + } + + std::int64_t endOf(const ParamAccess& access) + { + return access.offset + static_cast(access.size); + } + + bool sameReach(const std::optional& lhs, const std::optional& rhs) + { + if (!lhs || !rhs) + return lhs.has_value() == rhs.has_value(); + return lhs->offset == rhs->offset && lhs->size == rhs->size; + } + + /// Equal offsets and sizes; the locations, which only name an access, do not count. + bool sameReach(const ParamAccessSummary& lhs, const ParamAccessSummary& rhs) + { + if (lhs.complete != rhs.complete || lhs.params.size() != rhs.params.size()) + return false; + for (std::size_t i = 0; i < lhs.params.size(); ++i) + { + const ParamAccessBounds& a = lhs.params[i]; + const ParamAccessBounds& b = rhs.params[i]; + if (!sameReach(a.lowestRead, b.lowestRead) || + !sameReach(a.highestRead, b.highestRead) || + !sameReach(a.lowestWrite, b.lowestWrite) || + !sameReach(a.highestWrite, b.highestWrite)) + return false; + } + return true; + } + + bool sameReach(const ParamAccessIndex& lhs, const ParamAccessIndex& rhs) + { + return lhs.functions.size() == rhs.functions.size() && + std::equal(lhs.functions.begin(), lhs.functions.end(), rhs.functions.begin(), + [](const auto& a, const auto& b) + { return a.first == b.first && sameReach(a.second, b.second); }); + } + + /// Keeps @p access in @p bounds where it reaches further. A tie keeps the access found + /// first. + void widen(ParamAccessBounds& bounds, const ParamAccess& access, bool write) + { + std::optional& lowest = write ? bounds.lowestWrite : bounds.lowestRead; + std::optional& highest = write ? bounds.highestWrite : bounds.highestRead; + if (!lowest || access.offset < lowest->offset) + lowest = access; + if (!highest || endOf(access) > endOf(*highest)) + highest = access; + } + + /// The bytes a load or a store of @p type touches, if fixed. + std::optional storeBytes(const DataLayout& layout, Type* type) + { + const TypeSize size = layout.getTypeStoreSize(type); + if (size.isScalable()) + return std::nullopt; + return size.getFixedValue(); + } + + /// The summary of @p F, from the summaries known so far for the functions it calls. + /// + /// From each pointer parameter, the walk follows the pointer through GEPs with a + /// constant offset, and through the variables it is stored in, to the loads that read it + /// back (FunctionFacts::loadsReadingStore). It records the loads, stores and memory + /// intrinsics of a constant length made through it, and the accesses of the functions it + /// is passed to, shifted by its offset. Only an access on every run of @p F that returns + /// counts: its block post-dominates the entry. Anything else stops the walk there. + ParamAccessSummary summarize(Function& F, const ParamAccessSummaries& known, + const ParamAccessIndex* elsewhere) + { + ParamAccessSummary summary; + summary.params.resize(F.arg_size()); + if (std::none_of(F.arg_begin(), F.arg_end(), + [](const Argument& arg) { return arg.getType()->isPointerTy(); })) + return summary; + + const DataLayout& layout = F.getParent()->getDataLayout(); + std::optional facts; + std::optional postDominators; + const auto onEveryRun = [&](const Instruction& inst) + { + if (!postDominators) + postDominators.emplace(F); + return postDominators->dominates(inst.getParent(), &F.getEntryBlock()); + }; + const auto accessAt = + [&](const Instruction& inst, std::int64_t offset, std::uint64_t size) + { + ParamAccess access; + access.offset = offset; + access.size = size; + access.function = F.getName().str(); + if (const DILocation* location = inst.getDebugLoc().get()) + { + access.file = sys::path::filename(location->getFilename()).str(); + access.line = location->getLine(); + access.column = location->getColumn(); + } + return access; + }; + + for (Argument& arg : F.args()) + { + // byval and the like point to the callee's own copy, not to the caller's object. + if (!arg.getType()->isPointerTy() || arg.hasPassPointeeByValueCopyAttr()) + continue; + ParamAccessBounds& bounds = summary.params[arg.getArgNo()]; + const auto record = [&](const Instruction& inst, std::int64_t offset, + std::optional size, bool write) + { + if (!size || *size == 0) + return; + if (*size > static_cast(kMaxTrackedBytes)) + { + summary.complete = false; + return; + } + if (onEveryRun(inst)) + widen(bounds, accessAt(inst, offset, *size), write); + }; + + // Each value has one offset from the parameter: a GEP from its base, a load + // from the one store it reads. + SmallVector, 8> pointers{{&arg, 0}}; + SmallPtrSet seen; + while (!pointers.empty()) + { + Value* pointer = pointers.back().first; + const std::int64_t offset = pointers.back().second; + pointers.pop_back(); + if (!seen.insert(pointer).second) + continue; + for (Use& use : pointer->uses()) + { + auto* user = dyn_cast(use.getUser()); + if (!user) + continue; + + if (auto* gep = dyn_cast(user)) + { + APInt delta(layout.getIndexTypeSizeInBits(gep->getType()), 0); + if (use.getOperandNo() != GetElementPtrInst::getPointerOperandIndex() || + !gep->accumulateConstantOffset(layout, delta)) + continue; + if (delta.getSignificantBits() > 63 || + !withinBudget(delta.getSExtValue()) || + !withinBudget(offset + delta.getSExtValue())) + { + summary.complete = false; + continue; + } + pointers.emplace_back(gep, offset + delta.getSExtValue()); + continue; + } + if (auto* load = dyn_cast(user)) + { + record(*load, offset, storeBytes(layout, load->getType()), false); + continue; + } + if (auto* store = dyn_cast(user)) + { + if (use.getOperandNo() == StoreInst::getPointerOperandIndex()) + { + record(*store, offset, + storeBytes(layout, store->getValueOperand()->getType()), + true); + continue; + } + if (!facts) + facts.emplace(F); + SmallVector loads; + facts->loadsReadingStore(*store, loads); + for (LoadInst* load : loads) + pointers.emplace_back(load, offset); + continue; + } + if (auto* memory = dyn_cast(user)) + { + const auto* length = dyn_cast(memory->getLength()); + if (!length || length->getValue().getActiveBits() > 63) + continue; + if (use.getOperandNo() == 0) + record(*memory, offset, length->getZExtValue(), true); + else if (isa(memory) && use.getOperandNo() == 1) + record(*memory, offset, length->getZExtValue(), false); + continue; + } + + auto* call = dyn_cast(user); + if (!call || !call->isArgOperand(&use)) + continue; + const ParamAccessSummary* callee = + calleeParamAccesses(*call, known, elsewhere); + const unsigned argNo = call->getArgOperandNo(&use); + if (!callee || argNo >= callee->params.size() || !onEveryRun(*call)) + continue; + if (!callee->complete) + summary.complete = false; + const ParamAccessBounds& through = callee->params[argNo]; + const auto import = + [&](const std::optional& access, bool write) + { + if (!access) + return; + if (!withinBudget(offset + access->offset)) + { + summary.complete = false; + return; + } + ParamAccess shifted = *access; + shifted.offset = offset + access->offset; + widen(bounds, shifted, write); + }; + import(through.lowestRead, false); + import(through.highestRead, false); + import(through.lowestWrite, true); + import(through.highestWrite, true); + } + } + } + return summary; + } + } // namespace + + const ParamAccessSummary* calleeParamAccesses(const llvm::CallBase& call, + const ParamAccessSummaries& summaries, + const ParamAccessIndex* elsewhere) + { + const llvm::Function* callee = directCallee(call); + if (!callee) + return nullptr; + if (!callee->isDeclaration()) + { + // A definition the linker may replace says nothing about the one that runs. + if (callee->isInterposable()) + return nullptr; + const auto it = summaries.find(callee); + return it == summaries.end() ? nullptr : &it->second; + } + if (!elsewhere) + return nullptr; + const auto it = elsewhere->functions.find(linkerSymbolName(*callee)); + if (it == elsewhere->functions.end() || it->second.params.size() != call.arg_size()) + return nullptr; + return &it->second; + } + + ParamAccessSummaries computeParamAccessSummaries(llvm::Module& mod, + const ParamAccessIndex* elsewhere) + { + using namespace llvm; + ParamAccessSummaries summaries; + llvm::CallGraph graph(mod); + // Callees come before their callers. A cycle of calls is one component, summarized in + // rounds until a round changes nothing. + for (auto component = scc_begin(&graph); !component.isAtEnd(); ++component) + { + SmallVector functions; + for (CallGraphNode* node : *component) + { + Function* F = node->getFunction(); + if (F && !F->isDeclaration()) + functions.push_back(F); + } + if (functions.empty()) + continue; + + const unsigned maxRounds = component.hasCycle() ? kMaxRoundsPerCycle : 1; + bool changed = true; + for (unsigned round = 0; changed && round < maxRounds; ++round) + { + changed = false; + for (Function* F : functions) + { + ParamAccessSummary next = summarize(*F, summaries, elsewhere); + const auto [it, inserted] = summaries.try_emplace(F, next); + if (inserted || !sameReach(it->second, next)) + { + it->second = std::move(next); + changed = true; + } + } + } + if (component.hasCycle() && changed) + { + for (Function* F : functions) + summaries[F].complete = false; + } + } + return summaries; + } + + ParamAccessIndex paramAccessesAcrossModules(const std::vector& modules) + { + using namespace llvm; + std::map definitions; + for (const Module* mod : modules) + { + for (const Function& F : *mod) + if (!F.isDeclaration() && !F.hasLocalLinkage()) + ++definitions[linkerSymbolName(F)]; + } + + ParamAccessIndex published; + for (unsigned round = 0; round < kMaxRoundsAcrossModules; ++round) + { + ParamAccessIndex next; + for (Module* mod : modules) + { + for (const auto& [F, summary] : computeParamAccessSummaries(*mod, &published)) + { + if (F->hasLocalLinkage() || F->isInterposable()) + continue; + const std::string symbol = linkerSymbolName(*F); + if (definitions[symbol] == 1) + next.functions.emplace(symbol, summary); + } + } + if (sameReach(next, published)) + return published; + published = std::move(next); + } + for (auto& [symbol, summary] : published.functions) + summary.complete = false; + return published; + } +} // namespace ctrace::stack::analysis diff --git a/test/unit/analyzer_module_unit_tests.cpp b/test/unit/analyzer_module_unit_tests.cpp index a631e8d..65f8caf 100644 --- a/test/unit/analyzer_module_unit_tests.cpp +++ b/test/unit/analyzer_module_unit_tests.cpp @@ -10,6 +10,7 @@ #include "analysis/InputPipelineRevision.hpp" #include "analysis/IntRanges.hpp" #include "analysis/IRValueUtils.hpp" +#include "analysis/ParamAccessSummary.hpp" #include "analysis/PromotedValues.hpp" #include "analysis/ParameterDebugBinding.hpp" #include "analysis/Reachability.hpp" @@ -1798,6 +1799,250 @@ declare void @llvm.lifetime.end.p0(i64, ptr) return report.failures == 0; } + bool testParamAccessSummary(TestReport& report) + { + using namespace ctrace::stack::analysis; + llvm::LLVMContext context; + llvm::SMDiagnostic error; + const std::unique_ptr module = llvm::parseAssemblyString(R"( +declare void @clobber(ptr) + +; The slot of p has one store, but a call gets its address and may rewrite it. +define void @slot_escapes(ptr %p) { +entry: + %slot = alloca ptr + store ptr %p, ptr %slot + call void @clobber(ptr %slot) + %q = load ptr, ptr %slot + store i8 1, ptr %q + ret void +} + +; The slot of p is rewritten through an alias of its address. +define void @slot_aliased(ptr %p, ptr %other) { +entry: + %slot = alloca ptr + %alias = alloca ptr + store ptr %p, ptr %slot + store ptr %slot, ptr %alias + %a = load ptr, ptr %alias + store ptr %other, ptr %a + %q = load ptr, ptr %slot + store i8 1, ptr %q + ret void +} + +; The load runs before p is stored in the slot. +define void @read_before_store(ptr %p) { +entry: + %slot = alloca ptr + %early = load ptr, ptr %slot + store ptr %p, ptr %slot + store i8 1, ptr %early + ret void +} + +; One store, then loads only: the loads read p. +define void @plain_slot(ptr %p) { +entry: + %slot = alloca ptr + store ptr %p, ptr %slot + %q = load ptr, ptr %slot + %at = getelementptr i8, ptr %q, i64 3 + store i16 1, ptr %at + ret void +} + +; A write at a variable offset, and one on some runs only. +define void @conditional_and_variable(ptr %p, i1 %c, i64 %i) { +entry: + %at = getelementptr i8, ptr %p, i64 %i + store i8 1, ptr %at + br i1 %c, label %write, label %done +write: + store i8 1, ptr %p + br label %done +done: + ret void +} + +; Each call writes one byte further on: the rounds stop at their budget. +define void @walk(ptr %p) { +entry: + store i8 0, ptr %p + %next = getelementptr i8, ptr %p, i64 1 + call void @walk(ptr %next) + ret void +} + +define void @calls_walk(ptr %p) { +entry: + call void @walk(ptr %p) + ret void +} + +; An offset past the budget. +define void @far(ptr %p) { +entry: + %at = getelementptr i8, ptr %p, i64 8589934592 + store i8 1, ptr %at + ret void +} + +; A byval parameter points to the callee's own copy. +define void @by_value(ptr byval([4 x i8]) %p) { +entry: + %at = getelementptr i8, ptr %p, i64 3 + store i8 1, ptr %at + ret void +} +)", + error, context); + if (!module) + { + report.expect(false, "ParamAccessSummary setup: the IR does not parse"); + return false; + } + + const ParamAccessSummaries summaries = computeParamAccessSummaries(*module, nullptr); + const auto of = [&](const char* name) -> const ParamAccessSummary* + { + const auto it = summaries.find(module->getFunction(name)); + return it == summaries.end() ? nullptr : &it->second; + }; + const auto writesNothing = [&](const char* name, unsigned param) + { + const ParamAccessSummary* summary = of(name); + return summary && param < summary->params.size() && + !summary->params[param].lowestWrite && !summary->params[param].highestWrite; + }; + + report.expect(writesNothing("slot_escapes", 0), + "ParamAccessSummary: a slot whose address goes to a call is not read as p " + "after the call"); + report.expect(writesNothing("slot_aliased", 0), + "ParamAccessSummary: a slot rewritten through an alias of its address is " + "not read as p"); + report.expect(writesNothing("read_before_store", 0), + "ParamAccessSummary: a load before the store of p does not read p"); + + const ParamAccessSummary* plain = of("plain_slot"); + report.expect(plain && plain->complete && plain->params.size() == 1 && + plain->params[0].highestWrite && + plain->params[0].highestWrite->offset == 3 && + plain->params[0].highestWrite->size == 2 && !plain->params[0].highestRead, + "ParamAccessSummary: a slot written once and only loaded reads p: a 2-byte " + "write at offset 3"); + + const ParamAccessSummary* partial = of("conditional_and_variable"); + report.expect(partial && partial->complete && writesNothing("conditional_and_variable", 0), + "ParamAccessSummary: a write at a variable offset or on some runs only is " + "left out, and the summary stays complete"); + + const ParamAccessSummary* walk = of("walk"); + report.expect(walk && !walk->complete && walk->params[0].lowestWrite && + walk->params[0].lowestWrite->offset == 0, + "ParamAccessSummary: rounds stopped by their budget leave the summary " + "incomplete, with the accesses found"); + const ParamAccessSummary* callsWalk = of("calls_walk"); + report.expect(callsWalk && !callsWalk->complete && callsWalk->params[0].lowestWrite, + "ParamAccessSummary: a summary that uses an incomplete one is incomplete"); + const ParamAccessSummary* far = of("far"); + report.expect(far && !far->complete && writesNothing("far", 0), + "ParamAccessSummary: an offset past the budget is left out, and the summary " + "is incomplete"); + report.expect(writesNothing("by_value", 0), + "ParamAccessSummary: a write through a byval parameter is in the callee's " + "copy, not in the caller's object"); + + // Across modules: a chain through a declaration, a weak and a twice-defined function, + // and a cycle that grows until the budget of rounds. + const auto parse = [&](const char* text) + { return llvm::parseAssemblyString(text, error, context); }; + const std::unique_ptr callers = parse(R"( +declare void @leaf(ptr) +define void @mid(ptr %p) { +entry: + %q = getelementptr i8, ptr %p, i64 2 + call void @leaf(ptr %q) + ret void +} +define void @twice(ptr %p) { +entry: + store i8 0, ptr %p + ret void +} +)"); + const std::unique_ptr callees = parse(R"( +define void @leaf(ptr %p) { +entry: + store i32 0, ptr %p + ret void +} +define weak void @weak_leaf(ptr %p) { +entry: + store i8 0, ptr %p + ret void +} +define void @twice(ptr %p) { +entry: + store i8 0, ptr %p + ret void +} +)"); + const std::unique_ptr pingModule = parse(R"( +declare void @pong(ptr) +define void @ping(ptr %p) { +entry: + %q = getelementptr i8, ptr %p, i64 1 + call void @pong(ptr %q) + ret void +} +)"); + const std::unique_ptr pongModule = parse(R"( +declare void @ping(ptr) +define void @pong(ptr %p) { +entry: + store i8 0, ptr %p + call void @ping(ptr %p) + ret void +} +)"); + if (!callers || !callees || !pingModule || !pongModule) + { + report.expect(false, "ParamAccessSummary setup: the modules do not parse"); + return false; + } + + const ParamAccessIndex chain = paramAccessesAcrossModules({callers.get(), callees.get()}); + const auto published = [](const ParamAccessIndex& index, const llvm::Module& mod, + const char* name) -> const ParamAccessSummary* + { + const llvm::Function* F = mod.getFunction(name); + const auto it = F ? index.functions.find(linkerSymbolName(*F)) : index.functions.end(); + return it == index.functions.end() ? nullptr : &it->second; + }; + const ParamAccessSummary* mid = published(chain, *callers, "mid"); + report.expect(mid && mid->complete && mid->params[0].highestWrite && + mid->params[0].highestWrite->offset == 2 && + mid->params[0].highestWrite->size == 4 && + mid->params[0].highestWrite->function == "leaf", + "ParamAccessSummary: across modules, mid writes 4 bytes at offset 2 " + "through leaf, which makes the write"); + report.expect(!published(chain, *callees, "weak_leaf") && + !published(chain, *callers, "twice"), + "ParamAccessSummary: a weak or twice-defined function publishes no summary"); + + const ParamAccessIndex cycle = + paramAccessesAcrossModules({pingModule.get(), pongModule.get()}); + const ParamAccessSummary* ping = published(cycle, *pingModule, "ping"); + report.expect(ping && !ping->complete && ping->params[0].lowestWrite && + ping->params[0].lowestWrite->offset == 1, + "ParamAccessSummary: a cycle across modules that grows to the budget of " + "rounds leaves its summaries incomplete, with the accesses found"); + return report.failures == 0; + } + bool testIndexBoundsPerAccess(TestReport& report) { using ctrace::stack::analysis::StackBufferOverflowIssue; @@ -3263,6 +3508,7 @@ int main(int argc, char** argv) (void)testTryGetConstFromValue(report); (void)testPromotedValues(report); (void)testIndexBoundsPerAccess(report); + (void)testParamAccessSummary(report); (void)testSmtEncoderMemoryModel(repoRoot, report); (void)testSmtEncoderPathCondition(repoRoot, report); #ifdef CTRACE_STACK_ENABLE_Z3_BACKEND From b4373b3a056206ca1d9a55ca30d860fbfdac8bdd Mon Sep 17 00:00:00 2001 From: Hugo Date: Sat, 7 Nov 2026 19:27:41 +0100 Subject: [PATCH 2/2] fix(stack-buffer): check what callees access through a stack array's address Since a store of an element's address stopped counting as a write (#264), `char* p = &buf[8]; f(p);` got no report when f writes *p, and #265 only follows the pointer to accesses in the same function. A call that receives the address of a stack array, computed from the array at a constant offset, is now checked against the callee's access summary. The address may reach the call directly, through the variables #265 follows, or at a constant step from such a variable (`char* p = buf; f(p + 8);`), whose offset is counted in bytes; the loads and stores through a step stay with their own GEP. An access outside the array is reported on the call as a potential overflow, with its bytes, the function that makes it and its place. The constant index of the address may be in bounds: `&buf[7]` passed to a function that writes p[1] overflows. Across the files analyzed together, the summaries are published like the size-minus-one wrappers, and reach the calls to declarations. collectAccesses reads the variables back through FunctionFacts::loadsReadingStore. callee-access-write.c and callee-access-read.c cover the six criteria of #266 for writes and for reads, steps from a variable, and multi-byte reads past the end from a valid start. callee-access-calls.c covers moves and copies of the parameter in the callee, a write before the array, and cycles of calls. check_stack_buffer_callee_cross_tu runs a chain over three files in both orders, a weak definition and a static homonym. Fixes #266 --- CHANGELOG.md | 7 + include/StackUsageAnalyzer.hpp | 4 + include/analysis/StackBufferAnalysis.hpp | 16 + run_test.py | 42 +++ src/analysis/StackBufferAnalysis.cpp | 288 +++++++++++++++--- src/analyzer/DiagnosticEmitter.cpp | 31 +- src/app/AnalyzerApp.cpp | 7 + test/bound-storage/callee-access-calls.c | 117 +++++++ test/bound-storage/callee-access-read.c | 133 ++++++++ test/bound-storage/callee-access-write.c | 133 ++++++++ .../cross-tu-callee-access-leaf.c | 14 + .../cross-tu-callee-access-mid.c | 13 + .../cross-tu-callee-access-static.c | 24 ++ .../cross-tu-callee-access-use.c | 35 +++ .../cross-tu-callee-access-weak.c | 9 + 15 files changed, 828 insertions(+), 45 deletions(-) create mode 100644 test/bound-storage/callee-access-calls.c create mode 100644 test/bound-storage/callee-access-read.c create mode 100644 test/bound-storage/callee-access-write.c create mode 100644 test/bound-storage/cross-tu-callee-access-leaf.c create mode 100644 test/bound-storage/cross-tu-callee-access-mid.c create mode 100644 test/bound-storage/cross-tu-callee-access-static.c create mode 100644 test/bound-storage/cross-tu-callee-access-use.c create mode 100644 test/bound-storage/cross-tu-callee-access-weak.c diff --git a/CHANGELOG.md b/CHANGELOG.md index f7ede44..c2ad54f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,13 @@ Notable changes are grouped by release. See [all releases](https://github.com/Co *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. +- A call that receives the address of a stack array is checked against what the callee reads or + writes through it, in the same file or in another file analyzed with it (#266): + `char* p = &buf[8]; f(p);`, where `f` writes `*p` or passes `p` to a function that does, is + reported on the call, with the bytes written and the place of the write. Only the accesses + the callee makes on every run, at a constant offset from the address it receives, count. An + access behind a condition, at a variable offset, or through a function pointer is not + followed, and gives no report. - 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/include/StackUsageAnalyzer.hpp b/include/StackUsageAnalyzer.hpp index 27d9988..20be3c4 100644 --- a/include/StackUsageAnalyzer.hpp +++ b/include/StackUsageAnalyzer.hpp @@ -26,6 +26,7 @@ namespace ctrace::stack::analysis struct ConstPointeeParamIndex; struct GlobalReadBeforeWriteSummaryIndex; struct GlobalStackFacts; + struct ParamAccessIndex; struct ResourceSummaryIndex; struct SizeMinusKWrapperIndex; struct UninitializedSummaryIndex; @@ -78,6 +79,9 @@ namespace ctrace::stack // deterministic, and the lengths they all pass to a bounded write. std::shared_ptr> deterministicFunctions; std::shared_ptr sizeMinusKWrapperIndex; + // What the external functions defined across the modules analyzed together access + // through their pointer parameters (#266). + std::shared_ptr paramAccessIndex; // Max stacks and recursion over one call graph of the modules analyzed together, which // they were prepared for: the pipeline then leaves their IR as it is. std::shared_ptr globalStackFacts; diff --git a/include/analysis/StackBufferAnalysis.hpp b/include/analysis/StackBufferAnalysis.hpp index 77a90c9..74917a7 100644 --- a/include/analysis/StackBufferAnalysis.hpp +++ b/include/analysis/StackBufferAnalysis.hpp @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -24,6 +25,19 @@ namespace ctrace::stack::analysis Global, }; + /// An access a call makes through the address it receives, from the callee's summary (#266). + struct CalleeAccessOutOfBounds + { + std::string callee; // the function the call reaches + std::int64_t begin = 0; // bytes from the array's start + std::uint64_t size = 0; // bytes + std::uint64_t arrayBytes = 0; + std::string function; // the function that makes the access + std::string file; + unsigned line = 0; + unsigned column = 0; + }; + struct StackBufferOverflowIssue { std::string funcName; @@ -45,6 +59,8 @@ namespace ctrace::stack::analysis // A loop around the access runs no iteration, so the access never runs. std::uint64_t neverRuns : 1 = false; std::uint64_t reservedFlags : 59 = 0; + // Set when inst is a call that receives the address. + std::optional calleeAccess; // Optional : helper for sync string <- vector void rebuildAliasPathString(const std::string& sep = " -> ") { diff --git a/run_test.py b/run_test.py index 1d44a59..fa54180 100755 --- a/run_test.py +++ b/run_test.py @@ -3025,6 +3025,47 @@ def check_size_minus_one_cross_tu() -> bool: ) +def check_stack_buffer_callee_cross_tu() -> bool: + """ + #266: what a function defined in another file writes or reads through a pointer parameter + reaches its callers, as within one file, whatever the sort order: a call that passes an + address one byte past a stack array, to a callee that accesses it, is reported. The accesses + follow a chain of files, and never come from a weak definition or a static homonym. + """ + return _check_cross_tu_rule( + "stack array accesses through functions defined in another file", + "bound-storage", + "cross-tu-callee-access-", + "StackBufferOverflow", + [ + ("use alone", ["use.c"], []), + ("leaf and use", ["leaf.c", "use.c"], ["read_past_end", "write_past_end"]), + ( + "chain over three files", + ["leaf.c", "mid.c", "use.c"], + ["read_past_end", "write_past_end", "write_through_relay"], + ), + ("chain without its leaf", ["mid.c", "use.c"], []), + ("weak def", ["weak.c", "use.c"], []), + ("weak beside the definition", ["leaf.c", "weak.c", "use.c"], ["read_past_end"]), + ("static homonym alone", ["static.c", "use.c"], ["use_static"]), + ( + "static homonym beside the definition", + ["leaf.c", "static.c", "use.c"], + ["read_past_end", "use_static", "write_past_end"], + ), + ], + [ + ("use sorted first", ["use.c", "leaf.c"], ["read_past_end", "write_past_end"]), + ( + "chain in reverse order", + ["use.c", "mid.c", "leaf.c"], + ["read_past_end", "write_past_end", "write_through_relay"], + ), + ], + ) + + def check_cross_tu_call_graph() -> bool: """ #157: a call to a declaration reaches the definition of its symbol in another file when that @@ -5806,6 +5847,7 @@ def record_ok(ok: bool): check_duplicate_if_cross_tu, check_global_read_before_write_identity_across_files, check_size_minus_one_cross_tu, + check_stack_buffer_callee_cross_tu, check_cross_tu_call_graph, check_null_deref_nested_inter_tu, check_integer_overflow_advanced_inter_tu, diff --git a/src/analysis/StackBufferAnalysis.cpp b/src/analysis/StackBufferAnalysis.cpp index 4073bfb..e6e04e3 100644 --- a/src/analysis/StackBufferAnalysis.cpp +++ b/src/analysis/StackBufferAnalysis.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -15,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -26,6 +28,7 @@ #include "analysis/IntRanges.hpp" #include "analysis/FunctionFacts.hpp" #include "analysis/IRValueUtils.hpp" +#include "analysis/ParamAccessSummary.hpp" #include "analysis/PromotedValues.hpp" #include "analysis/smt/SmtEncoding.hpp" #include "analysis/smt/SmtRefinement.hpp" @@ -897,16 +900,29 @@ namespace ctrace::stack::analysis return first && first->isZero(); } + /// A call that receives a pointer to the address, @p offset bytes from it. + struct CallArgument + { + llvm::CallBase* call = nullptr; + std::int64_t offset = 0; + unsigned argNo = 0; + unsigned reservedPadding = 0; + }; + /// 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. + /// one variable to the next. A store of the address is not an access to it. @p calls + /// receives the calls that take the address as an argument, with its position, also + /// at a constant step from a variable (`char* p = buf; f(p + 8);`). + /// + /// A load of a variable stands for the address only when it reads exactly the store of + /// the address to that variable (FunctionFacts::loadsReadingStore). 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 may read another store, + /// and is left out. A load reads one store, so the walk meets each load once. /// - /// 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. + /// A step from a variable is not an index of the array: the loads and stores through it + /// are left to their own GEP. Only the calls get it, with its offset in bytes. /// /// @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. @@ -914,55 +930,212 @@ namespace ctrace::stack::analysis collectAccesses(llvm::GetElementPtrInst& address, bool followVariables, const FunctionFacts& facts, llvm::SmallVectorImpl& accesses, + llvm::SmallVectorImpl& calls, std::unordered_map>& via) { using namespace llvm; + constexpr std::int64_t kMaxStepBytes = std::int64_t{1} << 40; + const DataLayout& layout = address.getModule()->getDataLayout(); - SmallVector>, 4> pointers; - pointers.emplace_back(&address, std::vector{}); + struct Pointer + { + Value* value = nullptr; + std::vector names; + std::int64_t offset = 0; // bytes from the address + // A step from a variable: only calls count. + std::uint64_t stepped : 1 = false; + std::uint64_t reservedFlags : 63 = 0; + }; + const auto pointerTo = + [](Value* value, std::vector names, std::int64_t offset, bool stepped) + { + Pointer pointer; + pointer.value = value; + pointer.names = std::move(names); + pointer.offset = offset; + pointer.stepped = stepped; + return pointer; + }; + SmallVector pointers; + pointers.push_back(pointerTo(&address, {}, 0, false)); if (address.hasName()) - pointers.back().second.push_back(address.getName().str()); + pointers.back().names.push_back(address.getName().str()); while (!pointers.empty()) { - auto [pointer, names] = pointers.pop_back_val(); - for (User* user : pointer->users()) + Pointer pointer = pointers.pop_back_val(); + for (Use& use : pointer.value->uses()) { + User* user = use.getUser(); auto* store = dyn_cast(user); - if (isa(user) || (store && store->getPointerOperand() == pointer)) + auto* call = dyn_cast(user); + const bool access = !pointer.stepped && + (isa(user) || + (store && store->getPointerOperand() == pointer.value)); + if (access || (call && call->isArgOperand(&use))) { - accesses.push_back(cast(user)); - if (pointer != &address) - via[accesses.back()] = names; + if (call) + calls.push_back({call, pointer.offset, call->getArgOperandNo(&use), 0}); + else + accesses.push_back(cast(user)); + if (pointer.value != &address) + via[cast(user)] = pointer.names; } - if (!followVariables || !store || !store->isSimple() || - store->getValueOperand() != pointer) - continue; - Value* variable = store->getPointerOperand(); - for (User* variableUser : variable->users()) + // A constant step from a variable. A GEP on the address itself is checked + // as its own address. + if (auto* step = dyn_cast(user); + step && pointer.value != &address && + use.getOperandNo() == GetElementPtrInst::getPointerOperandIndex()) { - auto* load = dyn_cast(variableUser); - if (!load || !load->isSimple() || load->getType() != pointer->getType()) + APInt delta(layout.getIndexTypeSizeInBits(step->getType()), 0); + if (!step->accumulateConstantOffset(layout, delta) || + delta.getSignificantBits() > 41) continue; - const auto* def = - dyn_cast_or_null(facts.clobberingAccess(*load)); - if (!def || def->getMemoryInst() != store) + const std::int64_t offset = pointer.offset + delta.getSExtValue(); + if (offset < -kMaxStepBytes || offset > kMaxStepBytes) continue; + std::vector stepNames; + if (step->hasName()) + stepNames.push_back(step->getName().str()); + stepNames.insert(stepNames.end(), pointer.names.begin(), + pointer.names.end()); + pointers.push_back(pointerTo(step, std::move(stepNames), offset, true)); + continue; + } + + if (!followVariables || !store || store->getValueOperand() != pointer.value) + continue; + + Value* variable = store->getPointerOperand(); + SmallVector loads; + facts.loadsReadingStore(*store, loads); + for (LoadInst* load : loads) + { 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)); + loadNames.insert(loadNames.end(), pointer.names.begin(), + pointer.names.end()); + pointers.push_back( + pointerTo(load, std::move(loadNames), pointer.offset, pointer.stepped)); } } } } + using CalleeAccesses = std::function; + + /// The bytes of the array @p gep indexes, from the start of @p alloca: the alloca, or the + /// array field of a struct the GEP selects. None when they are not fixed. + static std::optional> + indexedArrayBytes(const llvm::GetElementPtrInst& gep, const llvm::AllocaInst& alloca, + const llvm::DataLayout& layout) + { + using namespace llvm; + constexpr std::uint64_t kMaxBytes = std::uint64_t{1} << 40; + std::int64_t begin = 0; + Type* type = gep.getSourceElementType(); + if (auto* record = dyn_cast(type)) + { + // [0, field, i]: the array field. + const auto* first = + gep.getNumIndices() >= 3 ? dyn_cast(gep.getOperand(1)) : nullptr; + const auto* field = first ? dyn_cast(gep.getOperand(2)) : nullptr; + if (!first || !first->isZero() || !field || + field->getZExtValue() >= record->getNumElements()) + return std::nullopt; + const auto fieldIndex = static_cast(field->getZExtValue()); + type = record->getElementType(fieldIndex); + if (!isa(type)) + return std::nullopt; + begin = static_cast( + layout.getStructLayout(record)->getElementOffset(fieldIndex).getFixedValue()); + } + else if (!isa(type)) + { + // An element pointer from the decayed array: the whole alloca. + if (alloca.isArrayAllocation()) + return std::nullopt; + type = alloca.getAllocatedType(); + } + const TypeSize size = layout.getTypeAllocSize(type); + if (size.isScalable() || size.getFixedValue() > kMaxBytes) + return std::nullopt; + return std::make_pair(begin, size.getFixedValue()); + } + + /// Reports each call of @p calls whose callee is known to access, through the address + /// @p gep it receives, bytes outside the array (#266). @p gep is computed from @p alloca + /// itself, at a constant offset. The summary lists accesses that happen on every run of + /// the callee that returns; nothing is concluded from what it does not list. + template + static void + reportCalleeAccesses(const llvm::GetElementPtrInst& gep, const llvm::AllocaInst& alloca, + llvm::ArrayRef calls, + const CalleeAccesses& calleeAccesses, const NewReport& newReport, + std::vector& out) + { + using namespace llvm; + if (calls.empty()) + return; + const DataLayout& layout = alloca.getModule()->getDataLayout(); + const auto bytes = indexedArrayBytes(gep, alloca, layout); + APInt offset(layout.getIndexTypeSizeInBits(gep.getType()), 0); + if (!bytes || !gep.accumulateConstantOffset(layout, offset) || + offset.getSignificantBits() > 41) + return; + const std::int64_t address = offset.getSExtValue(); + const std::int64_t arrayBegin = bytes->first; + const std::uint64_t arrayBytes = bytes->second; + const std::int64_t arrayEnd = arrayBegin + static_cast(arrayBytes); + + for (const CallArgument& argument : calls) + { + CallBase& call = *argument.call; + const ParamAccessSummary* summary = calleeAccesses(call); + if (!summary || argument.argNo >= summary->params.size()) + continue; + // The highest end is checked against the array's end, the lowest offset against + // its start. + const auto report = + [&](const std::optional& access, bool write, bool below) + { + if (!access) + return; + const std::int64_t begin = address + argument.offset + access->offset; + if (below ? begin >= arrayBegin + : begin + static_cast(access->size) <= arrayEnd) + return; + StackBufferOverflowIssue issue = newReport(call); + issue.isWrite = write; + issue.isLowerBoundViolation = below; + issue.lowerBound = begin - arrayBegin; + CalleeAccessOutOfBounds callee; + callee.callee = directCallee(call)->getName().str(); + callee.begin = begin - arrayBegin; + callee.size = access->size; + callee.arrayBytes = arrayBytes; + callee.function = access->function; + callee.file = access->file; + callee.line = access->line; + callee.column = access->column; + issue.calleeAccess = std::move(callee); + out.push_back(std::move(issue)); + }; + const ParamAccessBounds& bounds = summary->params[argument.argNo]; + report(bounds.highestWrite, true, false); + report(bounds.lowestWrite, true, true); + report(bounds.highestRead, false, false); + report(bounds.lowestRead, false, true); + } + } + static void analyzeStackBufferOverflowsInFunction( llvm::Function& F, std::vector& out, const AnalysisComplexityBudgets& budgets, const StackBufferConstraintEvaluator& evaluator, - const std::function& promotedValues) + const std::function& promotedValues, + const CalleeAccesses& calleeAccesses) { using namespace llvm; @@ -1136,22 +1309,29 @@ namespace ctrace::stack::analysis GV->hasName() ? GV->getName().str() : std::string(""); } - // A constant index within bounds is safe wherever the address is used. + // A constant index within bounds is safe wherever the address is used, but + // in a call: the callee may access past the address (#266). Calls are checked + // when the address is computed from a stack array itself, at a constant + // offset. auto* CIdx = dyn_cast(idxVal); - if (CIdx && CIdx->getSExtValue() >= 0 && - static_cast(CIdx->getSExtValue()) < arraySize) + const bool constantInBounds = + CIdx && CIdx->getSExtValue() >= 0 && + static_cast(CIdx->getSExtValue()) < arraySize; + const Value* array = AI ? static_cast(AI) : GV; + const bool fromArray = GEP->getPointerOperand()->stripPointerCasts() == array; + const bool checkCalls = AI && CIdx && fromArray; + if (constantInBounds && !checkCalls) 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. + // The loads, stores and calls 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; + SmallVector calls; std::unordered_map> via; - const Value* array = AI ? static_cast(AI) : GV; - collectAccesses(*GEP, - allowPointerStoreScan && - GEP->getPointerOperand()->stripPointerCasts() == array, - facts, accesses, via); + collectAccesses(*GEP, allowPointerStoreScan && fromArray, facts, accesses, + calls, via); const bool pilotAccess = isIndexBoundsPilotAccess(*GEP, AI); const auto newReport = [&](Instruction& access) { @@ -1164,8 +1344,8 @@ namespace ctrace::stack::analysis report.inst = &access; // The index-bounds pilot may show that the access never runs: its // reports then carry the unreachable-code note. - report.neverRuns = - pilotAccess && promotedValues().indexValuesAt(access, *idxVal).kind == + report.neverRuns = pilotAccess && !isa(access) && + promotedValues().indexValuesAt(access, *idxVal).kind == IndexValues::Kind::NeverRuns; std::vector path = aliasPath; if (const auto through = via.find(&access); through != via.end()) @@ -1188,6 +1368,13 @@ namespace ctrace::stack::analysis baseIdxVal = cast->getOperand(0); } + if (checkCalls) + { + reportCalleeAccesses(*GEP, *AI, calls, calleeAccesses, newReport, out); + if (constantInBounds) + continue; + } + // 4) Constant index case, out of bounds: test[11] if (CIdx) { @@ -1502,6 +1689,20 @@ namespace ctrace::stack::analysis promoted.emplace(mod, shouldAnalyze); return *promoted; }; + // What the functions of the module access through their parameters, computed on the + // first call that needs it. + const ParamAccessIndex* elsewhere = config.paramAccessIndex.get(); + std::optional summaries; + const CalleeAccesses calleeAccesses = + [&](const llvm::CallBase& call) -> const ParamAccessSummary* + { + const llvm::Function* callee = directCallee(call); + if (!callee || callee->isIntrinsic() || (callee->isDeclaration() && !elsewhere)) + return nullptr; + if (!summaries) + summaries = computeParamAccessSummaries(mod, elsewhere); + return calleeParamAccesses(call, *summaries, elsewhere); + }; for (llvm::Function& F : mod) { @@ -1509,7 +1710,8 @@ namespace ctrace::stack::analysis continue; if (!shouldAnalyze(F)) continue; - analyzeStackBufferOverflowsInFunction(F, out, budgets, evaluator, promotedValues); + analyzeStackBufferOverflowsInFunction(F, out, budgets, evaluator, promotedValues, + calleeAccesses); } return out; diff --git a/src/analyzer/DiagnosticEmitter.cpp b/src/analyzer/DiagnosticEmitter.cpp index 7755d83..51591c7 100644 --- a/src/analyzer/DiagnosticEmitter.cpp +++ b/src/analyzer/DiagnosticEmitter.cpp @@ -4,6 +4,7 @@ #include "analyzer/LocationResolver.hpp" #include "analysis/AnalyzerUtils.hpp" #include "analysis/Reachability.hpp" +#include "mangle.hpp" #include #include @@ -379,6 +380,26 @@ namespace ctrace::stack::analyzer std::ostringstream body; DiagnosticBuilder builder; + // A call that receives the address: what the callee accesses through it (#266). + const auto describeCalleeAccess = [&](const analysis::CalleeAccessOutOfBounds& access) + { + const auto displayed = [](const std::string& symbol) + { + return ctrace_tools::isMangled(symbol) ? ctrace_tools::demangle(symbol.c_str()) + : symbol; + }; + body << "\t\t ↳ the call to '" << displayed(access.callee) << "' " + << (issue.isWrite ? "writes " : "reads ") << access.size + << (access.size == 1 ? " byte" : " bytes") << " at byte offset " + << access.begin << " (valid byte offsets: 0.." + << (access.arrayBytes ? access.arrayBytes - 1 : 0) << ")\n"; + body << "\t\t ↳ the " << (issue.isWrite ? "write" : "read") << " is in '" + << displayed(access.function) << "'"; + if (access.line != 0) + body << " at " << access.file << ":" << access.line << ":" << access.column; + body << "\n"; + }; + if (issue.isLowerBoundViolation) { builder.errCode(DescriptiveErrorCode::NegativeStackIndex) @@ -390,7 +411,9 @@ namespace ctrace::stack::analyzer << issue.arraySize << ")\n"; if (!issue.aliasPath.empty()) body << "\t\t ↳ alias path: " << issue.aliasPath << "\n"; - if (everyRun) + if (issue.calleeAccess) + describeCalleeAccess(*issue.calleeAccess); + else if (everyRun) body << "\t\t ↳ index reaches " << issue.lowerBound << " on every run of the function that returns\n"; else @@ -414,7 +437,11 @@ namespace ctrace::stack::analyzer << issue.varName << "' (size " << issue.arraySize << ")\n"; if (!issue.aliasPath.empty()) body << "\t\t ↳ alias path: " << issue.aliasPath << "\n"; - if (issue.indexIsConstant) + if (issue.calleeAccess) + { + describeCalleeAccess(*issue.calleeAccess); + } + else if (issue.indexIsConstant) { body << "\t\t ↳ constant index " << issue.indexOrUpperBound << " is out of bounds (0.." << (issue.arraySize ? issue.arraySize - 1 : 0) diff --git a/src/app/AnalyzerApp.cpp b/src/app/AnalyzerApp.cpp index 5a1cde3..1c2f267 100644 --- a/src/app/AnalyzerApp.cpp +++ b/src/app/AnalyzerApp.cpp @@ -51,6 +51,7 @@ #include "analysis/FunctionFilter.hpp" #include "analysis/GlobalReadBeforeWriteAnalysis.hpp" #include "analysis/InputPipeline.hpp" +#include "analysis/ParamAccessSummary.hpp" #include "analysis/ResourceLifetimeAnalysis.hpp" #include "analysis/SizeMinusKWrites.hpp" #include "analysis/UninitializedVarAnalysis.hpp" @@ -1105,6 +1106,12 @@ static AppStatus analyzeWithSharedModuleLoading(const std::vector& } } cfg.sizeMinusKWrapperIndex = std::move(wrappers); + + std::vector modules; + for (const auto& loaded : loadedModules) + modules.push_back(loaded.module.get()); + cfg.paramAccessIndex = std::make_shared( + analysis::paramAccessesAcrossModules(modules)); } if (needsCrossTUResourceSummaries) diff --git a/test/bound-storage/callee-access-calls.c b/test/bound-storage/callee-access-calls.c new file mode 100644 index 0000000..403cdc4 --- /dev/null +++ b/test/bound-storage/callee-access-calls.c @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: Apache-2.0 +// What a callee does with its parameter before the access: moves it, copies it, passes it on, +// also around a cycle of calls (#266). + +// The parameter moves back one byte before the write. +void write_before(char* p) +{ + p = p - 1; + *p = 1; +} + +// A copy keeps the address while the parameter changes. +void write_through_copy(char* p) +{ + char* q = p; + p = 0; + *q = 1; +} + +// ping always calls pong, which always writes p[1]. +void ping(char* p, int n); + +void pong(char* p, int n) +{ + p[1] = 0; + if (n > 0) + ping(p, n - 1); +} + +void ping(char* p, int n) +{ + pong(p, n); +} + +// The write at p runs on every call; the recursive call only on some. +void fill_down(char* p, int n) +{ + *p = 0; + if (n > 0) + fill_down(p + 1, n - 1); +} + +// The write lands one byte before the array. +void write_before_start(void) +{ + char buf[8] = {0}; + write_before(buf); +} + +// The write lands on the first byte. +void write_before_second(void) +{ + char buf[8] = {0}; + write_before(buf + 1); +} + +void write_copy_past_end(void) +{ + char buf[8] = {0}; + write_through_copy(buf + 8); +} + +// p[1] from buf + 7. +void ping_past_end(void) +{ + char buf[8] = {0}; + ping(buf + 7, 2); +} + +// p[1] from buf + 6 is the last byte, on every call of the cycle. +void ping_in_bounds(void) +{ + char buf[8] = {0}; + ping(buf + 6, 2); +} + +void fill_down_past_end(void) +{ + char buf[8] = {0}; + fill_down(buf + 8, 0); +} + +void fill_down_last_byte(void) +{ + char buf[8] = {0}; + fill_down(buf + 7, 0); +} + +// strict-expectation-details: true + +// at line 47, column 5 +// [!!] potential negative index on variable 'buf' (size 8) +// ↳ alias path: buf +// ↳ the call to 'write_before' writes 1 byte at byte offset -1 (valid byte offsets: 0..7) +// ↳ the write is in 'write_before' at callee-access-calls.c:9:8 +// ↳ (this is a write access) + +// at line 60, column 5 +// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 8) +// ↳ alias path: buf -> arraydecay +// ↳ the call to 'write_through_copy' writes 1 byte at byte offset 8 (valid byte offsets: 0..7) +// ↳ the write is in 'write_through_copy' at callee-access-calls.c:17:8 +// ↳ (this is a write access) + +// at line 67, column 5 +// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 8) +// ↳ alias path: buf -> arraydecay +// ↳ the call to 'ping' writes 1 byte at byte offset 8 (valid byte offsets: 0..7) +// ↳ the write is in 'pong' at callee-access-calls.c:25:10 +// ↳ (this is a write access) + +// at line 80, column 5 +// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 8) +// ↳ alias path: buf -> arraydecay +// ↳ the call to 'fill_down' writes 1 byte at byte offset 8 (valid byte offsets: 0..7) +// ↳ the write is in 'fill_down' at callee-access-calls.c:38:8 +// ↳ (this is a write access) diff --git a/test/bound-storage/callee-access-read.c b/test/bound-storage/callee-access-read.c new file mode 100644 index 0000000..f2d83a8 --- /dev/null +++ b/test/bound-storage/callee-access-read.c @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: Apache-2.0 +// A call that receives the address of a stack array is checked against what the callee reads +// through it on every run, with the size of each read (#266). + +#include + +char read_first(const char* p) +{ + return *p; +} + +char relay_read(const char* p) +{ + const char* q = p; + return read_first(q); +} + +char read_if(const char* p, int enabled) +{ + return enabled ? *p : 0; +} + +// Four bytes from the address. +int read_four(const char* p) +{ + int value = 0; + memcpy(&value, p, sizeof value); + return value; +} + +// Two ints from the address. +int combine_pair(const int* p) +{ + return p[0] ^ p[1]; +} + +// A one-byte read through buf + 7 stays in bounds. +char read_last_byte(void) +{ + char buf[8] = {0}; + return read_first(buf + 7); +} + +// A one-byte read through buf + 8. +char read_past_end(void) +{ + char buf[8] = {0}; + return read_first(buf + 8); +} + +// The same read after copies in the caller and a relay that copies again. +char read_past_end_through_copies(void) +{ + char buf[8] = {0}; + const char* p = &buf[8]; + const char* q = p; + return relay_read(q); +} + +// The pointer moves to a large enough object before the call. +char reassigned_before_read(void) +{ + char buf[8] = {0}; + char big[16] = {0}; + const char* p = &buf[8]; + p = &big[8]; + return relay_read(p); +} + +// The callee reads only when its second argument is set, and the call clears it. +char guard_off_before_read(void) +{ + char buf[8] = {0}; + return read_if(&buf[8], 0); +} + +// Four bytes from buf + 4 end at the last byte. +int read_four_in_bounds(void) +{ + char buf[8] = {0}; + return read_four(buf + 4); +} + +// Four bytes from buf + 6: the start is valid, the end is not. +int read_four_past_end(void) +{ + char buf[8] = {0}; + return read_four(buf + 6); +} + +// Two ints from the third element end at the last one. +int combine_last_pair(void) +{ + int values[4] = {0}; + return combine_pair(&values[2]); +} + +// Two ints from the last element: the second one is past the end. +int combine_pair_past_end(void) +{ + int values[4] = {0}; + return combine_pair(&values[3]); +} + +// strict-expectation-details: true + +// at line 48, column 12 +// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 8) +// ↳ alias path: buf -> arraydecay +// ↳ the call to 'read_first' reads 1 byte at byte offset 8 (valid byte offsets: 0..7) +// ↳ the read is in 'read_first' at callee-access-read.c:9:12 +// ↳ (this is a read access) + +// at line 57, column 12 +// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 8) +// ↳ alias path: buf -> arrayidx -> p -> q +// ↳ the call to 'relay_read' reads 1 byte at byte offset 8 (valid byte offsets: 0..7) +// ↳ the read is in 'read_first' at callee-access-read.c:9:12 +// ↳ (this is a read access) + +// at line 88, column 12 +// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 8) +// ↳ alias path: buf -> arraydecay +// ↳ the call to 'read_four' reads 4 bytes at byte offset 6 (valid byte offsets: 0..7) +// ↳ the read is in 'read_four' at callee-access-read.c:27:5 +// ↳ (this is a read access) + +// at line 102, column 12 +// [ !!Warn ] potential stack buffer overflow on variable 'values' (size 4) +// ↳ alias path: values +// ↳ the call to 'combine_pair' reads 4 bytes at byte offset 16 (valid byte offsets: 0..15) +// ↳ the read is in 'combine_pair' at callee-access-read.c:34:19 +// ↳ (this is a read access) diff --git a/test/bound-storage/callee-access-write.c b/test/bound-storage/callee-access-write.c new file mode 100644 index 0000000..f676213 --- /dev/null +++ b/test/bound-storage/callee-access-write.c @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: Apache-2.0 +// A call that receives the address of a stack array is checked against what the callee writes +// through it on every run (#266). The report is on the call, and names the write in the callee. + +void write_first(char* p) +{ + *p = 42; +} + +void relay(char* p) +{ + char* q = p; + write_first(q); +} + +void keep_address(char* p) +{ + (void)p; +} + +void write_if(char* p, int enabled) +{ + if (enabled) + *p = 1; +} + +// The address one past the end, passed but never dereferenced. +void pass_past_end_only(void) +{ + char buf[8] = {0}; + keep_address(buf + 8); +} + +// A one-byte write through buf + 7 stays in bounds. +void write_last_byte(void) +{ + char buf[8] = {0}; + write_first(buf + 7); +} + +// A one-byte write through buf + 8. +void write_past_end(void) +{ + char buf[8] = {0}; + write_first(buf + 8); +} + +// The same write after copies in the caller and a relay that copies again. +void write_past_end_through_copies(void) +{ + char buf[8] = {0}; + char* p = &buf[8]; + char* q = p; + relay(q); +} + +// The pointer moves to a large enough object before the call. +void reassigned_before_call(void) +{ + char buf[8] = {0}; + char big[16] = {0}; + char* p = &buf[8]; + p = &big[8]; + relay(p); +} + +// The callee writes only when its second argument is set, and the call clears it. +void guard_off_in_callee(void) +{ + char buf[8] = {0}; + write_if(&buf[8], 0); +} + +// The call itself depends on the caller's guard, which may hold. +void call_under_guard(int n) +{ + char buf[8] = {0}; + if (n > 4) + write_first(&buf[8]); +} + +// The address of the whole array: the callee writes at its start. +void write_array_start(void) +{ + char buf[8] = {0}; + write_first(buf); +} + +// A constant step from a pointer variable that holds the array: buf + 8. +void write_after_step(void) +{ + char buf[8] = {0}; + char* p = buf; + write_first(p + 8); +} + +// One step back from the end: the last byte. +void write_before_end(void) +{ + char buf[8] = {0}; + char* end = buf + 8; + write_first(end - 1); +} + +// strict-expectation-details: true + +// at line 45, column 5 +// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 8) +// ↳ alias path: buf -> arraydecay +// ↳ the call to 'write_first' writes 1 byte at byte offset 8 (valid byte offsets: 0..7) +// ↳ the write is in 'write_first' at callee-access-write.c:7:8 +// ↳ (this is a write access) + +// at line 54, column 5 +// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 8) +// ↳ alias path: buf -> arrayidx -> p -> q +// ↳ the call to 'relay' writes 1 byte at byte offset 8 (valid byte offsets: 0..7) +// ↳ the write is in 'write_first' at callee-access-write.c:7:8 +// ↳ (this is a write access) + +// at line 79, column 9 +// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 8) +// ↳ alias path: buf +// ↳ the call to 'write_first' writes 1 byte at byte offset 8 (valid byte offsets: 0..7) +// ↳ the write is in 'write_first' at callee-access-write.c:7:8 +// ↳ (this is a write access) + +// at line 94, column 5 +// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 8) +// ↳ alias path: buf -> arraydecay -> p -> add.ptr +// ↳ the call to 'write_first' writes 1 byte at byte offset 8 (valid byte offsets: 0..7) +// ↳ the write is in 'write_first' at callee-access-write.c:7:8 +// ↳ (this is a write access) diff --git a/test/bound-storage/cross-tu-callee-access-leaf.c b/test/bound-storage/cross-tu-callee-access-leaf.c new file mode 100644 index 0000000..9e0f9dc --- /dev/null +++ b/test/bound-storage/cross-tu-callee-access-leaf.c @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Callees of cross-tu-callee-access-use.c (#266). Alone, nothing calls them with an array, and +// nothing is reported. run_test.py checks each pairing. + +void write_first(char* p) +{ + *p = 42; +} + +char read_first(const char* p) +{ + return *p; +} diff --git a/test/bound-storage/cross-tu-callee-access-mid.c b/test/bound-storage/cross-tu-callee-access-mid.c new file mode 100644 index 0000000..f07269f --- /dev/null +++ b/test/bound-storage/cross-tu-callee-access-mid.c @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// A relay between cross-tu-callee-access-use.c and cross-tu-callee-access-leaf.c (#266): it +// copies its parameter and passes it on. Alone, write_first() is a declaration, and nothing is +// reported. + +void write_first(char* p); + +void relay(char* p) +{ + char* q = p; + write_first(q); +} diff --git a/test/bound-storage/cross-tu-callee-access-static.c b/test/bound-storage/cross-tu-callee-access-static.c new file mode 100644 index 0000000..e47b65b --- /dev/null +++ b/test/bound-storage/cross-tu-callee-access-static.c @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// A static write_first() (#266) that writes further: it serves the calls of this file only, and +// not those of cross-tu-callee-access-use.c. + +static void write_first(char* p) +{ + p[100] = 42; +} + +void use_static(void) +{ + char buf[8] = {0}; + write_first(buf); +} + +// strict-expectation-details: true + +// at line 14, column 5 +// [ !!Warn ] potential stack buffer overflow on variable 'buf' (size 8) +// ↳ alias path: buf +// ↳ the call to 'write_first' writes 1 byte at byte offset 100 (valid byte offsets: 0..7) +// ↳ the write is in 'write_first' at cross-tu-callee-access-static.c:8:12 +// ↳ (this is a write access) diff --git a/test/bound-storage/cross-tu-callee-access-use.c b/test/bound-storage/cross-tu-callee-access-use.c new file mode 100644 index 0000000..4fdedb6 --- /dev/null +++ b/test/bound-storage/cross-tu-callee-access-use.c @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Callers of functions defined in other files (#266). Alone, they are declarations, and nothing +// is reported. With cross-tu-callee-access-leaf.c, a write or a read one byte past buf is +// reported at the call, as within one file; with cross-tu-callee-access-mid.c too, so is the +// write through relay(). run_test.py checks each pairing. + +void write_first(char* p); +char read_first(const char* p); +void relay(char* p); + +void write_past_end(void) +{ + char buf[8] = {0}; + write_first(buf + 8); +} + +void write_last_byte(void) +{ + char buf[8] = {0}; + write_first(buf + 7); +} + +char read_past_end(void) +{ + char buf[8] = {0}; + return read_first(buf + 8); +} + +void write_through_relay(void) +{ + char buf[8] = {0}; + char* p = &buf[8]; + relay(p); +} diff --git a/test/bound-storage/cross-tu-callee-access-weak.c b/test/bound-storage/cross-tu-callee-access-weak.c new file mode 100644 index 0000000..5afa216 --- /dev/null +++ b/test/bound-storage/cross-tu-callee-access-weak.c @@ -0,0 +1,9 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// A weak write_first() (#266): the linker may keep another definition, so its accesses do not +// reach the callers in cross-tu-callee-access-use.c. + +__attribute__((weak)) void write_first(char* p) +{ + *p = 42; +}