fix(stack-buffer): check what callees access through a stack array's address

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

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

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<const std::set<std::string>> deterministicFunctions;
         std::shared_ptr<const analysis::SizeMinusKWrapperIndex> sizeMinusKWrapperIndex;
+        // What the external functions defined across the modules analyzed together access
+        // through their pointer parameters (#266).
+        std::shared_ptr<const analysis::ParamAccessIndex> 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<const analysis::GlobalStackFacts> globalStackFacts;

include/analysis/FunctionFacts.hpp

@@ -15,6 +15,8 @@ namespace llvm
     class LazyValueInfo;
     class LoadInst;
     class MemoryAccess;
+    template <typename T> 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<llvm::LoadInst*>& loads) const;
+
       private:
         struct Impl;
         std::unique_ptr<Impl> impl_;

include/analysis/ParamAccessSummary.hpp

@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: Apache-2.0
+#pragma once
+
+#include <cstdint>
+#include <map>
+#include <optional>
+#include <string>
+#include <vector>
+
+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<ParamAccess> lowestRead;
+        std::optional<ParamAccess> highestRead;
+        std::optional<ParamAccess> lowestWrite;
+        std::optional<ParamAccess> 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<ParamAccessBounds> 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<std::string, ParamAccessSummary> functions;
+    };
+
+    using ParamAccessSummaries = std::map<const llvm::Function*, ParamAccessSummary>;
+
+    /// 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<llvm::Module*>& modules);
+} // namespace ctrace::stack::analysis

include/analysis/StackBufferAnalysis.hpp

@@ -3,6 +3,7 @@
 
 #include <cstdint>
 #include <functional>
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -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<CalleeAccessOutOfBounds> calleeAccess;
         // Optional : helper for sync string <- vector
         void rebuildAliasPathString(const std::string& sep = " -> ")
         {

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,

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<llvm::LoadInst*>& loads) const
+    {
+        using namespace llvm;
+        if (!store.isSimple())
+            return;
+        const Value* slot = store.getPointerOperand();
+        const Type* type = store.getValueOperand()->getType();
+
+        SmallVector<LoadInst*, 4> candidates;
+        const auto* alloca = dyn_cast<AllocaInst>(slot);
+        bool onlyThisStore = alloca && alloca->getFunction() == &impl_->function;
+        for (const Use& use : slot->uses())
+        {
+            if (auto* load = dyn_cast<LoadInst>(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<MemoryDef>(clobberingAccess(*load));
+            if (def && def->getMemoryInst() == &store)
+                loads.push_back(load);
+        }
+    }
 } // namespace ctrace::stack::analysis