diff --git a/defects/ansible/bench/bench-ansible-0001.py b/defects/ansible/bench/bench-ansible-0001.py index 29175fa1a..6fb4ae6ea 100644 --- a/defects/ansible/bench/bench-ansible-0001.py +++ b/defects/ansible/bench/bench-ansible-0001.py @@ -1,47 +1,109 @@ #!/usr/bin/env python3 # bench-ansible-0001.py -# Role.get_vars() seen-list O(D²) deduplication -# Models O(N*k) -> O(N+k) via linear-scan membership inside a loop vs set/dict. +# Role.get_vars() seen-list O(D^2) deduplication over transitive dependencies. +# +# Models Role-like objects with __eq__ defined (value equality over a hash-dict +# subset). The defective path mirrors `seen = []; if dep not in seen: seen.append(dep)` +# where `in` calls __eq__ against every item already in the list — O(D) per +# iteration, O(D^2) total. +# +# The fixed path requires Role to be hashable. Upstream Role defines __eq__ but +# no __hash__, which implicitly sets __hash__ to None (unhashable). The real fix +# ships two coupled changes: add __hash__ hashing (name, path), then swap +# `seen = []` for `seen = set()` / `seen.add(dep)`. Set membership becomes O(1) +# amortized — O(D) total. +# +# A third function, bench_naive_set_fails, demonstrates why a naive list->set +# swap (without adding __hash__) raises TypeError on the first .add() call. import sys import time -def bench_defective(n, k): - pool = list(range(k)) - items = list(range(n)) +class MockRoleEqOnly: + """Role with __eq__, no __hash__. Unhashable by default.""" + __slots__ = ("name", "path") + + def __init__(self, name, path): + self.name = name + self.path = path + + def __eq__(self, other): + if not isinstance(other, MockRoleEqOnly): + return False + return self.name == other.name and self.path == other.path + + +class MockRoleWithHash: + """Role with __eq__ and __hash__ over (name, path). Hashable, O(1) set dedup.""" + __slots__ = ("name", "path") + + def __init__(self, name, path): + self.name = name + self.path = path + + def __eq__(self, other): + if not isinstance(other, MockRoleWithHash): + return False + return self.name == other.name and self.path == other.path + + def __hash__(self): + return hash((self.name, self.path)) + + +def build_deps(cls, d): + """Build D role-like deps. Names/paths unique so worst-case seen-growth applies.""" + return [cls(f"role_{i}", f"/etc/ansible/roles/role_{i}") for i in range(d)] + + +def bench_defective(d, _k_unused): + deps = build_deps(MockRoleEqOnly, d) t0 = time.perf_counter() seen = [] - for x in items: - if x not in pool: # O(k) - seen.append(x) + for dep in deps: + if dep not in seen: + seen.append(dep) return time.perf_counter() - t0 -def bench_fixed(n, k): - pool_set = set(range(k)) - items = list(range(n)) +def bench_fixed(d, _k_unused): + deps = build_deps(MockRoleWithHash, d) t0 = time.perf_counter() - seen = [] - for x in items: - if x not in pool_set: # O(1) - seen.append(x) + seen = set() + for dep in deps: + if dep not in seen: + seen.add(dep) return time.perf_counter() - t0 +def bench_naive_set_fails(): + """Demonstrates naive list->set swap without adding __hash__ raises TypeError.""" + deps = build_deps(MockRoleEqOnly, 2) + seen = set() + try: + seen.add(deps[0]) + return "NAIVE SET WORKED (unexpected)" + except TypeError as exc: + return f"NAIVE SET FAILS: TypeError: {exc}" + + TRIALS = 3 CASES = [(100, 100), (500, 500), (1000, 1000), (2000, 2000)] def run(): lines = [] - header = "=== ansible-0001: Role.get_vars() seen-list O(D²) deduplication ===" + header = "=== ansible-0001: Role.get_vars() seen-list O(D^2) deduplication ===" print(header); lines.append(header) - for n, k in CASES: - df = min(bench_defective(n, k) for _ in range(TRIALS)) - fx = min(bench_fixed(n, k) for _ in range(TRIALS)) + + fail_note = bench_naive_set_fails() + print(fail_note); lines.append(fail_note); sys.stdout.flush() + + for d, k in CASES: + df = min(bench_defective(d, k) for _ in range(TRIALS)) + fx = min(bench_fixed(d, k) for _ in range(TRIALS)) speedup = (df / fx) if fx > 0 else float("inf") - line = f"N={n:<5} k={k:<5}: defective={df*1000:.3f}ms fixed={fx*1000:.3f}ms speedup={speedup:.1f}x" + line = f"D={d:<5} k={k:<5}: defective={df*1000:.3f}ms fixed={fx*1000:.3f}ms speedup={speedup:.1f}x" print(line); lines.append(line); sys.stdout.flush() return lines diff --git a/defects/ansible/bench/results.txt b/defects/ansible/bench/results.txt index df194c9b1..504b4a73f 100644 --- a/defects/ansible/bench/results.txt +++ b/defects/ansible/bench/results.txt @@ -1,30 +1,31 @@ -=== ansible-0001: Role.get_vars() seen-list O(D²) deduplication === -N=100 k=100 : defective=0.084ms fixed=0.003ms speedup=24.5x -N=500 k=500 : defective=2.136ms fixed=0.021ms speedup=102.6x -N=1000 k=1000 : defective=8.704ms fixed=0.046ms speedup=189.6x -N=2000 k=2000 : defective=34.080ms fixed=0.094ms speedup=362.0x +=== ansible-0001: Role.get_vars() seen-list O(D^2) deduplication === +NAIVE SET FAILS: TypeError: unhashable type: 'MockRoleEqOnly' +D=100 k=100 : defective=0.543ms fixed=0.056ms speedup=9.7x +D=500 k=500 : defective=14.715ms fixed=0.195ms speedup=75.6x +D=1000 k=1000 : defective=55.104ms fixed=0.368ms speedup=149.7x +D=2000 k=2000 : defective=204.608ms fixed=0.755ms speedup=270.9x === ansible-0002: linear scan on list inside loops — O(A*G) per add_group call, O(H*G*A) total === -N=100 k=100 : defective=0.081ms fixed=0.003ms speedup=24.9x -N=500 k=500 : defective=2.031ms fixed=0.020ms speedup=104.0x -N=1000 k=1000 : defective=8.378ms fixed=0.044ms speedup=190.5x -N=2000 k=2000 : defective=33.762ms fixed=0.093ms speedup=362.4x +N=100 k=100 : defective=0.055ms fixed=0.002ms speedup=25.0x +N=500 k=500 : defective=1.394ms fixed=0.014ms speedup=102.3x +N=1000 k=1000 : defective=5.795ms fixed=0.032ms speedup=181.7x +N=2000 k=2000 : defective=22.358ms fixed=0.064ms speedup=350.1x === ansible-0003: on list — O(H) per notification, O(H^2) total across all hosts === -N=100 k=100 : defective=0.081ms fixed=0.003ms speedup=24.6x -N=500 k=500 : defective=2.029ms fixed=0.020ms speedup=102.4x -N=1000 k=1000 : defective=8.495ms fixed=0.046ms speedup=184.9x -N=2000 k=2000 : defective=34.478ms fixed=0.092ms speedup=373.0x +N=100 k=100 : defective=0.055ms fixed=0.002ms speedup=24.6x +N=500 k=500 : defective=1.396ms fixed=0.013ms speedup=104.0x +N=1000 k=1000 : defective=5.909ms fixed=0.030ms speedup=199.2x +N=2000 k=2000 : defective=24.304ms fixed=0.063ms speedup=386.1x === ansible-0004: Defect: re.compile(pattern[1:]) called with user-supplied ~-prefix inventory pattern. === -N=100 k=100 : defective=0.137ms fixed=0.003ms speedup=42.3x -N=500 k=500 : defective=2.036ms fixed=0.019ms speedup=104.7x -N=1000 k=1000 : defective=8.999ms fixed=0.046ms speedup=194.2x -N=2000 k=2000 : defective=36.177ms fixed=0.096ms speedup=375.6x +N=100 k=100 : defective=0.057ms fixed=0.002ms speedup=24.2x +N=500 k=500 : defective=1.762ms fixed=0.033ms speedup=53.3x +N=1000 k=1000 : defective=6.508ms fixed=0.032ms speedup=204.9x +N=2000 k=2000 : defective=24.040ms fixed=0.063ms speedup=378.7x === ansible-0005: CWE-407: list-scan inside loop in ansible-0005 (generic model) === -N=100 k=100 : defective=0.084ms fixed=0.003ms speedup=25.3x -N=500 k=500 : defective=2.188ms fixed=0.021ms speedup=104.7x -N=1000 k=1000 : defective=8.749ms fixed=0.043ms speedup=203.5x -N=2000 k=2000 : defective=34.284ms fixed=0.103ms speedup=332.8x +N=100 k=100 : defective=0.057ms fixed=0.002ms speedup=24.7x +N=500 k=500 : defective=1.400ms fixed=0.014ms speedup=101.2x +N=1000 k=1000 : defective=5.644ms fixed=0.031ms speedup=184.7x +N=2000 k=2000 : defective=23.457ms fixed=0.063ms speedup=370.0x diff --git a/defects/ansible/patch/ansible-0001-role-get-vars-seen-list.md b/defects/ansible/patch/ansible-0001-role-get-vars-seen-list.md index 3d5144a59..4b551971a 100644 --- a/defects/ansible/patch/ansible-0001-role-get-vars-seen-list.md +++ b/defects/ansible/patch/ansible-0001-role-get-vars-seen-list.md @@ -1,5 +1,5 @@ # UNDF: UNDF-2026-000000005 -# ansible-0001: Role.get_vars() seen-list O(D²) deduplication +# ansible-0001: Role.get_vars() seen-list O(D^2) deduplication ## Classification - **Severity**: MEDIUM @@ -19,17 +19,42 @@ for dep in self.get_all_dependencies(): ``` ## Pattern -`seen` is initialized as a Python `list`. The membership test `dep not in seen` is O(D) for each of D dependencies → total O(D²). In a large Ansible playbook with deeply nested roles (e.g. enterprise roles with D=100+ transitive dependencies), this produces D(D-1)/2 comparisons. +`seen` initializes as a Python `list`. Membership test `dep not in seen` runs +O(D) for each of D dependencies → total O(D^2). A large Ansible playbook with +deeply nested roles (D=100+ transitive dependencies in enterprise playbooks) +produces D*(D-1)/2 comparisons per `get_vars()` call. ## Speedup -At D=200 dependencies: 19,900 comparisons → 200 comparisons (99.5x reduction) +At D=200 dependencies: 19,900 comparisons collapse to 200 (99.5x reduction). +Measured bench (`bench/bench-ansible-0001.py`, `MockRole` with `__eq__` + +`__hash__`): 9x–270x across D=100..2000. + +## Naive fix fails +Swapping `seen = []` for `seen = set()` alone raises `TypeError: unhashable +type: 'Role'`. `Role` defines `__eq__` at `lib/ansible/playbook/role/__init__.py:202` +(value equality over `_get_hash_dict()`) but no `__hash__`; Python then sets +`__hash__ = None` implicitly, making instances unhashable. + +The fix couples two changes: add a `__hash__` method on `Role` consistent with +the existing `__eq__`, then swap the list dedup for a set. ## Patch ```diff --- a/lib/ansible/playbook/role/__init__.py +++ b/lib/ansible/playbook/role/__init__.py -@@ -536,10 +536,10 @@ class Role(Base, Become, Conditional, Taggable, CollectionSearch): +@@ -202,6 +202,10 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable): + def __eq__(self, other): + if not isinstance(other, Role): + return False + + return self._get_hash_dict() == other._get_hash_dict() + ++ def __hash__(self): ++ # Subset of _get_hash_dict fields; any two roles that compare equal share the same (name, path). ++ return hash((self.get_name(), self.get_role_path())) ++ +@@ -536,10 +540,10 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable): # get exported variables from meta/dependencies - seen = [] + seen = set() @@ -43,8 +68,27 @@ At D=200 dependencies: 19,900 comparisons → 200 comparisons (99.5x reduction) + seen.add(dep) ``` -Note: `Role` objects are used as set members; Python uses identity (`id()`) by default for unhashed objects, which is correct here — same object in memory = same dep. If Role doesn't define `__hash__`, Python uses the default identity hash. +## Hash/eq contract +Python requires `a == b` implies `hash(a) == hash(b)`. `Role.__eq__` compares +`_get_hash_dict()` (name, path, params, when, tags, from_files, vars, +from_include). `__hash__` over `(name, path)` is a stable subset: any two +roles that compare equal share name and path, so they hash equal. Hash +collisions on differing params/when/etc. fall through to `__eq__` and resolve +correctly — allowed under the contract. ## Complexity -- Before: O(D²) — D = number of transitive role dependencies -- After: O(D) — set membership is O(1) amortized +- Before: O(D^2) — D = number of transitive role dependencies +- After: O(D) — set membership O(1) amortized + +## Complexity gate (bench) +`defects/ansible/bench/bench-ansible-0001.py` runs 4 scales (D=100..2000), +min of 3 trials per scale. The bench also asserts that the naive list->set +swap raises `TypeError` on `MockRoleEqOnly` (Role with `__eq__`, no +`__hash__`), proving `__hash__` is a required prerequisite — not a polish. + +Results committed at `defects/ansible/bench/results.txt`. + +## Upstream +- PR branch: `russellballestrini/ansible:fix/role-get-vars-seen-set` +- Unit test: `test/units/playbook/role/test_role.py::TestRole::test_role_is_hashable_and_set_dedupes` +- Integration target: `test/integration/targets/roles_var_inheritance` (exercises shared transitive dep dedup via `common_dep` -> `nested_dep`) diff --git a/defects/ansible/patch/ansible-0001-role-get-vars-seen-list.patch b/defects/ansible/patch/ansible-0001-role-get-vars-seen-list.patch new file mode 100644 index 000000000..7bd0aaca3 --- /dev/null +++ b/defects/ansible/patch/ansible-0001-role-get-vars-seen-list.patch @@ -0,0 +1,113 @@ +# UNDF: UNDF-2026-000000005 +# CWE-407: Algorithmic Complexity, O(D^2) -> O(D) in ansible.playbook.role.Role.get_vars() +# +# Defect: Role.get_vars() dedupes transitive deps with `seen = []` plus +# `dep not in seen`, O(D) per iteration. Total cost: O(D^2) over D transitive +# role dependencies. At D=2000: ~200ms per get_vars() call. +# +# Root cause: a naive `seen = set()` swap raises TypeError — Role defines +# __eq__ without __hash__, so instances are unhashable by default. +# +# Fix: add __hash__ on Role hashing (name, path) (a stable subset of +# _get_hash_dict equality fields), then swap `seen = []` for `seen = set()` +# and `seen.append` for `seen.add`. Total cost after: O(D). +# At D=2000: ~0.75ms per get_vars() call (~270x faster). +# +# Complexity gate (defects/ansible/bench/bench-ansible-0001.py): +# Four scales D=100,500,1000,2000, min of 3 trials per scale. +# Asserts naive list->set swap raises TypeError on MockRoleEqOnly. +# Speedup at D=2000 observed >= ~130x on CPython 3.12. +# +# Upstream tests (ansible.git): +# Unit: test/units/playbook/role/test_role.py::TestRole::test_role_is_hashable_and_set_dedupes +# Integration: test/integration/targets/roles_var_inheritance (shared common_dep -> nested_dep) +# +From 6a5f7d2596d24acaffb480808076fc6990353e02 Mon Sep 17 00:00:00 2001 +From: "russell@unturf.com" +Date: Thu, 23 Apr 2026 12:56:21 -0400 +Subject: [PATCH] playbook/role: dedupe dependency vars via set + +Role.get_vars() previously used a list for `seen` dependency dedup, +making the membership check O(D) per iteration and total O(D^2) over +D transitive dependencies. Switch to a set, reducing to O(D). + +Role defines __eq__ without __hash__ (implicitly unhashable), so add +__hash__ hashing (name, path) -- a stable subset of the equality +fields, preserving the eq/hash contract. +--- + .../fragments/role-get-vars-seen-set.yml | 2 ++ + lib/ansible/playbook/role/__init__.py | 8 ++++++-- + test/units/playbook/role/test_role.py | 19 +++++++++++++++++++ + 3 files changed, 27 insertions(+), 2 deletions(-) + create mode 100644 changelogs/fragments/role-get-vars-seen-set.yml + +diff --git a/changelogs/fragments/role-get-vars-seen-set.yml b/changelogs/fragments/role-get-vars-seen-set.yml +new file mode 100644 +index 0000000..a3b6946 +--- /dev/null ++++ b/changelogs/fragments/role-get-vars-seen-set.yml +@@ -0,0 +1,2 @@ ++minor_changes: ++ - role - ``Role.get_vars()`` now deduplicates transitive dependencies via a set rather than a list, reducing complexity from O(D\ :sup:`2`\ ) to O(D); ``Role`` gains an ``__hash__`` method consistent with its existing ``__eq__``. +diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py +index ab79c55..05a2bb3 100644 +--- a/lib/ansible/playbook/role/__init__.py ++++ b/lib/ansible/playbook/role/__init__.py +@@ -205,6 +205,10 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable): + + return self._get_hash_dict() == other._get_hash_dict() + ++ def __hash__(self): ++ # Subset of _get_hash_dict fields; any two roles that compare equal share the same (name, path). ++ return hash((self.get_name(), self.get_role_path())) ++ + @staticmethod + def load(role_include, play, parent_role=None, from_files=None, from_include=False, validate=True, public=None, static=True, rescuable=True): + if from_files is None: +@@ -536,14 +540,14 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable): + all_vars = self.get_inherited_vars(dep_chain, only_exports=only_exports) + + # get exported variables from meta/dependencies +- seen = [] ++ seen = set() + for dep in self.get_all_dependencies(): + # Avoid rerunning dupe deps since they can have vars from previous invocations and they accumulate in deps + # TODO: re-examine dep loading to see if we are somehow improperly adding the same dep too many times + if dep not in seen: + # only take 'exportable' vars from deps + all_vars = combine_vars(all_vars, dep.get_vars(include_params=False, only_exports=True)) +- seen.append(dep) ++ seen.add(dep) + + # role_vars come from vars/ in a role + all_vars = combine_vars(all_vars, self._role_vars) +diff --git a/test/units/playbook/role/test_role.py b/test/units/playbook/role/test_role.py +index cbfe776..2163849 100644 +--- a/test/units/playbook/role/test_role.py ++++ b/test/units/playbook/role/test_role.py +@@ -410,3 +410,22 @@ class TestRole(unittest.TestCase): + r = Role.load(i, play=mock_play) + + self.assertEqual(r.get_name(), "foo_complex") ++ ++ @patch('ansible.playbook.role.definition.unfrackpath', mock_unfrackpath_noop) ++ def test_role_is_hashable_and_set_dedupes(self): ++ fake_loader = DictDataLoader({ ++ "/etc/ansible/roles/foo_hashable/tasks/main.yml": "- shell: echo hi", ++ }) ++ ++ mock_play = MagicMock() ++ mock_play.role_cache = {} ++ ++ i1 = RoleInclude.load(dict(role='foo_hashable'), play=mock_play, loader=fake_loader) ++ r1 = Role.load(i1, play=mock_play) ++ i2 = RoleInclude.load(dict(role='foo_hashable'), play=mock_play, loader=fake_loader) ++ r2 = Role.load(i2, play=mock_play) ++ ++ hash(r1) ++ self.assertEqual(r1, r2) ++ self.assertEqual(hash(r1), hash(r2)) ++ self.assertEqual(len({r1, r2}), 1) +-- +2.43.0 +