diff --git a/src/coretrace_python/cache.py b/src/coretrace_python/cache.py index a76a4c9..b8d7b54 100644 --- a/src/coretrace_python/cache.py +++ b/src/coretrace_python/cache.py @@ -42,7 +42,7 @@ from coretrace_python.semantic.symbols import SymbolId from coretrace_python.source import FileLocation, Location, SourceId, SourceSpan -CACHE_FORMAT = 16 +CACHE_FORMAT = 17 @dataclass(frozen=True) diff --git a/src/coretrace_python/engine.py b/src/coretrace_python/engine.py index c7f1648..3291d45 100644 --- a/src/coretrace_python/engine.py +++ b/src/coretrace_python/engine.py @@ -96,6 +96,7 @@ discover_sources, name_modules, project_symbol, + starting_summaries, ) from coretrace_python.ir import PYIR_SCHEMA_VERSION from coretrace_python.ir.defuse import DefUseAnalysis @@ -831,7 +832,20 @@ def _analyse_managers( counts = Counter(manager.module.name for manager in managers.values()) names = {name: manager.module.name for name, manager in managers.items() if counts[manager.module.name] == 1} - index = seed + # The members' functions start from the summaries ``SummaryAnalysis`` starts from. + # Left out, a call from one member to another is a call to an unknown symbol on the + # first iteration and a project call on the next: around a cycle of modules, the + # iteration then shifts by one module each time and never settles (#261). + index = SummaryIndex( + { + **seed.summaries, + **{ + project_symbol(module, function): summary + for name, module in names.items() + for function, summary in starting_summaries(managers[name].get(CallGraphAnalysis)).items() + }, + } + ) for manager in managers.values(): manager.provide(ProjectSummaries, index) for _ in range(MAX_PROJECT_ITERATIONS): diff --git a/src/coretrace_python/interprocedural/__init__.py b/src/coretrace_python/interprocedural/__init__.py index fc053b2..e27a800 100644 --- a/src/coretrace_python/interprocedural/__init__.py +++ b/src/coretrace_python/interprocedural/__init__.py @@ -40,6 +40,7 @@ TemplateCallsAnalysis, TemplateFilter, cleared_by, + starting_summaries, ) __all__ = [ @@ -78,4 +79,5 @@ "discover_sources", "name_modules", "project_symbol", + "starting_summaries", ] diff --git a/src/coretrace_python/interprocedural/summaries.py b/src/coretrace_python/interprocedural/summaries.py index 2250c6d..a5a5c33 100644 --- a/src/coretrace_python/interprocedural/summaries.py +++ b/src/coretrace_python/interprocedural/summaries.py @@ -791,6 +791,22 @@ def _aligned(dependencies: tuple[_T, ...], cleared: tuple[Cleared, ...]) -> Iter return zip(dependencies, (*cleared, *(() for _ in range(missing))), strict=True) +def _span_order(span: SourceSpan | None) -> tuple[object, ...]: + """Source spans in source order, an absent one first.""" + + if span is None: + return () + return (span.source_id, span.start_line, span.start_column, span.end_line or 0, span.end_column or 0) + + +def _record_order(call: ExternalCall) -> tuple[object, ...]: + """External calls in source order, by the key ``record`` merges them on. A summary + compared in its fixpoint must not depend on the order in which the rounds met its + calls: around a call cycle, that order can rotate forever (#261).""" + + return (_span_order(call.location), _span_order(call.call_site), _span_order(call.invoked_at), call.symbol) + + def summarize( name: str, function: FunctionIR, @@ -826,7 +842,7 @@ def summarize( name, len(function.parameters), problem.returns.parameters, - tuple(problem.external.values()), + tuple(sorted(problem.external.values(), key=_record_order)), return_externals=problem.returns.externals, mutations=tuple(mutations), side_effects=frozenset(problem.mutated_globals), @@ -839,6 +855,21 @@ def summarize( ) +def starting_summaries(graph: CallGraph) -> dict[str, FunctionSummary]: + """The summary each function of ``graph`` starts from in a fixpoint: a supported + function reaches nothing yet, the bottom its summary grows from; an unsupported one + passes every parameter to its return value and keeps that summary.""" + + table: dict[str, FunctionSummary] = {} + for name, function in graph.definitions.items(): + count = len(function.parameters) + if name in graph.unsupported: + table[name] = FunctionSummary(name, count, frozenset(range(count)), (), True) + else: + table[name] = FunctionSummary(name, count, NONE, ()) + return table + + class SummaryAnalysis(Analysis[SummaryTable]): name: ClassVar[str] = "interprocedural.summaries" requires: ClassVar[frozenset[AnyAnalysis]] = frozenset( @@ -860,20 +891,17 @@ def compute(cls, ctx: AnalysisContext) -> SummaryTable: project = ctx.get(ProjectSummaries).summaries clearing = ctx.get(ClearingAnalysis) templates = ctx.get(TemplateCallsAnalysis) - table: dict[str, FunctionSummary] = {} - supported: dict[str, tuple[FunctionIR, CFG, HeapFacts, Callable[[BlockId], bool]]] = {} - for name, function in graph.definitions.items(): - if name in graph.unsupported: - count = len(function.parameters) - table[name] = FunctionSummary(name, count, frozenset(range(count)), (), True) - else: - supported[name] = ( - ctx.get(SSAAnalysis, function), - ctx.get(CFGAnalysis, function), - ctx.get(HeapAnalysis, function), - ctx.get(ConstantPropagation, function).reachable, - ) - table[name] = FunctionSummary(name, len(function.parameters), NONE, ()) + table = starting_summaries(graph) + supported: dict[str, tuple[FunctionIR, CFG, HeapFacts, Callable[[BlockId], bool]]] = { + name: ( + ctx.get(SSAAnalysis, function), + ctx.get(CFGAnalysis, function), + ctx.get(HeapAnalysis, function), + ctx.get(ConstantPropagation, function).reachable, + ) + for name, function in graph.definitions.items() + if name not in graph.unsupported + } changed = True while changed: changed = False diff --git a/tests/test_summary_fixpoint.py b/tests/test_summary_fixpoint.py new file mode 100644 index 0000000..7ed97b9 --- /dev/null +++ b/tests/test_summary_fixpoint.py @@ -0,0 +1,226 @@ +"""Acceptance tests for issue #261: the summary fixpoints terminate, a module's and a +project's. + +A function's summary lists the external calls it reaches, its own and those of the +project functions it calls. Those of a callee used to be copied in the callee's order, so +around a cycle of three functions or more each function's order depended on the order +its callee had in the previous round: the content settled, the order rotated, and the +order-sensitive comparison that ends the fixpoint never held. The analysis then never +returned. A summary now lists its external calls in source order, whatever the order in +which the rounds met them. + +The modules of an import cycle iterate their summaries together. Their own functions +now start from the summaries a module's fixpoint starts from, so the iteration grows to +its fixpoint instead of running to its bound. +""" + +from __future__ import annotations + +import dataclasses +import json +import subprocess +import sys +from collections import Counter +from pathlib import Path + +import pytest + +from coretrace_python import engine +from coretrace_python.analysis import AnalysisManager +from coretrace_python.cfg import CFGAnalysis +from coretrace_python.frontend import build_hir +from coretrace_python.interprocedural import CallGraphAnalysis, FunctionSummary, SummaryAnalysis +from coretrace_python.interprocedural.summaries import summarize +from coretrace_python.ir.ssa import SSAAnalysis +from coretrace_python.source import SourceManager + +# A converging analysis of these files takes about a second: a run past this is one that +# would not end. +TIMEOUT = 60 + +THREE_FUNCTIONS = """\ +import os + + +def a(x): + b(x) + os.system(x) + + +def b(x): + c(x) + print(x) + + +def c(x): + a(x) + + +def run(): + a(input()) +""" + +FOUR_FUNCTIONS = """\ +import os + + +def a(x): + b(x) + os.system(x) + + +def b(x): + c(x) + print(x) + + +def c(x): + d(x) + + +def d(x): + a(x) + + +def run(): + a(input()) +""" + +# The shape of ansible's ``Role.load``, ``_load_role_data`` and ``_load_dependencies``. +EXCEPTION_HANDLERS = """\ +import os + + +def load(include): + try: + load_data(include) + except RuntimeError: + raise ValueError(include.ds) + + +def load_data(include): + load_dependencies(include) + try: + os.system(include) + except AssertionError: + raise ValueError(include.name) + + +def load_dependencies(include): + load(include) + + +def run(): + load(input()) +""" + + +def checked(tmp_path: Path, source: str) -> list[tuple[str, int, str | None]]: + """The findings of ``--check`` on a project of one module, failing the test instead of + waiting when the analysis does not end.""" + + root = tmp_path / "project" + root.mkdir() + (root / "app.py").write_text(source, encoding="utf-8") + try: + completed = subprocess.run( + [sys.executable, "-m", "coretrace_python", "--check", str(root), "--format", "json"], + capture_output=True, + text=True, + check=False, + timeout=TIMEOUT, + ) + except subprocess.TimeoutExpired: + pytest.fail(f"the analysis did not end within {TIMEOUT} s") + report = json.loads(completed.stdout) + return [(f["rule_id"], f["location"]["line"], f["metadata"].get("verdict")) for f in report["findings"]] + + +@pytest.mark.parametrize( + ("source", "line"), + [(THREE_FUNCTIONS, 19), (FOUR_FUNCTIONS, 23), (EXCEPTION_HANDLERS, 24)], + ids=["three-functions", "four-functions", "exception-handlers"], +) +def test_a_call_cycle_with_external_calls_after_the_recursive_call_is_checked( + tmp_path: Path, source: str, line: int +) -> None: + assert checked(tmp_path, source) == [("command-injection", line, "vulnerability")] + + +CALLEE_FIRST = """\ +import os + + +def run(cmd): + helper(cmd) + os.system(cmd) + + +def helper(cmd): + print(cmd) + os.remove(cmd) +""" + + +def test_a_summary_lists_its_external_calls_in_source_order() -> None: + # ``run`` meets ``helper``'s calls first, at line 5, then its own at line 6. + table = engine.build_manager(build_hir(SourceManager().add_source("app.py", CALLEE_FIRST))).get(SummaryAnalysis) + + assert [(str(c.symbol), c.location.start_line) for c in table.summary("run").external_calls] == [ + ("python.os.system", 6), + ("python.builtins.print", 10), + ("python.os.remove", 11), + ] + + +def test_a_summary_does_not_depend_on_the_order_of_its_callee_s_external_calls() -> None: + manager = engine.build_manager(build_hir(SourceManager().add_source("app.py", CALLEE_FIRST))) + graph = manager.get(CallGraphAnalysis) + summaries = manager.get(SummaryAnalysis) + table = {name: summaries.summary(name) for name in summaries.names} + helper = table["helper"] + reversed_helper = dataclasses.replace(helper, external_calls=helper.external_calls[::-1]) + run = graph.definitions["run"] + + def run_summary(callee): + return summarize( + "run", manager.get(SSAAnalysis, run), manager.get(CFGAnalysis, run), graph, {**table, "helper": callee} + ) + + assert helper.external_calls != reversed_helper.external_calls + assert run_summary(reversed_helper) == run_summary(helper) + + +# The modules of an import cycle are analysed together, their summaries iterated until +# the index of the project's summaries stops changing. ``THREE_FUNCTIONS`` across three +# modules: a call from one member to another used to be a call to an unknown symbol on +# the first iteration and a project call on the next, and the iteration never settled +# before its bound. +CYCLE_OF_MODULES = { + "pkg/__init__.py": "", + "pkg/a.py": "import os\n\nfrom pkg import b\n\n\ndef a(x):\n b.b(x)\n os.system(x)\n\n\ndef run():\n a(input())\n", + "pkg/b.py": "from pkg import c\n\n\ndef b(x):\n c.c(x)\n print(x)\n", + "pkg/c.py": "from pkg import a\n\n\ndef c(x):\n a.a(x)\n", +} + + +def test_a_call_cycle_across_modules_converges_before_the_bound(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + root = tmp_path / "project" + for path, text in CYCLE_OF_MODULES.items(): + (root / path).parent.mkdir(parents=True, exist_ok=True) + (root / path).write_text(text, encoding="utf-8") + extractions: Counter[str] = Counter() + summaries_of = engine._summaries_of + + def counted(manager: AnalysisManager) -> dict[str, FunctionSummary]: + extractions[manager.module.name] += 1 + return summaries_of(manager) + + monkeypatch.setattr(engine, "_summaries_of", counted) + analysis = engine.analyze_project(root, [engine.BUNDLED_PLUGINS]) + + # Once per iteration, and once more for the module's results. + assert max(extractions[name] for name in ("pkg.a", "pkg.b", "pkg.c")) <= engine.MAX_PROJECT_ITERATIONS + assert [ + (f.rule_id, Path(str(f.span.source_id)).name, f.span.start_line, f.metadata["verdict"]) for f in analysis.findings + ] == [("command-injection", "a.py", 12, "vulnerability")]