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