From db38a0405022aa8ed0cf9b0bf4c373c82f15e6a4 Mon Sep 17 00:00:00 2001 From: Hugo Date: Thu, 22 Oct 2026 05:54:33 +0200 Subject: [PATCH 1/4] test(sarif): every rule the analyzer can report must carry a title and a description The SARIF writer sets each rule's shortDescription to the rule id and writes no fullDescription, so GitHub code scanning names an alert after its id and explains nothing. Every rule id the emitters can give, read from their sources as for the CWEs (explicit ids, const-parameter variants, clang classifications, and the error code names a diagnostic without an explicit id takes), must get a title other than its id and a description. Refs #242 --- test/unit/analyzer_module_unit_tests.cpp | 112 +++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/test/unit/analyzer_module_unit_tests.cpp b/test/unit/analyzer_module_unit_tests.cpp index f8f7f5d..3a98dfe 100644 --- a/test/unit/analyzer_module_unit_tests.cpp +++ b/test/unit/analyzer_module_unit_tests.cpp @@ -2165,6 +2165,117 @@ namespace return std::nullopt; } + /// Every rule id the emitters can give a diagnostic, read from their sources so that a new + /// rule cannot go undescribed unnoticed: the explicit ids, the const-parameter variants, the + /// clang classifications, and the error code names, which a diagnostic without an explicit + /// id takes. + std::set emittableRuleIds(const std::filesystem::path& repoRoot) + { + std::set ids; + const auto read = [&](const char* source) -> std::string + { + llvm::ErrorOr> buffer = + llvm::MemoryBuffer::getFile((repoRoot / source).string()); + return buffer ? (*buffer)->getBuffer().str() : std::string(); + }; + // The literal after each occurrence of `marker`, up to its closing quote. + const auto literalsAfter = + [&ids](const std::string& text, llvm::StringRef marker, const std::string& prefix = {}) + { + for (std::size_t at = text.find(marker.str()); at != std::string::npos; + at = text.find(marker.str(), at + 1)) + { + const std::size_t start = at + marker.size(); + const std::size_t end = text.find('"', start); + if (end != std::string::npos) + ids.insert(prefix + text.substr(start, end - start)); + } + }; + const std::string emitter = read("src/analyzer/DiagnosticEmitter.cpp"); + literalsAfter(emitter, "ruleId(\""); + literalsAfter(emitter, "ruleId = \""); + // `subLabel = "Pointer"`, and both arms of `subLabel = cond ? "A" : "B"`. + for (std::size_t at = emitter.find("subLabel ="); at != std::string::npos; + at = emitter.find("subLabel =", at + 1)) + { + const std::string statement = emitter.substr(at, emitter.find(';', at) - at); + literalsAfter(statement, "\"", "ConstParameterNotModified."); + } + literalsAfter(read("src/analysis/FrontendDiagnostics.cpp"), "Classification{\""); + for (const std::string_view name : + ctrace::stack::EnumTraits::names) + { + if (name != "None") + ids.insert(std::string(name)); + } + // literalsAfter("\"") also collects the closing quote of each literal as an opening + // one: keep the ids, which are the literals, not what lies between them. + std::erase_if(ids, + [](const std::string& id) + { + return id.empty() || id.find_first_of(" ?:;") != std::string::npos || + id == "ConstParameterNotModified."; + }); + return ids; + } + + /// The rule @p rule of the SARIF log @p sarif, or std::nullopt when it is absent. + std::optional sarifRule(const std::string& sarif, llvm::StringRef rule) + { + llvm::Expected parsed = llvm::json::parse(sarif); + if (!parsed) + { + llvm::consumeError(parsed.takeError()); + return std::nullopt; + } + const llvm::json::Object* root = parsed->getAsObject(); + const llvm::json::Array* runs = root ? root->getArray("runs") : nullptr; + const llvm::json::Object* run = runs && !runs->empty() ? (*runs)[0].getAsObject() : nullptr; + const llvm::json::Object* tool = run ? run->getObject("tool") : nullptr; + const llvm::json::Object* driver = tool ? tool->getObject("driver") : nullptr; + const llvm::json::Array* rules = driver ? driver->getArray("rules") : nullptr; + for (const llvm::json::Value& entry : rules ? *rules : llvm::json::Array{}) + { + const llvm::json::Object* object = entry.getAsObject(); + if (object && object->getString("id") == rule) + return *object; + } + return std::nullopt; + } + + /// SARIF: a consumer such as GitHub code scanning explains a finding from its rule, so every + /// rule the analyzer can report carries a title other than its id and a description. + bool testEveryRuleIsDescribed(const std::filesystem::path& repoRoot, TestReport& report) + { + const std::set ids = emittableRuleIds(repoRoot); + report.expect(ids.count("StackBufferOverflow") && ids.count("Recursion.Unconditional") && + ids.count("ConstParameterNotModified.ReferenceRvaluePreferValue") && + ids.count("FormatString.NonLiteral"), + "SARIF: the rule ids are read from the emitters' sources"); + for (const std::string& id : ids) + { + ctrace::stack::Diagnostic d; + d.ruleId = id; + d.filePath = "demo.c"; + d.line = 1; + d.column = 1; + d.message = "m"; + ctrace::stack::AnalysisResult result; + result.diagnostics = {d}; + const std::optional rule = + sarifRule(ctrace::stack::toSarif(result, "demo.c"), id); + const auto text = [&rule](llvm::StringRef key) -> llvm::StringRef + { + const llvm::json::Object* message = rule ? rule->getObject(key) : nullptr; + return message ? message->getString("text").value_or("") : ""; + }; + report.expect(rule && !text("shortDescription").empty() && + text("shortDescription") != id && !text("fullDescription").empty(), + "SARIF: rule " + id + " has a title and a description"); + } + return report.failures == 0; + } + /// SARIF: every CWE the diagnostics report has a security severity, so the rules that report /// it are security rules; dead code (CWE-561), a quality issue for CodeQL, is the exception. /// The CWEs are read from the emitters' sources, so a new one cannot be left unscored @@ -2662,6 +2773,7 @@ int main(int argc, char** argv) (void)testAnalysisReportContract(repoRoot, report); (void)testSarifRuleCweTags(report); (void)testSarifSecuritySeverity(report); + (void)testEveryRuleIsDescribed(repoRoot, report); (void)testAnalysisIncompleteNotesHaveTheirOwnRules(report); (void)testUnresolvedCallsMarkStackUnknown(repoRoot, report); (void)testAssumeExternalFrameReplacesUnknown(repoRoot, report); From 90cfb68a8459d30eb608af6015cb9ceb48bd8919 Mon Sep 17 00:00:00 2001 From: Hugo Date: Thu, 22 Oct 2026 05:55:48 +0200 Subject: [PATCH 2/4] feat(report): describe each rule the analyzer reports describeRule(ruleId), declared in the new public header RuleCatalog.hpp, gives a title and a description for each of the 52 rule ids the emitters can give, the error code names included: a program linking the analyzer, such as CoreTrace, can describe a finding's rule. The catalog lives with the report serialization, which will read it. Refs #242 --- include/RuleCatalog.hpp | 21 ++++ src/report/ReportSerialization.cpp | 187 +++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+) create mode 100644 include/RuleCatalog.hpp diff --git a/include/RuleCatalog.hpp b/include/RuleCatalog.hpp new file mode 100644 index 0000000..3728e98 --- /dev/null +++ b/include/RuleCatalog.hpp @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: Apache-2.0 +// RuleCatalog.hpp +#pragma once + +#include +#include + +namespace ctrace::stack +{ + /// What a rule detects, as the reports describe it: a title, and a sentence on what its + /// findings mean. GitHub code scanning shows them to explain an alert. + struct RuleDescription + { + std::string_view title; + std::string_view description; + }; + + /// The description of the rule @p ruleId, the id a Diagnostic carries; nothing for an id + /// the analyzer does not report. The SARIF report reads the same text. + [[nodiscard]] std::optional describeRule(std::string_view ruleId); +} // namespace ctrace::stack diff --git a/src/report/ReportSerialization.cpp b/src/report/ReportSerialization.cpp index d523224..bdaa79d 100644 --- a/src/report/ReportSerialization.cpp +++ b/src/report/ReportSerialization.cpp @@ -1,5 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 #include "StackUsageAnalyzer.hpp" +#include "RuleCatalog.hpp" #include #include @@ -542,4 +543,190 @@ namespace ctrace::stack return os.str(); } + std::optional describeRule(std::string_view ruleId) + { + struct DescribedRule + { + std::string_view ruleId; + RuleDescription description; + }; + // Sorted by id. Every id the emitters can give, the error code names included: a + // diagnostic without an explicit id takes the name of its code. + static constexpr DescribedRule kRules[] = { + {"AllocaTooLarge", + {"Large stack allocation", "An alloca or variable-length array larger than the " + "configured threshold, which may overflow the stack."}}, + {"AllocaUsageWarning", + {"Dynamic stack allocation", "An alloca whose size is known only at run time: the " + "stack usage of the function grows with that value."}}, + {"AllocaUserControlled", + {"User-controlled stack allocation size", + "An alloca or variable-length array whose size depends on user-controlled input, " + "which may exhaust the stack."}}, + {"CommandInjection", + {"Command injection", + "A command that is not a literal reaches a function that runs it."}}, + {"CommandInjection.NonLiteralCommand", + {"Non-literal command", + "A command that is not a string literal reaches a function that runs it (system, " + "popen, an exec function), so that data may choose what runs."}}, + {"ConstParameterNotModified", + {"Parameter could be const", "A pointer or reference parameter that is never used to " + "modify the object it designates."}}, + {"ConstParameterNotModified.Pointer", + {"Pointer parameter could point to const", + "A pointer parameter that is never used to modify the object it points to: it could " + "point to const."}}, + {"ConstParameterNotModified.PointerConstOnly", + {"Const pointer parameter could point to const", + "A pointer parameter declared const whose pointed object is never modified: the " + "pointed type could be const too."}}, + {"ConstParameterNotModified.Reference", + {"Reference parameter could be const", + "A reference parameter that is never used to modify the object it refers to: it " + "could be a reference to const."}}, + {"ConstParameterNotModified.ReferenceRvaluePreferValue", + {"Rvalue reference parameter could be a value", + "An rvalue reference parameter that is never used to modify the object it refers to: " + "it could be passed by value or by reference to const."}}, + {"DuplicateIfCondition", + {"Duplicate if condition", "An else-if branch whose condition is equivalent to a " + "previous one, so that it can never run."}}, + {"FormatString.NonLiteral", + {"Non-literal format string", + "A format string that is not a literal reaches a printf-like function, so that data " + "may control the format."}}, + {"GlobalReadBeforeWrite", + {"Global buffer read before its first write", + "A global buffer that a function reads before it first writes it: the read sees the " + "static zero initialization or a stale value."}}, + {"InformationExposure.UninitializedStackBytes", + {"Uninitialized stack bytes exposed", + "A local variable whose bytes are not all initialized reaches an external sink, " + "which may leak memory contents."}}, + {"IntegerConversion.SignedToSize", + {"Signed value converted to a size", + "A signed value that may be negative is converted to the size of an allocation or a " + "copy."}}, + {"IntegerOverflow", {"Integer overflow", "An integer computation that may overflow."}}, + {"IntegerOverflow.SignedArithmetic", + {"Signed integer overflow", + "A signed arithmetic operation that may overflow, which is undefined behavior."}}, + {"IntegerOverflow.SizeComputation", + {"Integer overflow in a size computation", + "The size of an allocation or a copy is computed with an operation that may overflow " + "before the size is used."}}, + {"IntegerTruncation.SizeComputation", + {"Integer truncation in a size computation", + "The size of an allocation or a copy may be truncated to a narrower type before it " + "is used."}}, + {"InvalidBaseReconstruction", + {"Invalid base reconstruction", + "A pointer to an object rebuilt from a pointer to one of its members with an offset " + "that does not lead back to the object, which is undefined behavior."}}, + {"MemcpyWithStackDest", + {"Stack buffer overflow in a memory copy", + "A memcpy, memset or similar copy whose length may exceed the stack buffer it " + "writes."}}, + {"MultipleStoresToStackBuffer", + {"Multiple stores to a stack buffer", + "A stack buffer written many times in one function: each index is worth checking " + "against the buffer's bounds."}}, + {"NegativeStackIndex", + {"Negative stack index", "An index into a stack array that may be negative, so that " + "the access lands before the array."}}, + {"NullPointerDereference", + {"Null pointer dereference", "A pointer that may be null is dereferenced."}}, + {"OutOfBoundsRead", {"Out-of-bounds read", "A read past the end of a buffer."}}, + {"OutOfBoundsRead.HeapIndex", + {"Out-of-bounds read on a heap buffer", + "A read at an index that may be past the end of a heap buffer."}}, + {"OutOfBoundsRead.MissingNullTerminator", + {"String buffer without a null terminator", + "A string function reads a buffer that may not be null-terminated, past its end."}}, + {"Recursion.Detected", + {"Recursive function", "A recursive or mutually recursive function: its stack usage " + "grows with the depth of the recursion."}}, + {"Recursion.Unconditional", + {"Unconditional recursion", "A function that calls itself on every path, with no base " + "case: it overflows the stack at run time."}}, + {"ResourceLifetime.AnalysisIncomplete", + {"Resource analysis incomplete", "The resource ownership analysis did not converge in " + "this function: its resources are not checked."}}, + {"ResourceLifetime.DoubleRelease", + {"Double release", "A resource handle released twice, such as a file closed or memory " + "freed a second time."}}, + {"ResourceLifetime.IncompleteInterproc", + {"Inter-procedural resource analysis incomplete", + "A handle passed to a function the analysis could not follow: whether it is released " + "is not known."}}, + {"ResourceLifetime.MissingDestructorRelease", + {"Resource acquired in a constructor may leak", + "A resource a constructor acquires and the class's destructor does not release."}}, + {"ResourceLifetime.MissingRelease", + {"Resource leak", + "A resource acquired in a function and not released on every path out of it."}}, + {"ResourceLifetime.ReleasedHandleEscapes", + {"Released handle escapes", + "A handle that was released is returned or stored, so that it may be used later."}}, + {"ResourceLifetime.UseAfterRelease", + {"Use after release", "A resource handle used after it was released."}}, + {"ResourceLifetimeIssue", + {"Resource lifetime issue", + "A resource that may leak, be released twice or be used after it was released."}}, + {"SizeMinusOneWrite", + {"Size minus a constant as a length", + "A length computed as a size minus a constant, which may fall below the minimum of " + "its type before a write or a call."}}, + {"SizeofPitfall", + {"Suspicious sizeof", + "A sizeof applied to a pointer or an array parameter, which gives the size of the " + "pointer, where the size of the data was likely meant."}}, + {"StackBufferOverflow", + {"Stack buffer overflow", + "A read or a write at an index that may be past the end of a stack array."}}, + {"StackFrameTooLarge", + {"Stack frame too large", + "A function whose stack frame exceeds the configured limit."}}, + {"StackPointerEscape", + {"Stack address escapes", "The address of a local variable escapes its function, so " + "that it may be used after the stack frame is gone."}}, + {"TOCTOU.PathCheckThenUse", + {"Path checked then used", "A path checked (access, stat) and then used (open) by " + "separate calls: the file may change in between."}}, + {"TOCTOURace", + {"Time-of-check to time-of-use race", "A resource checked and then used by separate " + "operations: it may change in between."}}, + {"TypeConfusion", + {"Type confusion", "The same memory accessed through incompatible types."}}, + {"TypeConfusion.IncompatibleStructView", + {"Incompatible struct views of one pointer", + "The same pointer used as two incompatible struct types."}}, + {"UninitializedLocalRead", + {"Read of an uninitialized local variable", + "A local variable read before it is written on some path."}}, + {"UninitializedLocalRead.AnalysisIncomplete", + {"Uninitialized-read analysis incomplete", + "The uninitialized-read analysis did not converge in this function: its reads are " + "not checked."}}, + {"UninitializedLocalVariable", + {"Uninitialized local variable", + "A local variable declared without an initializer and never definitely written."}}, + {"UnsafeFunction.DeprecatedGets", + {"Use of gets", + "A call to gets, which cannot bound what it writes and was removed from C11."}}, + {"VLAUsage", + {"Variable-length array", + "A stack allocation whose size is not a compile-time constant."}}, + {"VariadicFormatMismatch", + {"Format and arguments mismatch", + "The arguments of a printf-like call do not match its format string."}}, + }; + const auto found = std::lower_bound(std::begin(kRules), std::end(kRules), ruleId, + [](const DescribedRule& rule, std::string_view id) + { return rule.ruleId < id; }); + if (found == std::end(kRules) || found->ruleId != ruleId) + return std::nullopt; + return found->description; + } } // namespace ctrace::stack From 52595e5ac172945abddeffc09f772cd88fcb22e2 Mon Sep 17 00:00:00 2001 From: Hugo Date: Thu, 22 Oct 2026 05:55:48 +0200 Subject: [PATCH 3/4] feat(sarif): give each rule its title and description A SARIF rule took its id as shortDescription and had no fullDescription, so GitHub code scanning named an alert after the id and explained nothing. It now takes the rule's title as shortDescription and its description as fullDescription, from describeRule; an id the catalog does not describe keeps itself as shortDescription, as before. Refs #242 --- src/report/ReportSerialization.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/report/ReportSerialization.cpp b/src/report/ReportSerialization.cpp index bdaa79d..d78514d 100644 --- a/src/report/ReportSerialization.cpp +++ b/src/report/ReportSerialization.cpp @@ -437,8 +437,14 @@ namespace ctrace::stack const auto& rule = rules[i]; os << " {\n"; os << " \"id\": \"" << jsonEscape(rule.id) << "\",\n"; - os << " \"shortDescription\": { \"text\": \"" << jsonEscape(rule.id) - << "\" }"; + // The title and the description of the rule, when the catalog has them; the id + // otherwise, as before. + const std::optional described = describeRule(rule.id); + os << " \"shortDescription\": { \"text\": \"" + << jsonEscape(described ? std::string(described->title) : rule.id) << "\" }"; + if (described) + os << ",\n \"fullDescription\": { \"text\": \"" + << jsonEscape(std::string(described->description)) << "\" }"; if (!rule.cwes.empty()) { // One score for the whole rule: the highest of its CWEs. From 3f6be92285e8dfaeafd02e5d7012e2d50618c64f Mon Sep 17 00:00:00 2001 From: Hugo Date: Thu, 22 Oct 2026 05:56:56 +0200 Subject: [PATCH 4/4] docs: announce the rule titles and descriptions in the changelog Refs #242 --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dfa54ce..50c326b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ Notable changes are grouped by release. See [all releases](https://github.com/Co ## Unreleased - Added an authors file and linked the repository documents from the README. +- SARIF rules carry a title (`shortDescription`) and a description (`fullDescription`) instead + of their id, and `describeRule` (`RuleCatalog.hpp`) gives them to programs linking the analyzer. ## v0.22.0