From e0e85068a8f19376f7b9f4b64697b7ca2673665f Mon Sep 17 00:00:00 2001 From: Hugo Date: Wed, 11 Nov 2026 23:13:45 +0100 Subject: [PATCH 1/6] feat(ownership): add a Use event and list the uses of released resources A Use event states that the resource a location holds must be live there. It leaves the state unchanged, so summaries and the cross-TU cache do not move. releasedUses() lists the uses whose resource may be released and is not uncertain, or is released on every path where it exists. An incomplete solve lists nothing. --- .../analysis/ownership/OwnershipEngine.hpp | 14 ++ include/analysis/ownership/OwnershipFacts.hpp | 5 +- src/analysis/ownership/OwnershipEngine.cpp | 47 +++++++ test/unit/ownership_engine_unit_tests.cpp | 129 ++++++++++++++++++ 4 files changed, 194 insertions(+), 1 deletion(-) diff --git a/include/analysis/ownership/OwnershipEngine.hpp b/include/analysis/ownership/OwnershipEngine.hpp index 750384b5..d8a79d14 100644 --- a/include/analysis/ownership/OwnershipEngine.hpp +++ b/include/analysis/ownership/OwnershipEngine.hpp @@ -48,6 +48,20 @@ namespace ctrace::stack::analysis::ownership const OwnershipFacts& facts, const OwnershipResult& result, std::uint32_t block, const std::function& cb); + /// A Use event that may read a released resource. + struct ReleasedUse + { + const Event* use = nullptr; // points into the facts + ResourceId resource = 0; + std::uint32_t reservedPadding = 0; + }; + + /// Every (Use, resource) where the resource the Use's `src` may hold may be Released and is + /// not uncertain, or is Released on every path where it exists (uncertain or not), read off + /// the stabilised states (edges included). Empty when incomplete. + std::vector releasedUses(const OwnershipFacts& facts, + const OwnershipResult& result); + /// Union of t[s] over the states s in `in`. StateSet applyTransformer(const ParamTransformer& t, StateSet in); diff --git a/include/analysis/ownership/OwnershipFacts.hpp b/include/analysis/ownership/OwnershipFacts.hpp index c77abcf7..01770457 100644 --- a/include/analysis/ownership/OwnershipFacts.hpp +++ b/include/analysis/ownership/OwnershipFacts.hpp @@ -152,7 +152,10 @@ namespace ctrace::stack::analysis::ownership // lists locations whose resources are uncertain *at this exit* // Edge event: the conditional contract of acquisition `site` is decided here; // unknownValue = true means "did not acquire" (resource NotOwned, holders null). - ContractResolved + ContractResolved, + // The resource `src` holds must be live here (state unchanged); `dst` is the + // location to name it by (the slot the pointer was read from). + Use }; std::vector args; diff --git a/src/analysis/ownership/OwnershipEngine.cpp b/src/analysis/ownership/OwnershipEngine.cpp index 31577958..43e7c91b 100644 --- a/src/analysis/ownership/OwnershipEngine.cpp +++ b/src/analysis/ownership/OwnershipEngine.cpp @@ -263,10 +263,57 @@ namespace ctrace::stack::analysis::ownership } case Event::Kind::Exit: + case Event::Kind::Use: break; } } + std::vector releasedUses(const OwnershipFacts& facts, + const OwnershipResult& result) + { + std::vector found; + if (result.incomplete) + return found; + const auto check = [&](const Event& e, const AbstractState& s) + { + if (e.kind != Event::Kind::Use) + return; + for (const ResourceId r : s.locations[e.src].resources) + { + // Released on every path where it exists: a certain release through an exact + // handle came last, whatever was uncertain before it. + StateSet existing = s.resources[r]; + existing.bits &= static_cast(~StateSet::of(OwnState::NotOwned).bits); + if (s.resources[r].has(OwnState::Released) && + (!s.uncertain[r] || existing.isOnly(OwnState::Released))) + found.push_back({&e, r}); + } + }; + for (std::uint32_t block = 0; block < facts.blocks.size(); ++block) + { + if (block >= result.in.size() || !result.in[block].reached) + continue; + AbstractState state = result.in[block]; + for (const Event& e : facts.blocks[block].events) + { + check(e, state); + applyEvent(facts, e, state); + } + for (const Edge& edge : facts.edges) + { + if (edge.from != block) + continue; + AbstractState along = state; + for (const Event& e : edge.events) + { + check(e, along); + applyEvent(facts, e, along); + } + } + } + return found; + } + OwnershipResult solve(const OwnershipFacts& facts, const AbstractState& entry, unsigned iterationLimit) { diff --git a/test/unit/ownership_engine_unit_tests.cpp b/test/unit/ownership_engine_unit_tests.cpp index b3150cda..828cb556 100644 --- a/test/unit/ownership_engine_unit_tests.cpp +++ b/test/unit/ownership_engine_unit_tests.cpp @@ -139,6 +139,16 @@ namespace e.call.params.push_back({loc, t}); return e; } + /// A use of what `src` holds; `id` stands for the instruction, to tell uses apart. + Event use(LocationId src, std::uint32_t id) + { + Event e; + e.kind = Event::Kind::Use; + e.src = src; + e.dst = src; + e.instructionIndex = id; + return e; + } OwnershipFacts facts(std::size_t blocks, std::vector edges, std::size_t locations, std::uint32_t sites) @@ -981,6 +991,124 @@ namespace } return r.failures == 0; } + // Location 0 is the owner `p`, location 1 an alias `q`. + bool testUses(TestReport& r) + { + using Ids = std::vector; + const ResourceId n0 = newInstanceOf(0); + const ResourceId o0 = oldInstancesOf(0); + const auto reported = [](const OwnershipFacts& f, unsigned limit = 0) + { + const OwnershipResult res = solve(f, entryOf(f), limit); + std::vector> out; + for (const ReleasedUse& u : releasedUses(f, res)) + out.push_back({u.use->instructionIndex, u.resource}); + return out; + }; + const auto ids = [&](const OwnershipFacts& f) + { + Ids out; + for (const auto& [id, resource] : reported(f)) + out.push_back(id); + return out; + }; + + { + OwnershipFacts with = facts(1, {}, 1, 1); + with.blocks[0].events = {acquire(0, 0), use(0, 1), release(0), use(0, 2), exit()}; + OwnershipFacts without = facts(1, {}, 1, 1); + without.blocks[0].events = {acquire(0, 0), release(0), exit()}; + const OwnershipResult a = solve(with, entryOf(with)); + const OwnershipResult b = solve(without, entryOf(without)); + r.expect(a.out == b.out && a.exits.size() == 1 && a.exits[0].state == b.exits[0].state, + "Uses: a use leaves the state unchanged"); + } + { + OwnershipFacts f = facts(1, {}, 1, 1); + f.blocks[0].events = {acquire(0, 0), use(0, 1), release(0), use(0, 2), exit()}; + const auto found = reported(f); + r.expect(found.size() == 1 && found[0].first == 2 && found[0].second == n0, + "Uses: only the use after the release is reported"); + } + { + // q = p; free(p); p = NULL; use p; use q. + OwnershipFacts f = facts(1, {}, 2, 1); + f.blocks[0].events = {acquire(0, 0), copy(1, 0), release(0), overwrite(0), + use(0, 1), use(1, 2), exit()}; + r.expect(ids(f) == Ids{2}, + "Uses: the alias still holds the released resource, the cleared owner not"); + } + { + // free(p); p = acquire(); use p. + OwnershipFacts f = facts(1, {}, 1, 1); + f.blocks[0].events = {acquire(0, 0), release(0), acquire(0, 0), use(0, 1), exit()}; + r.expect(ids(f).empty(), "Uses: a reacquired owner holds the new, live instance"); + } + { + // saved = p; p = acquire(); free(saved); use saved. + OwnershipFacts f = facts(1, {}, 2, 1); + f.blocks[0].events = {acquire(0, 0), copy(1, 0), acquire(0, 0), + release(1), use(1, 1), exit()}; + const auto found = reported(f); + r.expect(found.size() == 1 && found[0].first == 1 && found[0].second == o0, + "Uses: an older instance released through its alias is reported"); + } + { + // Loop body: p = acquire(); use p; free(p). Only earlier instances are released. + OwnershipFacts f = facts(3, {edge(0, 1), edge(1, 1), edge(1, 2)}, 1, 1); + f.blocks[1].events = {acquire(0, 0), use(0, 1), release(0)}; + f.blocks[2].events = {exit()}; + r.expect(ids(f).empty(), "Uses: an instance released on an earlier iteration is " + "not the one used"); + } + { + // 0 → 1 (free) → 3, 0 → 2 → 3; 3: use p. + OwnershipFacts f = + facts(4, {edge(0, 1, {release(0)}), edge(0, 2), edge(1, 3), edge(2, 3)}, 1, 1); + f.blocks[0].events = {acquire(0, 0)}; + f.blocks[3].events = {use(0, 1), exit()}; + r.expect(ids(f) == Ids{1}, "Uses: a release on one path is a potential use after " + "release"); + } + { + OwnershipFacts f = facts(1, {}, 1, 1); + f.blocks[0].events = {acquire(0, 0), addressEscape(0), release(0), use(0, 1), exit()}; + r.expect(ids(f).empty(), "Uses: an uncertain resource is not judged"); + } + { + Event unknown; + unknown.kind = Event::Kind::UnknownCall; + unknown.args = {0}; + OwnershipFacts f = facts(1, {}, 1, 1); + f.blocks[0].events = {acquire(0, 0), unknown, use(0, 1), exit()}; + r.expect(ids(f).empty(), + "Uses: an unknown call that may release does not make later uses reported"); + f.blocks[0].events = {acquire(0, 0), unknown, release(0), use(0, 1), exit()}; + r.expect(ids(f) == Ids{1}, + "Uses: a certain release is judged even after an unknown call"); + } + { + // A merged return block split onto its incoming edge. + OwnershipFacts f = facts(2, {edge(0, 1, {use(0, 1)})}, 1, 1); + f.blocks[0].events = {acquire(0, 0), release(0)}; + f.blocks[1].events = {exit()}; + r.expect(ids(f) == Ids{1}, "Uses: a use on an edge is judged with the edge's state"); + } + { + OwnershipFacts f = facts(2, {}, 1, 1); + f.blocks[0].events = {acquire(0, 0), release(0), exit()}; + f.blocks[1].events = {use(0, 1)}; + r.expect(ids(f).empty(), "Uses: an unreachable use is not reported"); + } + { + OwnershipFacts f = facts(3, {edge(0, 1), edge(1, 1), edge(1, 2)}, 1, 1); + f.blocks[1].events = {acquire(0, 0), release(0), use(0, 1)}; + f.blocks[2].events = {exit()}; + r.expect(reported(f, /*limit=*/1).empty(), + "Uses: nothing is reported from an incomplete solve"); + } + return r.failures == 0; + } } // namespace int main(int, char**) @@ -988,6 +1116,7 @@ int main(int, char**) TestReport report; (void)testDomain(report); (void)testEngine(report); + (void)testUses(report); (void)testContracts(report); (void)testEdgeExits(report); (void)testExitUncertainty(report); From 7f411c9d4e442ef0430700b18f56fc08cf6eba03 Mon Sep 17 00:00:00 2001 From: Hugo Date: Wed, 11 Nov 2026 23:13:45 +0100 Subject: [PATCH 2/6] fix(resource-lifetime): report use-after-release from the ownership engine The rule counted any read or write of a variable after a release as a use, so `p = NULL` after free(p) was reported and `*q` through a copy was missed. The collector now emits Use for memory accesses through a pointer, pointer arguments the callee likely dereferences, and returned pointers. Copies, writes into the slot, comparisons and the released argument are not uses. The diagnostic comes from the engine run that already reports leaks. The counter path for this rule is removed. 05_null_deref.c: the two use-after-release expectations, on `p = NULL` and on the null dereference after it, become a negative assertion. The null dereference stays pinned. Part of #104. --- src/analysis/ResourceLifetimeAnalysis.cpp | 306 ++---------------- src/analysis/ResourceLifetimeInternal.hpp | 6 + .../ownership/ResourceFactCollector.cpp | 109 ++++++- .../delete-twice-with-destructor.cpp | 22 ++ .../use-after-release-alias.c | 113 +++++++ .../security/null-dereference/05_null_deref.c | 10 +- test/unit/analyzer_module_unit_tests.cpp | 29 ++ test/unit/ownership_collector_input.c | 22 ++ 8 files changed, 317 insertions(+), 300 deletions(-) create mode 100644 test/resource-lifetime/delete-twice-with-destructor.cpp create mode 100644 test/resource-lifetime/use-after-release-alias.c diff --git a/src/analysis/ResourceLifetimeAnalysis.cpp b/src/analysis/ResourceLifetimeAnalysis.cpp index 089db923..34ec3040 100644 --- a/src/analysis/ResourceLifetimeAnalysis.cpp +++ b/src/analysis/ResourceLifetimeAnalysis.cpp @@ -13,8 +13,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -22,7 +24,6 @@ #include #include #include -#include #include #include #include @@ -92,7 +93,6 @@ namespace ctrace::stack::analysis std::string resourceKind; std::string funcName; const llvm::Instruction* firstAcquireInst = nullptr; - std::vector releaseInsts; int acquires = 0; int releases = 0; OwnershipState ownership = OwnershipState::Unknown; @@ -1032,28 +1032,8 @@ namespace ctrace::stack::analysis name == "read"; } - static bool callArgumentIsDirectReleaseArg(const llvm::CallBase& CB, - const llvm::Function* callee, - const ResourceModel& model, unsigned argIndex) - { - if (!callee) - return false; - - for (const ResourceRule& rule : model.rules) - { - if (rule.action != RuleAction::ReleaseArg) - continue; - if (rule.argIndex != argIndex) - continue; - if (ruleMatchesFunction(rule, *callee)) - return true; - } - - return false; - } - - static bool callArgumentLikelyDereferenced(const llvm::CallBase& CB, - const llvm::Function* callee, unsigned argIndex) + bool callArgumentLikelyDereferenced(const llvm::CallBase& CB, const llvm::Function* callee, + unsigned argIndex) { if (argIndex >= CB.arg_size()) return false; @@ -1086,135 +1066,6 @@ namespace ctrace::stack::analysis return false; } - static bool instructionMayReach(const llvm::Instruction& from, const llvm::Instruction& to) - { - if (from.getFunction() != to.getFunction()) - return false; - if (&from == &to) - return true; - if (from.getParent() == to.getParent()) - return from.comesBefore(&to); - return llvm::isPotentiallyReachable(&from, &to); - } - - static bool - valueFeedsOnlyDirectReleaseArgs(const llvm::Value* value, const ResourceModel& model, - llvm::SmallPtrSet& visited, - bool& sawReleaseUse, unsigned depth = 0) - { - if (!value || depth > 10) - return false; - - value = value->stripPointerCasts(); - if (!visited.insert(value).second) - return true; - - bool sawMeaningfulUse = false; - for (const llvm::Use& U : value->uses()) - { - const llvm::User* user = U.getUser(); - if (const auto* II = llvm::dyn_cast(user)) - { - if (llvm::isa(II) || - llvm::isa(II)) - { - continue; - } - } - - if (const auto* CB = llvm::dyn_cast(user)) - { - const llvm::Function* callee = directCallee(*CB); - if (!callee) - return false; - - bool callUseIsReleaseOnly = false; - for (unsigned argIdx = 0; argIdx < CB->arg_size(); ++argIdx) - { - if (CB->getArgOperand(argIdx)->stripPointerCasts() != value) - continue; - - sawMeaningfulUse = true; - if (!callArgumentIsDirectReleaseArg(*CB, callee, model, argIdx)) - return false; - - callUseIsReleaseOnly = true; - sawReleaseUse = true; - } - if (!callUseIsReleaseOnly) - return false; - continue; - } - - if (const auto* CI = llvm::dyn_cast(user)) - { - sawMeaningfulUse = true; - if (!valueFeedsOnlyDirectReleaseArgs(CI, model, visited, sawReleaseUse, - depth + 1)) - { - return false; - } - continue; - } - if (const auto* BC = llvm::dyn_cast(user)) - { - sawMeaningfulUse = true; - if (!valueFeedsOnlyDirectReleaseArgs(BC, model, visited, sawReleaseUse, - depth + 1)) - { - return false; - } - continue; - } - if (const auto* GEP = llvm::dyn_cast(user)) - { - sawMeaningfulUse = true; - if (!valueFeedsOnlyDirectReleaseArgs(GEP, model, visited, sawReleaseUse, - depth + 1)) - { - return false; - } - continue; - } - if (const auto* PN = llvm::dyn_cast(user)) - { - sawMeaningfulUse = true; - if (!valueFeedsOnlyDirectReleaseArgs(PN, model, visited, sawReleaseUse, - depth + 1)) - { - return false; - } - continue; - } - if (const auto* Sel = llvm::dyn_cast(user)) - { - sawMeaningfulUse = true; - if (!valueFeedsOnlyDirectReleaseArgs(Sel, model, visited, sawReleaseUse, - depth + 1)) - { - return false; - } - continue; - } - - // Any store/comparison/arithmetic or unknown use means this load - // participates in logic other than a pure release call path. - return false; - } - - return sawMeaningfulUse && sawReleaseUse; - } - - static bool loadFeedsOnlyDirectReleaseArgs(const llvm::LoadInst& load, - const ResourceModel& model) - { - llvm::SmallPtrSet visited; - bool sawReleaseUse = false; - if (!valueFeedsOnlyDirectReleaseArgs(&load, model, visited, sawReleaseUse)) - return false; - return sawReleaseUse; - } - static const llvm::AllocaInst* findOwnerLocalPointerSlotForReleasedHandleArg(const llvm::Value* handleArg, const llvm::DataLayout& DL) @@ -2406,11 +2257,11 @@ namespace ctrace::stack::analysis namespace { /// Runs the ownership engine on one function and turns its stabilised states into - /// MissingRelease / ReferenceLost / AnalysisIncomplete issues (spec §8). - void analyzeMissingReleaseWithEngine(const llvm::Function& F, const ResourceModel& model, - const ownership::SummaryLookup& summaries, - const llvm::DataLayout& DL, - std::vector& issues) + /// UseAfterRelease / MissingRelease / ReferenceLost / AnalysisIncomplete issues (spec §8). + void analyzeWithOwnershipEngine(const llvm::Function& F, const ResourceModel& model, + const ownership::SummaryLookup& summaries, + const llvm::DataLayout& DL, + std::vector& issues) { using namespace ownership; @@ -2470,6 +2321,23 @@ namespace ctrace::stack::analysis return; } + // A use of a resource that may be released: one issue per site and source line. + std::set> usedAt; + for (const ReleasedUse& u : releasedUses(collected.facts, result)) + { + const llvm::Instruction* at = collected.eventInstructions[u.use->instructionIndex]; + const llvm::DebugLoc loc = at->getDebugLoc(); + if (!usedAt.insert({u.resource / 2u, loc ? loc.getLine() : 0u, loc ? nullptr : at}) + .second) + continue; + ResourceLifetimeIssue issue = + issueFor(u.resource, ResourceLifetimeIssueKind::UseAfterRelease); + issue.inst = at; + if (collected.locationIsSlot[u.use->dst]) + issue.handleName = collected.locationNames[u.use->dst]; + issues.push_back(std::move(issue)); + } + const std::size_t resourceCount = entry.resources.size(); // One issue per acquisition site (spec §8): both instances of a site share it. std::vector reportedSite(collected.facts.siteCount, false); @@ -2629,7 +2497,6 @@ namespace ctrace::stack::analysis std::unordered_map localStates; std::unordered_map unknownAcquireEscapeCache; std::unordered_set interprocUncertaintyReported; - std::unordered_set useAfterReleaseReported; std::unordered_set releasedEscapeReported; auto trackAcquire = [&](const StorageKey& storage, const std::string& resourceKind, const llvm::Instruction* anchorInst) @@ -2663,76 +2530,6 @@ namespace ctrace::stack::analysis } }; - auto sameLocalStorage = [&](const StorageKey& lhs, const StorageKey& rhs) - { - if (lhs.scope != StorageScope::Local || rhs.scope != StorageScope::Local) - return false; - if (lhs.key == rhs.key) - return true; - return lhs.localAlloca && rhs.localAlloca && lhs.localAlloca == rhs.localAlloca && - lhs.offset == rhs.offset; - }; - - auto reportUseAfterReleaseIfNeeded = - [&](const StorageKey& storage, const llvm::Instruction* anchorInst) - { - if (!storage.valid() || storage.scope != StorageScope::Local || !anchorInst) - return; - - for (const auto& entry : localStates) - { - const LocalHandleState& state = entry.second; - if (!sameLocalStorage(state.storage, storage)) - continue; - if (state.acquires <= 0) - continue; - if (state.releases <= 0) - continue; - - bool reachableRelease = false; - for (const llvm::Instruction* releaseInst : state.releaseInsts) - { - if (!releaseInst) - continue; - if (instructionMayReach(*releaseInst, *anchorInst)) - { - reachableRelease = true; - break; - } - } - if (!reachableRelease) - continue; - - std::ostringstream dedupKey; - if (state.storage.localAlloca) - { - dedupKey << static_cast(state.storage.localAlloca) << ":" - << state.storage.offset; - } - else - { - dedupKey << state.storage.key; - } - dedupKey << "|"; - if (const llvm::DebugLoc loc = anchorInst->getDebugLoc()) - dedupKey << loc.getLine(); - else - dedupKey << static_cast(anchorInst); - if (!useAfterReleaseReported.insert(dedupKey.str()).second) - continue; - - ResourceLifetimeIssue issue; - issue.funcName = F.getName().str(); - issue.resourceKind = state.resourceKind; - issue.handleName = state.storage.displayName.empty() - ? std::string("") - : state.storage.displayName; - issue.inst = anchorInst; - issue.kind = ResourceLifetimeIssueKind::UseAfterRelease; - issues.push_back(std::move(issue)); - } - }; - auto reportReleasedHandleEscapesIfNeeded = [&](const llvm::Value* releasedHandleValue, const std::string& resourceKind, const llvm::Instruction* anchorInst) @@ -2812,8 +2609,6 @@ namespace ctrace::stack::analysis } state.releases += 1; - if (anchorInst) - state.releaseInsts.push_back(anchorInst); if (state.releases > state.acquires) { if (state.acquires == 0 && state.storage.localAlloca) @@ -2914,34 +2709,6 @@ namespace ctrace::stack::analysis { for (llvm::Instruction& I : BB) { - if (const auto* LI = llvm::dyn_cast(&I)) - { - if (loadFeedsOnlyDirectReleaseArgs(*LI, model)) - continue; - StorageKey storage = - resolveHandleStorage(LI->getPointerOperand(), F, DL, methodInfo); - reportUseAfterReleaseIfNeeded(storage, &I); - } - else if (const auto* SI = llvm::dyn_cast(&I)) - { - StorageKey storage = - resolveHandleStorage(SI->getPointerOperand(), F, DL, methodInfo); - reportUseAfterReleaseIfNeeded(storage, &I); - } - else if (const auto* MI = llvm::dyn_cast(&I)) - { - StorageKey dstStorage = - resolveHandleStorage(MI->getDest(), F, DL, methodInfo); - reportUseAfterReleaseIfNeeded(dstStorage, &I); - - if (const auto* MTI = llvm::dyn_cast(MI)) - { - StorageKey srcStorage = - resolveHandleStorage(MTI->getSource(), F, DL, methodInfo); - reportUseAfterReleaseIfNeeded(srcStorage, &I); - } - } - auto* CB = llvm::dyn_cast(&I); if (!CB) continue; @@ -2950,20 +2717,6 @@ namespace ctrace::stack::analysis if (!callee) continue; - for (unsigned argIdx = 0; argIdx < CB->arg_size(); ++argIdx) - { - const llvm::Value* arg = CB->getArgOperand(argIdx); - if (!arg || !arg->getType()->isPointerTy()) - continue; - if (callArgumentIsDirectReleaseArg(*CB, callee, model, argIdx)) - continue; - if (!callArgumentLikelyDereferenced(*CB, callee, argIdx)) - continue; - - StorageKey storage = resolveHandleStorage(arg, F, DL, methodInfo); - reportUseAfterReleaseIfNeeded(storage, &I); - } - bool matchedDirectRule = false; for (const ResourceRule& rule : model.rules) { @@ -3096,7 +2849,6 @@ namespace ctrace::stack::analysis continue; StorageKey storage = resolveHandleStorage(retVal, F, DL, methodInfo); - reportUseAfterReleaseIfNeeded(storage, RI); for (auto& entry : localStates) { LocalHandleState& state = entry.second; @@ -3133,9 +2885,9 @@ namespace ctrace::stack::analysis } } - // MissingRelease comes from the ownership engine (spec §8); the counters - // above keep serving the other rules. - analyzeMissingReleaseWithEngine(F, model, ownershipLookup, DL, issues); + // UseAfterRelease and MissingRelease come from the ownership engine (spec §8); the + // counters above keep serving the other rules. + analyzeWithOwnershipEngine(F, model, ownershipLookup, DL, issues); if (methodInfo.isDtor && !methodInfo.className.empty()) { diff --git a/src/analysis/ResourceLifetimeInternal.hpp b/src/analysis/ResourceLifetimeInternal.hpp index 2515179c..bfc20549 100644 --- a/src/analysis/ResourceLifetimeInternal.hpp +++ b/src/analysis/ResourceLifetimeInternal.hpp @@ -63,4 +63,10 @@ namespace ctrace::stack::analysis::lifetime_detail std::uint64_t offset, bool viaPointerSlot, const llvm::Function& caller, const llvm::DataLayout& DL, const MethodClassInfo& callerMethodInfo); + /// Whether `callee` likely reads or writes through pointer argument `argIndex`: the + /// parameter's attributes say so, the callee is a known libc accessor, or it is an + /// external declaration (conservative). False for a defined callee without such + /// attributes and for an indirect call (callee null). + bool callArgumentLikelyDereferenced(const llvm::CallBase& CB, const llvm::Function* callee, + unsigned argIndex); } // namespace ctrace::stack::analysis::lifetime_detail diff --git a/src/analysis/ownership/ResourceFactCollector.cpp b/src/analysis/ownership/ResourceFactCollector.cpp index 6e5ec8a5..de8891bd 100644 --- a/src/analysis/ownership/ResourceFactCollector.cpp +++ b/src/analysis/ownership/ResourceFactCollector.cpp @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -165,6 +166,7 @@ namespace ctrace::stack::analysis::ownership case Event::Kind::Overwrite: case Event::Kind::Return: case Event::Kind::Exit: + case Event::Kind::Use: break; default: return; @@ -385,8 +387,68 @@ namespace ctrace::stack::analysis::ownership // ---- instruction mapping --------------------------------------------------- + /// What `I` needs alive: memory accessed through a pointer, a pointer argument the + /// callee likely dereferences, a pointer handed back to the caller. A copy, a + /// comparison, a reassignment or the argument a call releases only moves or + /// inspects the pointer value: no Use. + void emitUses(const llvm::Instruction& I) + { + if (const auto* load = llvm::dyn_cast(&I)) + use(I, load->getPointerOperand()); + else if (const auto* store = llvm::dyn_cast(&I)) + use(I, store->getPointerOperand()); + else if (const auto* rmw = llvm::dyn_cast(&I)) + use(I, rmw->getPointerOperand()); + else if (const auto* cas = llvm::dyn_cast(&I)) + use(I, cas->getPointerOperand()); + else if (const auto* mem = llvm::dyn_cast(&I)) + { + use(I, mem->getDest()); + if (const auto* transfer = llvm::dyn_cast(mem)) + use(I, transfer->getSource()); + } + else if (const auto* ret = llvm::dyn_cast(&I)) + { + // Named by the resource: the slot of a merged return is no handle. + if (const llvm::Value* v = ret->getReturnValue(); + v && v->getType()->isPointerTy()) + use(I, v, /*atRead=*/false); + } + else if (const auto* call = llvm::dyn_cast(&I); + call && !llvm::isa(call) && !call->isInlineAsm()) + { + const llvm::Function* callee = directCallee(*call); + if (!callee) + return; + const std::vector released = releasedArgs(*call, *callee); + for (unsigned i = 0; i < call->arg_size(); ++i) + { + if (std::find(released.begin(), released.end(), i) == released.end() && + lifetime_detail::callArgumentLikelyDereferenced(*call, callee, i)) + use(I, call->getArgOperand(i)); + } + } + } + + /// A Use of the pointer `ptr` is based on, unless that is a local slot, a global or a + /// constant: those hold no resource themselves. With `atRead`, a pointer just read + /// from a slot is reported where it was read, under the slot's name. + void use(const llvm::Instruction& I, const llvm::Value* ptr, bool atRead = true) + { + const llvm::Value* base = llvm::getUnderlyingObject(ptr); + if (llvm::isa(base) || llvm::isa(base)) + return; + const auto* load = atRead ? llvm::dyn_cast(base) : nullptr; + const auto from = load ? loadedFrom_.find(load) : loadedFrom_.end(); + Event e = make(Event::Kind::Use, from != loadedFrom_.end() ? *load : I); + e.src = valueLocation(base); + e.dst = from != loadedFrom_.end() ? from->second : e.src; + emit(std::move(e)); + } + void visit(const llvm::Instruction& I) { + emitUses(I); if (const auto* store = llvm::dyn_cast(&I)) { visitStore(*store); @@ -398,6 +460,7 @@ namespace ctrace::stack::analysis::ownership return; if (const auto slot = slotOf(load->getPointerOperand())) { + loadedFrom_[load] = *slot; Event e = make(Event::Kind::Copy, I); e.dst = valueLocation(load); e.src = *slot; @@ -672,32 +735,45 @@ namespace ctrace::stack::analysis::ownership emitOnEdge(invoke.getParent(), invoke.getUnwindDest(), std::move(weak)); } - /// Locations whose resource this call may release (model rule or summary). - std::vector releaseTargets(const llvm::CallBase& call, - const llvm::Function* callee) + static bool releases(const ParamTransformer& t) { - std::vector targets; - if (!callee) - return targets; + return t[static_cast(OwnState::Owned)].has(OwnState::Released); + } + + /// Arguments this call may release as passed (model rule or summary). + std::vector releasedArgs(const llvm::CallBase& call, + const llvm::Function& callee) const + { + std::vector args; for (const ResourceRule& rule : model_.rules) { - if (rule.action != RuleAction::ReleaseArg || - !ruleMatchesFunction(rule, *callee) || rule.argIndex >= call.arg_size()) - continue; - targets.push_back(valueLocation(call.getArgOperand(rule.argIndex))); + if (rule.action == RuleAction::ReleaseArg && + ruleMatchesFunction(rule, callee) && rule.argIndex < call.arg_size()) + args.push_back(rule.argIndex); } - if (const FunctionOwnershipSummary* summary = summaries_.byFunction(*callee)) + if (const FunctionOwnershipSummary* summary = summaries_.byFunction(callee)) { - const auto releases = [](const ParamTransformer& t) - { - return t[static_cast(OwnState::Owned)].has(OwnState::Released); - }; for (const auto& [argIndex, transformer] : summary->normal.params) { if (argIndex < call.arg_size() && releases(transformer) && call.getArgOperand(argIndex)->getType()->isPointerTy()) - targets.push_back(valueLocation(call.getArgOperand(argIndex))); + args.push_back(argIndex); } + } + return args; + } + + /// Locations whose resource this call may release (model rule or summary). + std::vector releaseTargets(const llvm::CallBase& call, + const llvm::Function* callee) + { + std::vector targets; + if (!callee) + return targets; + for (const unsigned i : releasedArgs(call, *callee)) + targets.push_back(valueLocation(call.getArgOperand(i))); + if (const FunctionOwnershipSummary* summary = summaries_.byFunction(*callee)) + { for (const auto& [path, transformer] : summary->normal.pointeeParams) { if (!releases(transformer)) @@ -1008,6 +1084,7 @@ namespace ctrace::stack::analysis::ownership edgeIndex_; std::map slotIds_; llvm::DenseMap valueIds_; + llvm::DenseMap loadedFrom_; // the slot read std::map exceptionalSummaryEvents_; std::optional unknownTarget_; LocationId returnLocation_ = 0; diff --git a/test/resource-lifetime/delete-twice-with-destructor.cpp b/test/resource-lifetime/delete-twice-with-destructor.cpp new file mode 100644 index 00000000..9f1085bd --- /dev/null +++ b/test/resource-lifetime/delete-twice-with-destructor.cpp @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: Apache-2.0 +// resource-model: models/resource-lifetime/generic.txt +// Unlike new-double-delete.cpp, the second delete runs a destructor on the released object: a +// use after release as well as a double release. +struct Obj +{ + int* data; + ~Obj(); +}; + +void delete_twice() +{ + Obj* o = new Obj(); + delete o; + delete o; +} + +// at line 15, column 5 +// [!!!Error] potential double release: 'CppHeap' handle 'o' is released without a matching acquire in this function + +// at line 15, column 12 +// [!!!Error] potential use-after-release: 'CppHeap' handle 'o' is used after a release in this function diff --git a/test/resource-lifetime/use-after-release-alias.c b/test/resource-lifetime/use-after-release-alias.c new file mode 100644 index 00000000..a7472415 --- /dev/null +++ b/test/resource-lifetime/use-after-release-alias.c @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: Apache-2.0 +// resource-model: models/resource-lifetime/generic.txt +// A copy of a handle keeps naming the released memory; clearing or reassigning the owner, or +// comparing the pointer, reads no released memory. +#include + +extern void consume(int* p); +extern int* other_ptr(void); + +void alias_then_null(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + int* q = p; + free(p); + p = NULL; /* not a use */ + *q = 42; /* use after release */ +} + +void alias_no_null(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + int* q = p; + free(p); + *q = 42; +} + +int alias_read(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return 0; + *p = 1; + int* q = p; + free(p); + return *q; +} + +void null_only(void) +{ + int* p = malloc(sizeof *p); + free(p); + p = NULL; +} + +void reassigned_before_use(void) +{ + int* p = malloc(sizeof *p); + free(p); + p = malloc(sizeof *p); + if (!p) + return; + *p = 1; + free(p); +} + +int compare_after_release(const int* other) +{ + int* p = malloc(sizeof *p); + if (!p) + return 0; + free(p); + return p == other; +} + +void pass_to_external(void) +{ + int* h = malloc(sizeof *h); + if (!h) + return; + free(h); + consume(h); +} + +void use_after_external_then_release(void) +{ + int* s = malloc(sizeof *s); + if (!s) + return; + consume(s); /* may keep or release it: unknown */ + free(s); /* released for certain */ + *s = 1; /* use after release */ +} + +void reassigned_from_unmodelled_callee(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + p = other_ptr(); /* not a use: the slot now holds another pointer */ + consume(p); +} + +// at line 18, column 6 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'q' is used after a release in this function + +// at line 28, column 6 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'q' is used after a release in this function + +// at line 39, column 13 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'q' is used after a release in this function + +// at line 75, column 13 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'h' is used after a release in this function + +// at line 85, column 6 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 's' is used after a release in this function + +// not contains: potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function diff --git a/test/security/null-dereference/05_null_deref.c b/test/security/null-dereference/05_null_deref.c index cde098f1..ab8848f5 100644 --- a/test/security/null-dereference/05_null_deref.c +++ b/test/security/null-dereference/05_null_deref.c @@ -68,10 +68,6 @@ int main(void) // [!!!Error] potential null pointer dereference on '' // ↳ a preceding local-slot store sets the pointer to null before use -// at line 31, column 7 -// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function -// ↳ a later dereference/call argument use may access invalid memory - -// at line 32, column 27 -// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function -// ↳ a later dereference/call argument use may access invalid memory +// vuln_null_after_free: `p = NULL` overwrites the handle and reads no released memory; `*p` then +// dereferences null (the error above), not the released allocation. +// not contains: potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function diff --git a/test/unit/analyzer_module_unit_tests.cpp b/test/unit/analyzer_module_unit_tests.cpp index f1e8ca0f..66e2b5ba 100644 --- a/test/unit/analyzer_module_unit_tests.cpp +++ b/test/unit/analyzer_module_unit_tests.cpp @@ -1416,6 +1416,35 @@ namespace report.expect(count(blocks, Kind::AddressEscape) == 1, "OwnershipCollector: passing &h to an unmodelled callee"); } + // The Use events of a function, by the name of the slot each one is read from. + const auto usesOf = [&](const char* name) + { + std::vector names; + llvm::Function* fn = loaded.module->getFunction(name); + if (!fn) + return names; + const CollectedFunction c = + collectOwnershipFacts(*fn, model, noSummaries, loaded.module->getDataLayout()); + const auto add = [&](const Event& e) + { + if (e.kind == Kind::Use) + names.push_back(c.locationIsSlot[e.dst] ? c.locationNames[e.dst] + : std::string("")); + }; + for (const Block& b : c.facts.blocks) + for (const Event& e : b.events) + add(e); + for (const Edge& edge : c.facts.edges) + for (const Event& e : edge.events) + add(e); + return names; + }; + report.expect(usesOf("use_kinds") == std::vector{"p", "q", "p"}, + "OwnershipCollector: a store through the pointer, a memset and an external " + "callee are uses; a copy, a comparison, the released argument and a " + "reassignment are not"); + report.expect(usesOf("use_return") == std::vector{""}, + "OwnershipCollector: returning the pointer is a use, named by its resource"); return report.failures == 0; } bool testOwnershipCollectorExceptions(const std::filesystem::path& repoRoot, TestReport& report) diff --git a/test/unit/ownership_collector_input.c b/test/unit/ownership_collector_input.c index 3b1ca581..6bfa11d9 100644 --- a/test/unit/ownership_collector_input.c +++ b/test/unit/ownership_collector_input.c @@ -42,3 +42,25 @@ void unknown_call(void) unknown_sink(h); unknown_out(&h); } + +/* Which uses of a pointer are Use events. */ +int use_kinds(char* other) +{ + char* p = acquire_ret(); + char* q = p; /* copy into a slot: not a use */ + if (p == other) /* comparison: not a use */ + return 0; + *p = 1; /* store through the pointer: a use */ + __builtin_memset(q, 0, 1); /* memory intrinsic: a use */ + unknown_sink(p); /* external callee: a use (conservative) */ + release_handle(p); /* the released argument: not a use */ + p = 0; /* reassignment: not a use */ + return 1; +} + +char* use_return(void) +{ + char* p = acquire_ret(); + release_handle(p); + return p; /* the pointer leaves the function: a use */ +} From 917aa5cf0b164202bbfb78cea9d562fcaa0a99d2 Mon Sep 17 00:00:00 2001 From: Hugo Date: Wed, 11 Nov 2026 23:13:45 +0100 Subject: [PATCH 3/6] test(resource-lifetime): expect no use-after-release on a second plain delete The second `delete p` of an int stays a double release, now pinned at its exact location, 7:5. Its use-after-release came from the null test that delete emits, which reads only the pointer value. A delete that runs a destructor still reports both (see delete-twice-with-destructor.cpp). --- test/resource-lifetime/new-double-delete.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/resource-lifetime/new-double-delete.cpp b/test/resource-lifetime/new-double-delete.cpp index ab75876a..e6c2cbff 100644 --- a/test/resource-lifetime/new-double-delete.cpp +++ b/test/resource-lifetime/new-double-delete.cpp @@ -8,8 +8,10 @@ int new_double_delete() return 0; } -// at line 6, column 5 +// at line 7, column 5 // [!!!Error] potential double release: 'CppHeap' handle 'p' is released without a matching acquire in this function -// at line 6, column 12 -// [!!!Error] potential use-after-release: 'CppHeap' handle 'p' is used after a release in this function +// The second `delete p` releases the memory again (the error above); its null test reads only +// the pointer value, not the released memory. delete-twice-with-destructor.cpp keeps both +// diagnostics, because there the destructor runs on the released object. +// not contains: potential use-after-release: 'CppHeap' handle 'p' is used after a release in this function From 3f26bffcb26215ecb27e30352322e81204d21888 Mon Sep 17 00:00:00 2001 From: Hugo Date: Thu, 12 Nov 2026 04:48:06 +0100 Subject: [PATCH 4/6] fix(ownership): keep a released resource released when it is handed over Copying a released pointer into a global, an out parameter or a heap object, returning it, or passing it to unknown code marked the resource Escaped, so a later access through the pointer was no longer reported. A new owner does not make freed memory live again: these hand-overs now change live resources only. Summaries change for released inputs, so the resource summary cache moves to v6. Summaries also list the arguments that a Use in the function may reach (usedArgs, kept in memory only), for the collector to read. --- include/analysis/ownership/OwnershipFacts.hpp | 4 + run_test.py | 2 +- src/analysis/ownership/OwnershipEngine.cpp | 128 ++++++++++++------ src/app/AnalyzerApp.cpp | 9 +- test/unit/ownership_engine_unit_tests.cpp | 74 ++++++++++ 5 files changed, 173 insertions(+), 44 deletions(-) diff --git a/include/analysis/ownership/OwnershipFacts.hpp b/include/analysis/ownership/OwnershipFacts.hpp index 01770457..f7369520 100644 --- a/include/analysis/ownership/OwnershipFacts.hpp +++ b/include/analysis/ownership/OwnershipFacts.hpp @@ -120,6 +120,10 @@ namespace ctrace::stack::analysis::ownership { ExitTransformer normal; ExitTransformer exceptional; + // Sorted indices of the pointer arguments a Use of the function may reach: the function + // may need what they point to alive. In memory only: a call to a declaration is a use + // of its pointer arguments anyway. + std::vector usedArgs; bool incomplete = false; std::uint8_t reservedPadding[7] = {}; }; diff --git a/run_test.py b/run_test.py index fa541804..18fff7cb 100755 --- a/run_test.py +++ b/run_test.py @@ -2511,7 +2511,7 @@ def checked_output(inputs): if checked_output(inputs) != first: return fail_check("obsolete summaries changed the wrapper diagnostics") # Intermediate fixpoint keys need not be reused; active keys must be rewritten. - if not any(json.loads(path.read_text())["schema"] == "resource-summary-cache-v5" for path in cache_files): + if not any(json.loads(path.read_text())["schema"] == "resource-summary-cache-v6" for path in cache_files): return fail_check("obsolete ownership summaries were not rebuilt") print(" ✅ ownership wrapper metadata preserved locally, across TUs and in the cache\n") return True diff --git a/src/analysis/ownership/OwnershipEngine.cpp b/src/analysis/ownership/OwnershipEngine.cpp index 43e7c91b..fd6dbf31 100644 --- a/src/analysis/ownership/OwnershipEngine.cpp +++ b/src/analysis/ownership/OwnershipEngine.cpp @@ -33,6 +33,26 @@ namespace ctrace::stack::analysis::ownership } } + /// Hands what `held` names over to someone else (a global, the caller): a live resource + /// becomes Escaped; a released one stays Released, since a new owner does not make freed + /// memory live again. Strong only when the contents name exactly one resource. + void handOver(AbstractState& s, const Contents& held) + { + for (const ResourceId r : held.resources) + { + StateSet next = s.resources[r]; + if (next.has(OwnState::Owned) || next.has(OwnState::Escaped)) + { + next.bits &= static_cast(~StateSet::of(OwnState::Owned).bits); + next |= StateSet::of(OwnState::Escaped); + } + if (held.isExactly(r)) + s.resources[r] = next; + else + s.resources[r] |= next; + } + } + void retargetLocations(AbstractState& s, ResourceId from, ResourceId to) { for (Contents& c : s.locations) @@ -154,7 +174,7 @@ namespace ctrace::stack::analysis::ownership dst.merge(src); const LocationKind kind = facts.locations[e.dst].kind; if (kind == LocationKind::NonLocal || kind == LocationKind::ArgPointee) - markHeld(s, src, StateSet::of(OwnState::Escaped), /*guaranteed=*/true); + handOver(s, src); break; } @@ -176,7 +196,7 @@ namespace ctrace::stack::analysis::ownership case Event::Kind::Return: { const Contents held = s.locations[e.src]; - markHeld(s, held, StateSet::of(OwnState::Escaped), /*guaranteed=*/true); + handOver(s, held); break; } @@ -198,6 +218,11 @@ namespace ctrace::stack::analysis::ownership { for (const ResourceId r : s.locations[loc].resources) { + // Unknown code may release or keep a live resource; it cannot make a + // released one live again, nor one that was never acquired exist. + if (!s.resources[r].has(OwnState::Owned) && + !s.resources[r].has(OwnState::Escaped)) + continue; s.resources[r] |= StateSet::of(OwnState::Released) | StateSet::of(OwnState::Escaped); s.uncertain[r] = true; @@ -268,49 +293,59 @@ namespace ctrace::stack::analysis::ownership } } - std::vector releasedUses(const OwnershipFacts& facts, - const OwnershipResult& result) + namespace { - std::vector found; - if (result.incomplete) - return found; - const auto check = [&](const Event& e, const AbstractState& s) + /// Calls visit(useEvent, stateBefore) for each Use of every reached block and of its + /// outgoing edges, replayed from the stabilised in-states. + template + void forEachUse(const OwnershipFacts& facts, const OwnershipResult& result, Visit visit) { - if (e.kind != Event::Kind::Use) - return; - for (const ResourceId r : s.locations[e.src].resources) + const auto step = [&](const Event& e, AbstractState& s) { - // Released on every path where it exists: a certain release through an exact - // handle came last, whatever was uncertain before it. - StateSet existing = s.resources[r]; - existing.bits &= static_cast(~StateSet::of(OwnState::NotOwned).bits); - if (s.resources[r].has(OwnState::Released) && - (!s.uncertain[r] || existing.isOnly(OwnState::Released))) - found.push_back({&e, r}); - } - }; - for (std::uint32_t block = 0; block < facts.blocks.size(); ++block) - { - if (block >= result.in.size() || !result.in[block].reached) - continue; - AbstractState state = result.in[block]; - for (const Event& e : facts.blocks[block].events) - { - check(e, state); - applyEvent(facts, e, state); - } - for (const Edge& edge : facts.edges) + if (e.kind == Event::Kind::Use) + visit(e, static_cast(s)); + applyEvent(facts, e, s); + }; + for (std::uint32_t block = 0; block < facts.blocks.size(); ++block) { - if (edge.from != block) + if (block >= result.in.size() || !result.in[block].reached) continue; - AbstractState along = state; - for (const Event& e : edge.events) + AbstractState state = result.in[block]; + for (const Event& e : facts.blocks[block].events) + step(e, state); + for (const Edge& edge : facts.edges) { - check(e, along); - applyEvent(facts, e, along); + if (edge.from != block) + continue; + AbstractState along = state; + for (const Event& e : edge.events) + step(e, along); } } } + } // namespace + + std::vector releasedUses(const OwnershipFacts& facts, + const OwnershipResult& result) + { + std::vector found; + if (result.incomplete) + return found; + forEachUse(facts, result, + [&](const Event& e, const AbstractState& s) + { + for (const ResourceId r : s.locations[e.src].resources) + { + // Released on every path where it exists: a certain release through + // an exact handle came last, whatever was uncertain before it. + StateSet existing = s.resources[r]; + existing.bits &= + static_cast(~StateSet::of(OwnState::NotOwned).bits); + if (s.resources[r].has(OwnState::Released) && + (!s.uncertain[r] || existing.isOnly(OwnState::Released))) + found.push_back({&e, r}); + } + }); return found; } @@ -618,8 +653,23 @@ namespace ctrace::stack::analysis::ownership } } - // Fresh resources: read off the solve with every parameter Owned. + // Fresh resources and used arguments: read off the solve with every parameter Owned. const ResourceId paramResourceLowerBound = newInstanceOf(paramSiteBase); + forEachUse(facts, res, + [&](const Event& e, const AbstractState& s) + { + for (const ResourceId r : s.locations[e.src].resources) + { + if (r < paramResourceLowerBound) + continue; // one of the function's own resources + const std::size_t p = r / 2u - paramSiteBase; + if (p < facts.paramLocations.size()) + summary.usedArgs.push_back(facts.paramLocations[p].first); + } + }); + std::sort(summary.usedArgs.begin(), summary.usedArgs.end()); + summary.usedArgs.erase(std::unique(summary.usedArgs.begin(), summary.usedArgs.end()), + summary.usedArgs.end()); summary.normal.returns.certainty = freshResourceCertainty( facts, res, false, LocationKind::Return, ArgPath{}, paramResourceLowerBound); summary.normal.returns.kind = freshResourceKind(facts, res, false, LocationKind::Return, @@ -657,8 +707,8 @@ namespace ctrace::stack::analysis::ownership return x.present == y.present && x.returns == y.returns && x.outArgs == y.outArgs && x.params == y.params && x.pointeeParams == y.pointeeParams; }; - return a.incomplete == b.incomplete && sameExit(a.normal, b.normal) && - sameExit(a.exceptional, b.exceptional); + return a.incomplete == b.incomplete && a.usedArgs == b.usedArgs && + sameExit(a.normal, b.normal) && sameExit(a.exceptional, b.exceptional); } std::vector computeSummaryFixpoint( diff --git a/src/app/AnalyzerApp.cpp b/src/app/AnalyzerApp.cpp index 1c2f2670..af50db10 100644 --- a/src/app/AnalyzerApp.cpp +++ b/src/app/AnalyzerApp.cpp @@ -1832,7 +1832,7 @@ static bool writeSummaryCacheFile(const std::filesystem::path& cacheFile, } llvm::json::Object root; - root["schema"] = "resource-summary-cache-v5"; + root["schema"] = "resource-summary-cache-v6"; root["functions"] = std::move(functionArray); std::ofstream out(cacheFile, std::ios::out | std::ios::trunc | std::ios::binary); @@ -1863,7 +1863,7 @@ readSummaryCacheFile(const std::filesystem::path& cacheFile) if (!obj) return std::nullopt; auto schema = obj->getString("schema"); - if (!schema || *schema != "resource-summary-cache-v5") + if (!schema || *schema != "resource-summary-cache-v6") return std::nullopt; const auto* functions = obj->getArray("functions"); @@ -1979,8 +1979,9 @@ struct ResourceSummaryOperations std::string cacheKey(std::size_t module, const std::string& externalHash) const { - // Keep the schema and key components identical: this refactor changes no summaries. - return md5Hex("cross-tu-resource-summary-v5|" + modelHash + "|" + externalHash + "|" + + // The version moves with the file schema whenever the analyzer computes different + // summaries (v6: handing a released resource over leaves it released). + return md5Hex("cross-tu-resource-summary-v6|" + modelHash + "|" + externalHash + "|" + filterHash + "|" + compileArgsHashes[module] + "|" + irHashes[module]); } bool usesDiskCache() const diff --git a/test/unit/ownership_engine_unit_tests.cpp b/test/unit/ownership_engine_unit_tests.cpp index 828cb556..360c6c2f 100644 --- a/test/unit/ownership_engine_unit_tests.cpp +++ b/test/unit/ownership_engine_unit_tests.cpp @@ -1086,6 +1086,9 @@ namespace f.blocks[0].events = {acquire(0, 0), unknown, release(0), use(0, 1), exit()}; r.expect(ids(f) == Ids{1}, "Uses: a certain release is judged even after an unknown call"); + f.blocks[0].events = {acquire(0, 0), release(0), unknown, use(0, 1), exit()}; + r.expect(ids(f) == Ids{1}, + "Uses: unknown code handed a released pointer cannot make it live again"); } { // A merged return block split onto its incoming edge. @@ -1107,6 +1110,76 @@ namespace r.expect(reported(f, /*limit=*/1).empty(), "Uses: nothing is reported from an incomplete solve"); } + { + // free(p); global = p; use p. Location 1 is a global, then an out parameter. + OwnershipFacts f = facts(1, {}, 2, 1); + f.locations[1].kind = LocationKind::NonLocal; + f.blocks[0].events = {acquire(0, 0), release(0), copy(1, 0), use(0, 1), exit()}; + r.expect(ids(f) == Ids{1}, + "Uses: a released resource copied into a global stays released"); + f.locations[1].kind = LocationKind::ArgPointee; + r.expect(ids(f) == Ids{1}, + "Uses: a released resource stored through an out parameter stays released"); + } + { + // Released on one path, then global = p: handed over where it was live only. + OwnershipFacts f = + facts(4, {edge(0, 1, {release(0)}), edge(0, 2), edge(1, 3), edge(2, 3)}, 2, 1); + f.locations[1].kind = LocationKind::NonLocal; + f.blocks[0].events = {acquire(0, 0)}; + f.blocks[3].events = {copy(1, 0), use(0, 1), exit()}; + const StateSet st = solve(f, entryOf(f)).exits.at(0).state.resources[n0]; + r.expect(st.has(OwnState::Escaped) && st.has(OwnState::Released) && + !st.has(OwnState::Owned), + "Uses: handing a resource over keeps the path where it was released"); + r.expect(ids(f) == Ids{1}, "Uses: a use after that hand-over is still judged"); + } + { + // free(p); return p. + OwnershipFacts f = facts(1, {}, 2, 1); + f.blocks[0].events = {acquire(0, 0), release(0), copy(1, 0), ret(1), exit()}; + r.expect( + solve(f, entryOf(f)).exits.at(0).state.resources[n0].isOnly(OwnState::Released), + "Uses: returning a released resource leaves it released"); + } + return r.failures == 0; + } + + // Which arguments a function may use, read off its summary. + bool testUsedArgs(TestReport& r) + { + using Args = std::vector; + { + // Argument 2 at location 1, copied to location 2, used on one path only. + OwnershipFacts f = facts(3, {edge(0, 1), edge(0, 2)}, 3, 0); + f.paramLocations = {{0, 0}, {2, 1}}; + f.blocks[0].events = {copy(2, 1)}; + f.blocks[1].events = {use(2, 1), exit()}; + f.blocks[2].events = {exit()}; + r.expect(computeSummary(f).usedArgs == Args{2}, + "UsedArgs: a use through a copy, on one path, of argument 2 only"); + } + { + OwnershipFacts f = facts(2, {}, 1, 0); + f.paramLocations = {{0, 0}}; + f.blocks[0].events = {exit()}; + f.blocks[1].events = {use(0, 1)}; + r.expect(computeSummary(f).usedArgs.empty(), + "UsedArgs: an unreachable use does not count"); + } + { + OwnershipFacts f = facts(1, {}, 2, 1); + f.paramLocations = {{0, 0}}; + f.blocks[0].events = {acquire(0, 1), use(1, 1), exit()}; + r.expect(computeSummary(f).usedArgs.empty(), + "UsedArgs: a use of the function's own resource is no argument's"); + } + { + FunctionOwnershipSummary a; + FunctionOwnershipSummary b; + b.usedArgs = {0}; + r.expect(!sameSummary(a, b), "UsedArgs: two summaries differ by their used arguments"); + } return r.failures == 0; } } // namespace @@ -1117,6 +1190,7 @@ int main(int, char**) (void)testDomain(report); (void)testEngine(report); (void)testUses(report); + (void)testUsedArgs(report); (void)testContracts(report); (void)testEdgeExits(report); (void)testExitUncertainty(report); From bb5f1e7c9fde01ceb482ec9c4f2a419f8c30e9e2 Mon Sep 17 00:00:00 2001 From: Hugo Date: Thu, 12 Nov 2026 04:48:06 +0100 Subject: [PATCH 5/6] fix(resource-lifetime): report uses through callees and computed addresses The counters reported these uses after a release; the engine missed them: - A call to a defined function that reads or writes through the argument, on some path or through further calls. The call is now a use when the callee's summary lists the argument. A call through a pointer, or to a function without a summary, is a use, like a call to a declaration. An argument that a callee releases only through unknown code is still a use. - p = p + 1, p++ or q = &p->field. The copy now carries the resource of the pointer the address is computed from, so *p and *q are judged. Computing the address is not a use. --- .../ownership/ResourceFactCollector.cpp | 42 ++++- .../use-after-release-escape-offset.c | 148 +++++++++++++++++ .../use-after-release-through-callee.c | 157 ++++++++++++++++++ test/unit/analyzer_module_unit_tests.cpp | 79 ++++++++- test/unit/ownership_collector_input.c | 38 +++++ 5 files changed, 453 insertions(+), 11 deletions(-) create mode 100644 test/resource-lifetime/use-after-release-escape-offset.c create mode 100644 test/resource-lifetime/use-after-release-through-callee.c diff --git a/src/analysis/ownership/ResourceFactCollector.cpp b/src/analysis/ownership/ResourceFactCollector.cpp index de8891bd..18f216b7 100644 --- a/src/analysis/ownership/ResourceFactCollector.cpp +++ b/src/analysis/ownership/ResourceFactCollector.cpp @@ -418,18 +418,39 @@ namespace ctrace::stack::analysis::ownership call && !llvm::isa(call) && !call->isInlineAsm()) { const llvm::Function* callee = directCallee(*call); - if (!callee) - return; - const std::vector released = releasedArgs(*call, *callee); + const std::vector released = + callee ? releasedArgs(*call, *callee, /*knownOnly=*/true) + : std::vector{}; for (unsigned i = 0; i < call->arg_size(); ++i) { if (std::find(released.begin(), released.end(), i) == released.end() && - lifetime_detail::callArgumentLikelyDereferenced(*call, callee, i)) + argumentUsed(*call, callee, i)) use(I, call->getArgOperand(i)); } } } + /// Whether the call may need what argument `i` points to alive. Attributes and the + /// library list decide first (a declaration is a use). Otherwise an unknown callee + /// (called through a pointer, or defined without a summary) is a use, and a defined + /// one is when a Use in its body may reach that parameter. + bool argumentUsed(const llvm::CallBase& call, const llvm::Function* callee, + unsigned i) const + { + if (lifetime_detail::callArgumentLikelyDereferenced(call, callee, i)) + return true; + if (!call.getArgOperand(i)->getType()->isPointerTy() || + call.paramHasAttr(i, llvm::Attribute::ReadNone)) + return false; + if (!callee) + return true; + if (callee->doesNotAccessMemory()) + return false; + const FunctionOwnershipSummary* summary = summaries_.byFunction(*callee); + return !summary || summary->incomplete || + std::binary_search(summary->usedArgs.begin(), summary->usedArgs.end(), i); + } + /// A Use of the pointer `ptr` is based on, unless that is a local slot, a global or a /// constant: those hold no resource themselves. With `atRead`, a pointer just read /// from a slot is reported where it was read, under the slot's name. @@ -551,9 +572,11 @@ namespace ctrace::stack::analysis::ownership e.unknownValue = !llvm::isa(stripped); return e; } + // An address computed from a pointer (p + 1, &p->field) points into the same + // resource: copy from the pointer it is computed from. Event e = make(Event::Kind::Copy, I); e.dst = dst; - e.src = valueLocation(value); + e.src = valueLocation(llvm::getUnderlyingObject(stripped)); e.strong = strong; return e; } @@ -740,9 +763,12 @@ namespace ctrace::stack::analysis::ownership return t[static_cast(OwnState::Owned)].has(OwnState::Released); } - /// Arguments this call may release as passed (model rule or summary). + /// Arguments this call may release as passed (model rule or summary). With + /// `knownOnly`, a summary counts only where that release is not uncertain: a callee + /// that hands the argument to unknown code may also use it. std::vector releasedArgs(const llvm::CallBase& call, - const llvm::Function& callee) const + const llvm::Function& callee, + bool knownOnly = false) const { std::vector args; for (const ResourceRule& rule : model_.rules) @@ -756,6 +782,8 @@ namespace ctrace::stack::analysis::ownership for (const auto& [argIndex, transformer] : summary->normal.params) { if (argIndex < call.arg_size() && releases(transformer) && + (!knownOnly || + !transformer.isUncertain(StateSet::of(OwnState::Owned))) && call.getArgOperand(argIndex)->getType()->isPointerTy()) args.push_back(argIndex); } diff --git a/test/resource-lifetime/use-after-release-escape-offset.c b/test/resource-lifetime/use-after-release-escape-offset.c new file mode 100644 index 00000000..27118d82 --- /dev/null +++ b/test/resource-lifetime/use-after-release-escape-offset.c @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: Apache-2.0 +// resource-model: models/resource-lifetime/generic.txt +#include + +// Handing a released pointer over (to a global, an out parameter, a heap object, unknown code) +// does not make the memory live again, and an address computed from it (p + 1, p++, &t->b) still +// names the released memory. The use is the access through the pointer; the copy or the +// computation is not. Each live_ function does the same before the release, or without one: no +// diagnostic. + +struct Holder +{ + int* q; +}; +struct Pair +{ + int a; + int b; +}; +int* global; +extern void ext_keep(int* p); + +void released_to_global(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + global = p; + *p = 42; +} +void released_to_out_param(int** out) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + *out = p; + *p = 42; +} +void released_to_heap_field(struct Holder* h) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + h->q = p; + *p = 42; +} +void released_then_offset(void) +{ + int* p = malloc(2 * sizeof *p); + if (!p) + return; + free(p); + p = p + 1; + *p = 42; +} +void released_then_increment(void) +{ + int* p = malloc(2 * sizeof *p); + if (!p) + return; + free(p); + p++; + *p = 42; +} +void released_field_address(void) +{ + struct Pair* t = malloc(sizeof *t); + if (!t) + return; + free(t); + int* b = &t->b; + *b = 42; +} +void released_to_unknown_code(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + ext_keep(p); /* a use: unknown code may read it */ + *p = 42; /* still released: the callee cannot make it live again */ +} + +void live_to_global(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + global = p; + *p = 42; +} +void live_to_out_param(int** out) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + *out = p; + *p = 42; +} +void live_offset(void) +{ + int* p = malloc(2 * sizeof *p); + if (!p) + return; + int* q = p + 1; + *q = 42; + free(p); +} +void live_field_address(void) +{ + struct Pair* t = malloc(sizeof *t); + if (!t) + return; + int* b = &t->b; + *b = 42; + free(t); +} + +// Reported at the access through the pointer (and at the call that hands it to unknown code), +// not at the copy or the address computation; the strict count checks that the live_ functions +// report nothing. + +// at line 30, column 6 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 39, column 6 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 48, column 6 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 57, column 6 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 66, column 6 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 75, column 6 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'b' is used after a release in this function + +// at line 83, column 14 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 84, column 6 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function diff --git a/test/resource-lifetime/use-after-release-through-callee.c b/test/resource-lifetime/use-after-release-through-callee.c new file mode 100644 index 00000000..dcd06033 --- /dev/null +++ b/test/resource-lifetime/use-after-release-through-callee.c @@ -0,0 +1,157 @@ +// SPDX-License-Identifier: Apache-2.0 +// resource-model: models/resource-lifetime/generic.txt +#include + +// A call after a release is a use when the callee may need the memory alive: it writes or reads +// through the pointer (maybe on some paths only), hands it to unknown code, or passes it on. A +// callee that only keeps the pointer is not a use. Each live_ function makes the same call before +// the release: no diagnostic. + +extern void ext_sink(int* p); +int* kept; + +void writes(int* p) +{ + *p = 42; +} +int reads(const int* p) +{ + return *p; +} +void writes_if(int* p, int c) +{ + if (c) + *p = 1; +} +void forwards_to_external(int* p) +{ + ext_sink(p); +} +void forwards_to_local(int* p) +{ + writes(p); +} +void keeps(int* p) +{ + kept = p; +} + +void released_writes(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + writes(p); +} +int released_reads(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return 0; + free(p); + return reads(p); +} +void released_writes_if(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + writes_if(p, 1); +} +void released_forwards_to_external(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + forwards_to_external(p); +} +void released_forwards_to_local(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + forwards_to_local(p); +} +void released_indirect(void (*fp)(int*)) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + fp(p); +} +void released_keeps(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + keeps(p); /* only keeps the pointer: not a use */ +} + +void live_writes(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + writes(p); + free(p); +} +int live_reads(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return 0; + const int v = reads(p); + free(p); + return v; +} +void live_writes_if(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + writes_if(p, 1); + free(p); +} +void live_forwards_to_local(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + forwards_to_local(p); + free(p); +} +void live_indirect(void (*fp)(int*)) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + fp(p); + free(p); +} + +// Reported at the argument of each released_ call except released_keeps; the strict count +// checks that released_keeps and the live_ functions report nothing. + +// at line 45, column 12 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 53, column 18 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 61, column 15 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 69, column 26 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 77, column 23 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 85, column 8 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function diff --git a/test/unit/analyzer_module_unit_tests.cpp b/test/unit/analyzer_module_unit_tests.cpp index 66e2b5ba..57e7c3f5 100644 --- a/test/unit/analyzer_module_unit_tests.cpp +++ b/test/unit/analyzer_module_unit_tests.cpp @@ -16,6 +16,7 @@ #include "analysis/Reachability.hpp" #include "analysis/ResourceLifetimeAnalysis.hpp" #include "analysis/ResourceModel.hpp" +#include "analysis/ownership/OwnershipEngine.hpp" #include "analysis/ownership/ResourceFactCollector.hpp" #include "analysis/StackBufferAnalysis.hpp" #include "analysis/UninitializedVarAnalysis.hpp" @@ -1417,14 +1418,14 @@ namespace "OwnershipCollector: passing &h to an unmodelled callee"); } // The Use events of a function, by the name of the slot each one is read from. - const auto usesOf = [&](const char* name) + const auto usesOf = [&](const char* name, const SummaryLookup& summaries) { std::vector names; llvm::Function* fn = loaded.module->getFunction(name); if (!fn) return names; const CollectedFunction c = - collectOwnershipFacts(*fn, model, noSummaries, loaded.module->getDataLayout()); + collectOwnershipFacts(*fn, model, summaries, loaded.module->getDataLayout()); const auto add = [&](const Event& e) { if (e.kind == Kind::Use) @@ -1439,12 +1440,82 @@ namespace add(e); return names; }; - report.expect(usesOf("use_kinds") == std::vector{"p", "q", "p"}, + report.expect(usesOf("use_kinds", noSummaries) == std::vector{"p", "q", "p"}, "OwnershipCollector: a store through the pointer, a memset and an external " "callee are uses; a copy, a comparison, the released argument and a " "reassignment are not"); - report.expect(usesOf("use_return") == std::vector{""}, + report.expect(usesOf("use_return", noSummaries) == std::vector{""}, "OwnershipCollector: returning the pointer is a use, named by its resource"); + + // A call to a defined function is a use of the arguments its summary uses; without a + // summary, or through a pointer, the callee is unknown and the call is a use. + FunctionOwnershipSummary usesFirst; + usesFirst.usedArgs = {0}; + FunctionOwnershipSummary keepsOnly; + const SummaryLookup bodies{[&](const llvm::Function& fn) -> const FunctionOwnershipSummary* + { + if (fn.getName() == "callee_uses") + return &usesFirst; + if (fn.getName() == "callee_keeps") + return &keepsOnly; + return nullptr; + }}; + report.expect(usesOf("use_callees", bodies) == std::vector{"p", "p"}, + "OwnershipCollector: a defined callee that uses the argument and an " + "indirect call are uses; a callee that only keeps the pointer is not"); + report.expect(usesOf("use_callees", noSummaries) == std::vector{"p", "p", "p"}, + "OwnershipCollector: a defined callee without a summary is a use"); + keepsOnly.incomplete = true; + report.expect(usesOf("use_callees", bodies) == std::vector{"p", "p", "p"}, + "OwnershipCollector: a defined callee with an incomplete summary is a use"); + const auto usedArgsOf = [&](const char* name) + { + const llvm::Function* fn = loaded.module->getFunction(name); + if (!fn) + return std::vector{99}; + return computeSummary(collectOwnershipFacts(*fn, model, noSummaries, + loaded.module->getDataLayout()) + .facts) + .usedArgs; + }; + report.expect(usedArgsOf("callee_uses") == std::vector{0}, + "OwnershipSummary: a store through the parameter uses it"); + report.expect(usedArgsOf("callee_keeps").empty(), + "OwnershipSummary: storing the parameter somewhere does not use it"); + if (const llvm::Function* fn = loaded.module->getFunction("callee_forwards")) + { + const FunctionOwnershipSummary forwards = computeSummary( + collectOwnershipFacts(*fn, model, noSummaries, loaded.module->getDataLayout()) + .facts); + const SummaryLookup forwarding{ + [&](const llvm::Function& callee) -> const FunctionOwnershipSummary* + { return callee.getName() == "callee_forwards" ? &forwards : nullptr; }}; + report.expect(usesOf("use_wrapped", forwarding) == std::vector{"p"}, + "OwnershipCollector: an argument a callee hands to unknown code is a " + "use, though that callee may release it"); + } + else + { + report.expect(false, "OwnershipCollector: callee_forwards collected"); + } + + // p = p + 1 keeps the resource in p: releasing p then releases it. + if (const llvm::Function* fn = loaded.module->getFunction("offset_reassign")) + { + const CollectedFunction c = + collectOwnershipFacts(*fn, model, noSummaries, loaded.module->getDataLayout()); + const OwnershipResult res = solve( + c.facts, AbstractState::entry(2u * c.facts.siteCount, c.facts.locations.size())); + report.expect( + res.exits.size() == 1 && + res.exits[0].state.resources[newInstanceOf(0)].isOnly(OwnState::Released), + "OwnershipCollector: an address computed from the handle carries its " + "resource"); + } + else + { + report.expect(false, "OwnershipCollector: offset_reassign collected"); + } return report.failures == 0; } bool testOwnershipCollectorExceptions(const std::filesystem::path& repoRoot, TestReport& report) diff --git a/test/unit/ownership_collector_input.c b/test/unit/ownership_collector_input.c index 6bfa11d9..7e930a58 100644 --- a/test/unit/ownership_collector_input.c +++ b/test/unit/ownership_collector_input.c @@ -64,3 +64,41 @@ char* use_return(void) release_handle(p); return p; /* the pointer leaves the function: a use */ } + +/* Calls to defined functions: a use when the callee's body may use the argument. */ +static char* kept; +void callee_uses(char* p) +{ + *p = 1; /* a use of the parameter */ +} +void callee_keeps(char* p) +{ + kept = p; /* only stores the pointer: no use */ +} +int use_callees(void (*fp)(char*)) +{ + char* p = acquire_ret(); + callee_uses(p); /* a use: the callee's summary says it uses the argument */ + callee_keeps(p); /* not a use: the callee only keeps it */ + fp(p); /* indirect call: an unknown callee, a use */ + release_handle(p); + return 0; +} + +void callee_forwards(char* p) +{ + unknown_sink(p); /* unknown code: may use the pointer, may release it */ +} +void use_wrapped(void) +{ + char* p = acquire_ret(); + callee_forwards(p); /* a use, though the callee's summary says it may release */ +} + +/* An address computed from the handle carries its resource. */ +void offset_reassign(void) +{ + char* p = acquire_ret(); + p = p + 1; + release_handle(p); +} From 04073d260861b47a4f202f7a2696303f5c172934 Mon Sep 17 00:00:00 2001 From: Hugo Date: Thu, 12 Nov 2026 05:25:12 +0100 Subject: [PATCH 6/6] fix(resource-lifetime): report a use by a callee that also releases the argument A callee that releases its argument was never a use, even when it also accesses it, so `free(p); write_and_free(p);` reported only the double release. The counters reported the use-after-release as well. The call is now a use of a released argument when the callee's summary lists a Use of it, on the same path as the release or on another one. A model release or a wrapper that only releases stays no use. The summary cache now stores usedArgs, so a callee in another file gives the same result when its summary comes from the cache. A cache file without them is a miss and is rebuilt. --- include/analysis/ownership/OwnershipFacts.hpp | 3 +- run_test.py | 33 +++++++ .../ownership/ResourceFactCollector.cpp | 19 +++- src/app/AnalyzerApp.cpp | 17 +++- .../cross-tu-use-then-release-def.c | 23 +++++ .../cross-tu-use-then-release-use.c | 56 ++++++++++++ .../use-after-release-through-callee.c | 89 ++++++++++++++++++- test/unit/analyzer_module_unit_tests.cpp | 34 +++++++ test/unit/ownership_collector_input.c | 19 ++++ 9 files changed, 283 insertions(+), 10 deletions(-) create mode 100644 test/resource-lifetime/cross-tu-use-then-release-def.c create mode 100644 test/resource-lifetime/cross-tu-use-then-release-use.c diff --git a/include/analysis/ownership/OwnershipFacts.hpp b/include/analysis/ownership/OwnershipFacts.hpp index f7369520..a2415ba1 100644 --- a/include/analysis/ownership/OwnershipFacts.hpp +++ b/include/analysis/ownership/OwnershipFacts.hpp @@ -121,8 +121,7 @@ namespace ctrace::stack::analysis::ownership ExitTransformer normal; ExitTransformer exceptional; // Sorted indices of the pointer arguments a Use of the function may reach: the function - // may need what they point to alive. In memory only: a call to a declaration is a use - // of its pointer arguments anyway. + // may need what they point to alive, even when it also releases them. std::vector usedArgs; bool incomplete = false; std::uint8_t reservedPadding[7] = {}; diff --git a/run_test.py b/run_test.py index 18fff7cb..a14c5f68 100755 --- a/run_test.py +++ b/run_test.py @@ -2448,6 +2448,39 @@ def check_ownership_cross_tu() -> bool: if outputs[0] != outputs[1]: print(f" ❌ {name}: cached run differs from the first run") ok = False + # Callees that release their argument: the call after a release is a use when the callee + # also accesses it (after the release, or on another path), not when it only releases + # it. Same result with Z3, and when the summaries are read back from the cache: the + # second run may still recompute intermediate rounds of the fixpoint, the third reads + # every summary from it. + files = [ + str(fixtures / "cross-tu-use-then-release-def.c"), + str(fixtures / "cross-tu-use-then-release-use.c"), + ] + expected_uses = [ + f"at line {line}, column {column}\n\t[!!!Error] potential use-after-release: 'HeapAlloc' handle 'p'" + for line, column in ((18, 32), (26, 30)) + ] + smt_args = [ + "--smt=on", + "--smt-backend=z3", + "--smt-mode=single", + f"--smt-rules={','.join(_all_smt_rules())}", + f"--smt-timeout-ms={SMT_FIXTURE_TIMEOUT_MS}", + ] + for mode, extra in (("default", []), ("smt-z3", smt_args)): + args = files + common[:-1] + [f"--resource-summary-cache-dir={tmpdir / f'use-then-release-{mode}'}"] + for run in ("first", "second", "third"): + result = run_analyzer_uncached(args + extra) + output = result.stdout or "" + if ( + result.returncode != 0 + or output.count("use-after-release") != len(expected_uses) + or not all(use in output for use in expected_uses) + ): + print(f" ❌ use-then-release ({mode}, {run}): expected the use-after-release at lines 18 and 26 only") + print(output + (result.stderr or "")) + ok = False if ok: print(" ✅ resource ownership cross-TU transformers OK\n") return ok diff --git a/src/analysis/ownership/ResourceFactCollector.cpp b/src/analysis/ownership/ResourceFactCollector.cpp index 18f216b7..bcdb2579 100644 --- a/src/analysis/ownership/ResourceFactCollector.cpp +++ b/src/analysis/ownership/ResourceFactCollector.cpp @@ -389,7 +389,7 @@ namespace ctrace::stack::analysis::ownership /// What `I` needs alive: memory accessed through a pointer, a pointer argument the /// callee likely dereferences, a pointer handed back to the caller. A copy, a - /// comparison, a reassignment or the argument a call releases only moves or + /// comparison, a reassignment or an argument the call only releases moves or /// inspects the pointer value: no Use. void emitUses(const llvm::Instruction& I) { @@ -423,8 +423,11 @@ namespace ctrace::stack::analysis::ownership : std::vector{}; for (unsigned i = 0; i < call->arg_size(); ++i) { - if (std::find(released.begin(), released.end(), i) == released.end() && - argumentUsed(*call, callee, i)) + // A released argument is still used when the callee's summary shows a + // Use of it (`*p = 1; free(p);`), not when the callee only releases it. + const bool isReleased = + std::find(released.begin(), released.end(), i) != released.end(); + if (isReleased ? summaryUses(callee, i) : argumentUsed(*call, callee, i)) use(I, call->getArgOperand(i)); } } @@ -447,7 +450,15 @@ namespace ctrace::stack::analysis::ownership if (callee->doesNotAccessMemory()) return false; const FunctionOwnershipSummary* summary = summaries_.byFunction(*callee); - return !summary || summary->incomplete || + return !summary || summary->incomplete || summaryUses(callee, i); + } + + /// Whether the summary of `callee` lists a Use of its parameter `i`. + bool summaryUses(const llvm::Function* callee, unsigned i) const + { + const FunctionOwnershipSummary* summary = + callee ? summaries_.byFunction(*callee) : nullptr; + return summary && std::binary_search(summary->usedArgs.begin(), summary->usedArgs.end(), i); } diff --git a/src/app/AnalyzerApp.cpp b/src/app/AnalyzerApp.cpp index af50db10..54f3e66a 100644 --- a/src/app/AnalyzerApp.cpp +++ b/src/app/AnalyzerApp.cpp @@ -1633,6 +1633,10 @@ encodeOwnershipSummary(const ctrace::stack::analysis::ownership::FunctionOwnersh { llvm::json::Object obj; obj["incomplete"] = s.incomplete; + llvm::json::Array usedArgs; + for (const unsigned i : s.usedArgs) + usedArgs.push_back(static_cast(i)); + obj["usedArgs"] = std::move(usedArgs); obj["normal"] = encodeExitTransformer(s.normal); obj["exceptional"] = encodeExitTransformer(s.exceptional); return obj; @@ -1734,11 +1738,19 @@ decodeOwnershipSummary(const llvm::json::Object& obj, ctrace::stack::analysis::ownership::FunctionOwnershipSummary& out) { const auto incomplete = obj.getBoolean("incomplete"); + const auto* usedArgs = obj.getArray("usedArgs"); const auto* normal = obj.getObject("normal"); const auto* exceptional = obj.getObject("exceptional"); - if (!incomplete || !normal || !exceptional) + if (!incomplete || !usedArgs || !normal || !exceptional) return false; out.incomplete = *incomplete; + for (const auto& value : *usedArgs) + { + const auto i = value.getAsInteger(); + if (!i || *i < 0 || (!out.usedArgs.empty() && *i <= out.usedArgs.back())) + return false; // sorted and distinct: the collector searches it + out.usedArgs.push_back(static_cast(*i)); + } return decodeExitTransformer(*normal, out.normal) && decodeExitTransformer(*exceptional, out.exceptional); } @@ -1980,7 +1992,8 @@ struct ResourceSummaryOperations std::string cacheKey(std::size_t module, const std::string& externalHash) const { // The version moves with the file schema whenever the analyzer computes different - // summaries (v6: handing a released resource over leaves it released). + // summaries (v6: handing a released resource over leaves it released, and a summary + // lists the arguments the function uses). return md5Hex("cross-tu-resource-summary-v6|" + modelHash + "|" + externalHash + "|" + filterHash + "|" + compileArgsHashes[module] + "|" + irHashes[module]); } diff --git a/test/resource-lifetime/cross-tu-use-then-release-def.c b/test/resource-lifetime/cross-tu-use-then-release-def.c new file mode 100644 index 00000000..e2638ffb --- /dev/null +++ b/test/resource-lifetime/cross-tu-use-then-release-def.c @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: Apache-2.0 +// Driven by check_ownership_cross_tu(): callees that release their argument, after or beside an +// access through it, or without one. +#include + +void writes_then_frees_cross_tu(int* p) +{ + *p = 42; + free(p); +} + +void frees_or_writes_cross_tu(int* p, int c) +{ + if (c) + free(p); + else + *p = 1; +} + +void frees_cross_tu(int* p) +{ + free(p); +} diff --git a/test/resource-lifetime/cross-tu-use-then-release-use.c b/test/resource-lifetime/cross-tu-use-then-release-use.c new file mode 100644 index 00000000..3bba2f64 --- /dev/null +++ b/test/resource-lifetime/cross-tu-use-then-release-use.c @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: Apache-2.0 +// Driven by check_ownership_cross_tu(): the callees defined in cross-tu-use-then-release-def.c +// release their argument. The call after a release is a use when the callee also accesses the +// argument (lines 18 and 26), not when it only releases it. The live_ functions report no use. +#include + +extern void writes_then_frees_cross_tu(int* p); +extern void frees_or_writes_cross_tu(int* p, int c); +extern void frees_cross_tu(int* p); + +// strict-diagnostic-count: false +void released_writes_then_frees(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + writes_then_frees_cross_tu(p); +} +void released_frees_or_writes(int c) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + frees_or_writes_cross_tu(p, c); +} +void released_frees(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + frees_cross_tu(p); /* a double release, not a use */ +} +void live_writes_then_frees(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + writes_then_frees_cross_tu(p); +} +void live_frees_or_writes(int c) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + frees_or_writes_cross_tu(p, c); +} +void live_frees(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + frees_cross_tu(p); +} diff --git a/test/resource-lifetime/use-after-release-through-callee.c b/test/resource-lifetime/use-after-release-through-callee.c index dcd06033..88a2ac0a 100644 --- a/test/resource-lifetime/use-after-release-through-callee.c +++ b/test/resource-lifetime/use-after-release-through-callee.c @@ -135,8 +135,75 @@ void live_indirect(void (*fp)(int*)) free(p); } -// Reported at the argument of each released_ call except released_keeps; the strict count -// checks that released_keeps and the live_ functions report nothing. +// A callee that releases the pointer is still a use when it also accesses it, on the same path +// or on another one. A callee that only releases it is no use: the second release is reported +// as a double release only. +void writes_then_frees(int* p) +{ + *p = 42; + free(p); +} +void frees_or_writes(int* p, int c) +{ + if (c) + free(p); + else + *p = 1; +} +void frees(int* p) +{ + free(p); +} + +void released_writes_then_frees(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + writes_then_frees(p); +} +void released_frees_or_writes(int c) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + frees_or_writes(p, c); +} +void released_frees(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + free(p); + frees(p); /* a double release, not a use */ +} +void live_writes_then_frees(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + writes_then_frees(p); +} +void live_frees_or_writes(int c) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + frees_or_writes(p, c); +} +void live_frees(void) +{ + int* p = malloc(sizeof *p); + if (!p) + return; + frees(p); +} + +// Reported at the argument of each released_ call except released_keeps and released_frees; +// the strict count checks that those and the live_ functions report nothing else. The only +// other diagnostics are the double releases and the leak live_frees_or_writes may cause. // at line 45, column 12 // [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function @@ -155,3 +222,21 @@ void live_indirect(void (*fp)(int*)) // at line 85, column 8 // [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 164, column 23 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 164, column 5 +// [!!!Error] potential double release: 'HeapAlloc' handle 'p' is released without a matching acquire in this function + +// at line 172, column 21 +// [!!!Error] potential use-after-release: 'HeapAlloc' handle 'p' is used after a release in this function + +// at line 172, column 5 +// [!!!Error] potential double release: 'HeapAlloc' handle 'p' is released without a matching acquire in this function + +// at line 180, column 5 +// [!!!Error] potential double release: 'HeapAlloc' handle 'p' is released without a matching acquire in this function + +// at line 191, column 14 +// [ !!Warn ] potential resource leak: 'HeapAlloc' acquired in handle 'p' may leave the function without being released diff --git a/test/unit/analyzer_module_unit_tests.cpp b/test/unit/analyzer_module_unit_tests.cpp index 57e7c3f5..82fa8b97 100644 --- a/test/unit/analyzer_module_unit_tests.cpp +++ b/test/unit/analyzer_module_unit_tests.cpp @@ -1516,6 +1516,40 @@ namespace { report.expect(false, "OwnershipCollector: offset_reassign collected"); } + + // Releasing an argument does not hide a use of it in the same callee. + const auto summaryOf = [&](const char* name) + { + const llvm::Function* fn = loaded.module->getFunction(name); + return fn ? computeSummary(collectOwnershipFacts(*fn, model, noSummaries, + loaded.module->getDataLayout()) + .facts) + : FunctionOwnershipSummary{}; + }; + const FunctionOwnershipSummary usesThenReleases = summaryOf("callee_uses_then_releases"); + const FunctionOwnershipSummary releasesOnly = summaryOf("callee_releases"); + const auto releasesFirstArg = [](const FunctionOwnershipSummary& s) + { + const auto it = s.normal.params.find(0); + return it != s.normal.params.end() && + it->second[static_cast(OwnState::Owned)].has(OwnState::Released); + }; + report.expect(releasesFirstArg(usesThenReleases) && releasesFirstArg(releasesOnly) && + usesThenReleases.usedArgs == std::vector{0} && + releasesOnly.usedArgs.empty(), + "OwnershipSummary: both callees release the argument, one also uses it"); + const SummaryLookup releasing{ + [&](const llvm::Function& fn) -> const FunctionOwnershipSummary* + { + if (fn.getName() == "callee_uses_then_releases") + return &usesThenReleases; + if (fn.getName() == "callee_releases") + return &releasesOnly; + return nullptr; + }}; + report.expect(usesOf("use_releasing_callees", releasing) == std::vector{"p"}, + "OwnershipCollector: a callee that uses the argument it releases is a use; " + "one that only releases it is not"); return report.failures == 0; } bool testOwnershipCollectorExceptions(const std::filesystem::path& repoRoot, TestReport& report) diff --git a/test/unit/ownership_collector_input.c b/test/unit/ownership_collector_input.c index 7e930a58..c5a8ccca 100644 --- a/test/unit/ownership_collector_input.c +++ b/test/unit/ownership_collector_input.c @@ -102,3 +102,22 @@ void offset_reassign(void) p = p + 1; release_handle(p); } + +/* A callee that uses its argument and also releases it still uses it; one that only releases + it does not. */ +void callee_uses_then_releases(char* p) +{ + *p = 1; + release_handle(p); +} +void callee_releases(char* p) +{ + release_handle(p); +} +void use_releasing_callees(void) +{ + char* p = acquire_ret(); + callee_uses_then_releases(p); /* a use, though the callee also releases it */ + char* q = acquire_ret(); + callee_releases(q); /* not a use: the callee only releases it */ +}