Fix audit findings: soundness, coverage reporting, docs, and review follow-ups #1
11 changed files with 54 additions and 66 deletions
refactor(cli): one TU path normalizer for coverage, fan-out and quarantine
Problem: normalizeClangTuPath, normalizeQuarantinePath and fan-out's normalizePath were three copies of the same absolute + weakly_canonical routine. Coverage, worker records and the quarantine file must agree on a TU's spelling, and three copies can drift apart. Fix: move the routine to fold/cli/tu_path.hpp as normalizeTuPath and use it at every site. No behaviour change. Test: existing test_scan_coverage (path spellings), test_tu_quarantine and test_subprocess_fanout, updated to the new name. Full suite: 140/140.
commit
fcf474e4f0
|
|
@ -35,10 +35,6 @@ struct ClangShardTuHooks {
|
||||||
|
|
||||||
[[nodiscard]] std::string currentClangTuPath(const clang::ASTContext& ctx);
|
[[nodiscard]] std::string currentClangTuPath(const clang::ASTContext& ctx);
|
||||||
|
|
||||||
// Absolute, symlink-resolved form used to match TU paths across the compile
|
|
||||||
// DB, the front end, and worker records.
|
|
||||||
[[nodiscard]] std::string normalizeClangTuPath(const std::string& path);
|
|
||||||
|
|
||||||
// `gap_reason` is empty when the TU was analyzed on a clean AST; otherwise it
|
// `gap_reason` is empty when the TU was analyzed on a clean AST; otherwise it
|
||||||
// is a CoverageGap reason ("compile-errors", "analysis-failed").
|
// is a CoverageGap reason ("compile-errors", "analysis-failed").
|
||||||
struct ClangShardTuOutcome {
|
struct ClangShardTuOutcome {
|
||||||
|
|
|
||||||
11
include/fold/cli/tu_path.hpp
Normal file
11
include/fold/cli/tu_path.hpp
Normal file
|
|
@ -0,0 +1,11 @@
|
||||||
|
#pragma once
|
||||||
|
|
||||||
|
#include <string>
|
||||||
|
|
||||||
|
namespace fold::cli {
|
||||||
|
|
||||||
|
// Absolute, symlink-resolved form used to match TU paths across the compile
|
||||||
|
// DB, the front end, worker records and the quarantine file.
|
||||||
|
[[nodiscard]] std::string normalizeTuPath(const std::string& path);
|
||||||
|
|
||||||
|
} // namespace fold::cli
|
||||||
|
|
@ -4,9 +4,9 @@
|
||||||
#include <string_view>
|
#include <string_view>
|
||||||
#include <unordered_set>
|
#include <unordered_set>
|
||||||
|
|
||||||
namespace fold::cli {
|
#include "fold/cli/tu_path.hpp"
|
||||||
|
|
||||||
[[nodiscard]] std::string normalizeQuarantinePath(const std::string& path);
|
namespace fold::cli {
|
||||||
|
|
||||||
/// Read quarantine.txt from `quarantine_dir`.
|
/// Read quarantine.txt from `quarantine_dir`.
|
||||||
///
|
///
|
||||||
|
|
|
||||||
|
|
@ -294,6 +294,7 @@ fold_core_sources += files(
|
||||||
'src/cli/source_summary_pipeline.cpp',
|
'src/cli/source_summary_pipeline.cpp',
|
||||||
'src/cli/source_tu_runner.cpp',
|
'src/cli/source_tu_runner.cpp',
|
||||||
'src/cli/tu_quarantine.cpp',
|
'src/cli/tu_quarantine.cpp',
|
||||||
|
'src/cli/tu_path.cpp',
|
||||||
'src/cli/detector_plan.cpp',
|
'src/cli/detector_plan.cpp',
|
||||||
'src/cli/detector_profile_accum.cpp',
|
'src/cli/detector_profile_accum.cpp',
|
||||||
'src/cli/finding_evidence.cpp',
|
'src/cli/finding_evidence.cpp',
|
||||||
|
|
|
||||||
|
|
@ -13,6 +13,7 @@
|
||||||
|
|
||||||
#include "fold/cli/scan_options.hpp"
|
#include "fold/cli/scan_options.hpp"
|
||||||
#include "fold/cli/signal_isolation.hpp"
|
#include "fold/cli/signal_isolation.hpp"
|
||||||
|
#include "fold/cli/tu_path.hpp"
|
||||||
#include "fold/report/finding.hpp"
|
#include "fold/report/finding.hpp"
|
||||||
#include "fold/report/finding_json.hpp"
|
#include "fold/report/finding_json.hpp"
|
||||||
#include "fold/report/scan_coverage.hpp"
|
#include "fold/report/scan_coverage.hpp"
|
||||||
|
|
@ -61,20 +62,6 @@ void emitWorkerTuPayload(std::ostream& out, const ClangShardTuState& state,
|
||||||
|
|
||||||
} // namespace
|
} // namespace
|
||||||
|
|
||||||
std::string normalizeClangTuPath(const std::string& path) {
|
|
||||||
if (path.empty())
|
|
||||||
return path;
|
|
||||||
std::error_code ec;
|
|
||||||
std::filesystem::path p(path);
|
|
||||||
std::filesystem::path abs = p.is_absolute() ? p : std::filesystem::absolute(p, ec);
|
|
||||||
if (ec)
|
|
||||||
return p.lexically_normal().string();
|
|
||||||
auto canonical = std::filesystem::weakly_canonical(abs, ec);
|
|
||||||
if (!ec)
|
|
||||||
return canonical.string();
|
|
||||||
return abs.lexically_normal().string();
|
|
||||||
}
|
|
||||||
|
|
||||||
std::string currentClangTuPath(const clang::ASTContext& ctx) {
|
std::string currentClangTuPath(const clang::ASTContext& ctx) {
|
||||||
const auto& sm = ctx.getSourceManager();
|
const auto& sm = ctx.getSourceManager();
|
||||||
auto fid = sm.getMainFileID();
|
auto fid = sm.getMainFileID();
|
||||||
|
|
@ -85,7 +72,7 @@ std::string currentClangTuPath(const clang::ASTContext& ctx) {
|
||||||
|
|
||||||
ClangShardTuOutcome runClangShardTranslationUnit(const ScanOptions& opts, clang::ASTContext& ctx,
|
ClangShardTuOutcome runClangShardTranslationUnit(const ScanOptions& opts, clang::ASTContext& ctx,
|
||||||
ClangShardTuState state, ClangShardTuHooks hooks) {
|
ClangShardTuState state, ClangShardTuHooks hooks) {
|
||||||
const std::string tu_path = normalizeClangTuPath(currentClangTuPath(ctx));
|
const std::string tu_path = normalizeTuPath(currentClangTuPath(ctx));
|
||||||
ClangShardTuOutcome outcome{tu_path, "analysis-failed"};
|
ClangShardTuOutcome outcome{tu_path, "analysis-failed"};
|
||||||
if (!state.summaries || !state.func_summaries || !state.findings ||
|
if (!state.summaries || !state.func_summaries || !state.findings ||
|
||||||
!state.quarantined_this_run) {
|
!state.quarantined_this_run) {
|
||||||
|
|
@ -156,12 +143,12 @@ ScanCoverage tuCoverage(const std::vector<std::string>& requested,
|
||||||
const std::vector<ClangShardTuOutcome>& outcomes) {
|
const std::vector<ClangShardTuOutcome>& outcomes) {
|
||||||
std::unordered_map<std::string, std::string> gap_by_path;
|
std::unordered_map<std::string, std::string> gap_by_path;
|
||||||
for (const auto& outcome : outcomes)
|
for (const auto& outcome : outcomes)
|
||||||
gap_by_path.insert_or_assign(normalizeClangTuPath(outcome.path), outcome.gap_reason);
|
gap_by_path.insert_or_assign(normalizeTuPath(outcome.path), outcome.gap_reason);
|
||||||
ScanCoverage coverage;
|
ScanCoverage coverage;
|
||||||
coverage.tracked = true;
|
coverage.tracked = true;
|
||||||
coverage.tus_attempted = requested.size();
|
coverage.tus_attempted = requested.size();
|
||||||
for (const auto& file : requested) {
|
for (const auto& file : requested) {
|
||||||
const auto it = gap_by_path.find(normalizeClangTuPath(file));
|
const auto it = gap_by_path.find(normalizeTuPath(file));
|
||||||
if (it == gap_by_path.end())
|
if (it == gap_by_path.end())
|
||||||
coverage.gaps.push_back({file, "frontend-failed"});
|
coverage.gaps.push_back({file, "frontend-failed"});
|
||||||
else if (!it->second.empty())
|
else if (!it->second.empty())
|
||||||
|
|
|
||||||
|
|
@ -345,7 +345,7 @@ ShardResult runClangDetectorsOnShard(clang::tooling::CompilationDatabase& db,
|
||||||
files.reserve(files_in.size());
|
files.reserve(files_in.size());
|
||||||
std::vector<std::string> skipped;
|
std::vector<std::string> skipped;
|
||||||
for (const auto& f : files_in) {
|
for (const auto& f : files_in) {
|
||||||
if (quarantined.count(normalizeQuarantinePath(f)) != 0u) {
|
if (quarantined.count(normalizeTuPath(f)) != 0u) {
|
||||||
skipped.push_back(f);
|
skipped.push_back(f);
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -24,6 +24,7 @@
|
||||||
#include <vector>
|
#include <vector>
|
||||||
|
|
||||||
#include "fold/cli/clang_shard_tu.hpp"
|
#include "fold/cli/clang_shard_tu.hpp"
|
||||||
|
#include "fold/cli/tu_path.hpp"
|
||||||
#include "fold/cli/tu_quarantine.hpp"
|
#include "fold/cli/tu_quarantine.hpp"
|
||||||
#include "fold/detect/clone.hpp"
|
#include "fold/detect/clone.hpp"
|
||||||
#include "fold/detect/equiv.hpp"
|
#include "fold/detect/equiv.hpp"
|
||||||
|
|
@ -46,22 +47,6 @@ namespace {
|
||||||
|
|
||||||
using json = nlohmann::json;
|
using json = nlohmann::json;
|
||||||
|
|
||||||
std::string normalizePath(const std::string& path) {
|
|
||||||
if (path.empty())
|
|
||||||
return path;
|
|
||||||
std::error_code ec;
|
|
||||||
std::filesystem::path p(path);
|
|
||||||
std::filesystem::path abs = p.is_absolute() ? p : std::filesystem::absolute(p, ec);
|
|
||||||
if (ec) {
|
|
||||||
return p.lexically_normal().string();
|
|
||||||
}
|
|
||||||
auto canonical = std::filesystem::weakly_canonical(abs, ec);
|
|
||||||
if (!ec) {
|
|
||||||
return canonical.string();
|
|
||||||
}
|
|
||||||
return abs.lexically_normal().string();
|
|
||||||
}
|
|
||||||
|
|
||||||
struct ChildProc {
|
struct ChildProc {
|
||||||
pid_t pid = -1;
|
pid_t pid = -1;
|
||||||
int stdin_fd = -1;
|
int stdin_fd = -1;
|
||||||
|
|
@ -170,7 +155,7 @@ ShardOutput runShardWithRecovery(const std::string& binary,
|
||||||
} else if (kind == "func_summary") {
|
} else if (kind == "func_summary") {
|
||||||
local_func_summaries.push_back(fromJson(data));
|
local_func_summaries.push_back(fromJson(data));
|
||||||
} else if (kind == "file_finished") {
|
} else if (kind == "file_finished") {
|
||||||
finished_files.insert(normalizePath(data.value("path", "")));
|
finished_files.insert(normalizeTuPath(data.value("path", "")));
|
||||||
result.tu_outcomes.push_back(
|
result.tu_outcomes.push_back(
|
||||||
{data.value("path", ""), data.value("gap", "")});
|
{data.value("path", ""), data.value("gap", "")});
|
||||||
}
|
}
|
||||||
|
|
@ -210,7 +195,7 @@ ShardOutput runShardWithRecovery(const std::string& binary,
|
||||||
std::string failed_file;
|
std::string failed_file;
|
||||||
std::size_t failed_index = shard_files.size();
|
std::size_t failed_index = shard_files.size();
|
||||||
for (std::size_t i = 0; i < shard_files.size(); ++i) {
|
for (std::size_t i = 0; i < shard_files.size(); ++i) {
|
||||||
if (finished_files.count(normalizePath(shard_files[i])) == 0) {
|
if (finished_files.count(normalizeTuPath(shard_files[i])) == 0) {
|
||||||
failed_file = shard_files[i];
|
failed_file = shard_files[i];
|
||||||
failed_index = i;
|
failed_index = i;
|
||||||
break;
|
break;
|
||||||
|
|
|
||||||
23
src/cli/tu_path.cpp
Normal file
23
src/cli/tu_path.cpp
Normal file
|
|
@ -0,0 +1,23 @@
|
||||||
|
#include "fold/cli/tu_path.hpp"
|
||||||
|
|
||||||
|
#include <filesystem>
|
||||||
|
#include <system_error>
|
||||||
|
|
||||||
|
namespace fold::cli {
|
||||||
|
|
||||||
|
std::string normalizeTuPath(const std::string& path) {
|
||||||
|
if (path.empty())
|
||||||
|
return path;
|
||||||
|
std::error_code error;
|
||||||
|
std::filesystem::path input(path);
|
||||||
|
std::filesystem::path absolute =
|
||||||
|
input.is_absolute() ? input : std::filesystem::absolute(input, error);
|
||||||
|
if (error)
|
||||||
|
return input.lexically_normal().string();
|
||||||
|
auto canonical = std::filesystem::weakly_canonical(absolute, error);
|
||||||
|
if (!error)
|
||||||
|
return canonical.string();
|
||||||
|
return absolute.lexically_normal().string();
|
||||||
|
}
|
||||||
|
|
||||||
|
} // namespace fold::cli
|
||||||
|
|
@ -21,21 +21,6 @@ bool hasSignalSuffix(const std::string& line, std::size_t colon) {
|
||||||
|
|
||||||
} // namespace
|
} // namespace
|
||||||
|
|
||||||
std::string normalizeQuarantinePath(const std::string& path) {
|
|
||||||
if (path.empty())
|
|
||||||
return path;
|
|
||||||
std::error_code error;
|
|
||||||
std::filesystem::path input(path);
|
|
||||||
std::filesystem::path absolute =
|
|
||||||
input.is_absolute() ? input : std::filesystem::absolute(input, error);
|
|
||||||
if (error)
|
|
||||||
return input.lexically_normal().string();
|
|
||||||
auto canonical = std::filesystem::weakly_canonical(absolute, error);
|
|
||||||
if (!error)
|
|
||||||
return canonical.string();
|
|
||||||
return absolute.lexically_normal().string();
|
|
||||||
}
|
|
||||||
|
|
||||||
std::unordered_set<std::string> loadQuarantine(const std::string& quarantine_dir) {
|
std::unordered_set<std::string> loadQuarantine(const std::string& quarantine_dir) {
|
||||||
std::unordered_set<std::string> paths;
|
std::unordered_set<std::string> paths;
|
||||||
if (quarantine_dir.empty())
|
if (quarantine_dir.empty())
|
||||||
|
|
@ -61,7 +46,7 @@ std::unordered_set<std::string> loadQuarantine(const std::string& quarantine_dir
|
||||||
const auto colon = line.rfind(':');
|
const auto colon = line.rfind(':');
|
||||||
std::string path = hasSignalSuffix(line, colon) ? line.substr(0, colon) : line;
|
std::string path = hasSignalSuffix(line, colon) ? line.substr(0, colon) : line;
|
||||||
if (!path.empty())
|
if (!path.empty())
|
||||||
paths.insert(normalizeQuarantinePath(path));
|
paths.insert(normalizeTuPath(path));
|
||||||
}
|
}
|
||||||
return paths;
|
return paths;
|
||||||
}
|
}
|
||||||
|
|
@ -70,7 +55,7 @@ void appendQuarantineFailure(const std::string& quarantine_dir, const std::strin
|
||||||
std::string_view cause, int code) {
|
std::string_view cause, int code) {
|
||||||
if (quarantine_dir.empty() || tu_path.empty())
|
if (quarantine_dir.empty() || tu_path.empty())
|
||||||
return;
|
return;
|
||||||
const std::string normalized = normalizeQuarantinePath(tu_path);
|
const std::string normalized = normalizeTuPath(tu_path);
|
||||||
if (loadQuarantine(quarantine_dir).contains(normalized))
|
if (loadQuarantine(quarantine_dir).contains(normalized))
|
||||||
return;
|
return;
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -200,8 +200,8 @@ TEST_CASE("quarantined TUs are coverage gaps in serial and fan-out scans",
|
||||||
CHECK(coverage.tus_attempted == 3);
|
CHECK(coverage.tus_attempted == 3);
|
||||||
CHECK(coverage.tus_analyzed == 2);
|
CHECK(coverage.tus_analyzed == 2);
|
||||||
REQUIRE(coverage.gaps.size() == 1);
|
REQUIRE(coverage.gaps.size() == 1);
|
||||||
CHECK(fold::cli::normalizeQuarantinePath(coverage.gaps[0].file) ==
|
CHECK(fold::cli::normalizeTuPath(coverage.gaps[0].file) ==
|
||||||
fold::cli::normalizeQuarantinePath(files[0]));
|
fold::cli::normalizeTuPath(files[0]));
|
||||||
CHECK(coverage.gaps[0].reason == "quarantined");
|
CHECK(coverage.gaps[0].reason == "quarantined");
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|
@ -260,8 +260,8 @@ TEST_CASE("SubprocessFanout isolates sequential frontend failures and preserves
|
||||||
std::vector<std::string>{"fanout-test:after.cpp", "fanout-test:before.cpp"});
|
std::vector<std::string>{"fanout-test:after.cpp", "fanout-test:before.cpp"});
|
||||||
|
|
||||||
const auto quarantined = fold::cli::loadQuarantine(quarantine.string());
|
const auto quarantined = fold::cli::loadQuarantine(quarantine.string());
|
||||||
REQUIRE(quarantined.contains(fold::cli::normalizeQuarantinePath(files[1])));
|
REQUIRE(quarantined.contains(fold::cli::normalizeTuPath(files[1])));
|
||||||
REQUIRE(quarantined.contains(fold::cli::normalizeQuarantinePath(files[2])));
|
REQUIRE(quarantined.contains(fold::cli::normalizeTuPath(files[2])));
|
||||||
std::ifstream input(quarantine / "quarantine.txt");
|
std::ifstream input(quarantine / "quarantine.txt");
|
||||||
const std::string records((std::istreambuf_iterator<char>(input)),
|
const std::string records((std::istreambuf_iterator<char>(input)),
|
||||||
std::istreambuf_iterator<char>());
|
std::istreambuf_iterator<char>());
|
||||||
|
|
|
||||||
|
|
@ -36,7 +36,7 @@ TEST_CASE("TU quarantine loads signal suffixes and ignores comments", "[cli][qua
|
||||||
|
|
||||||
const auto paths = fold::cli::loadQuarantine(temp.path().string());
|
const auto paths = fold::cli::loadQuarantine(temp.path().string());
|
||||||
REQUIRE(paths.size() == 1);
|
REQUIRE(paths.size() == 1);
|
||||||
CHECK(paths.contains(fold::cli::normalizeQuarantinePath(source.string())));
|
CHECK(paths.contains(fold::cli::normalizeTuPath(source.string())));
|
||||||
}
|
}
|
||||||
|
|
||||||
TEST_CASE("TU quarantine append is normalized and idempotent", "[cli][quarantine]") {
|
TEST_CASE("TU quarantine append is normalized and idempotent", "[cli][quarantine]") {
|
||||||
|
|
@ -51,11 +51,11 @@ TEST_CASE("TU quarantine append is normalized and idempotent", "[cli][quarantine
|
||||||
|
|
||||||
const auto paths = fold::cli::loadQuarantine(temp.path().string());
|
const auto paths = fold::cli::loadQuarantine(temp.path().string());
|
||||||
REQUIRE(paths.size() == 1);
|
REQUIRE(paths.size() == 1);
|
||||||
CHECK(paths.contains(fold::cli::normalizeQuarantinePath(source.string())));
|
CHECK(paths.contains(fold::cli::normalizeTuPath(source.string())));
|
||||||
|
|
||||||
std::ifstream input(temp.path() / "quarantine.txt");
|
std::ifstream input(temp.path() / "quarantine.txt");
|
||||||
std::string line;
|
std::string line;
|
||||||
REQUIRE(std::getline(input, line));
|
REQUIRE(std::getline(input, line));
|
||||||
CHECK(line == fold::cli::normalizeQuarantinePath(source.string()) + "\tsignal:6");
|
CHECK(line == fold::cli::normalizeTuPath(source.string()) + "\tsignal:6");
|
||||||
CHECK_FALSE(std::getline(input, line));
|
CHECK_FALSE(std::getline(input, line));
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue