Skip to content

Commit c7a7127

Browse files
committed
fix(ownts): make the new release matchers fail-closed (Codex + CodeRabbit)
The broadened cleanup matchers from this PR could over-suppress (false negatives). Reviewers found three; all are now tied to the specific resource: - AbortController: `{ signal }` is resolved back to its controller (via `const signal = c.signal` / `const { signal } = c`) and release requires `<that controller>.abort()`, not any `.abort()`. So `{signal: c1.signal}` + `c2.abort()` still leaks. - Observer subscribe: a bare `recv.subscribe(args)` is released only by `recv.unsubscribe(args)` on an EXACT receiver match (iterate + compare in Python — no substring `otherro`, no interpolated regex / ReDoS) with the SAME argument list (`disconnect()` with no args tears down everything). So `ro.subscribe(a,h)` + `ro.unsubscribe(b,h)` still leaks. - Conditional cleanup: _cleanup_span now returns a coverage bound; a `return` inside a branch (or a braceless `if (x) return …`) only releases acquires it dominates (co-guarded in the same block). So `setInterval(...)` then `if (enabled) return () => clearInterval(id)` still leaks on the !enabled path. EffectLeakControl.tsx pins all three as OWN001 (a false-negative control beside the EffectRealWorld happy path) + a CI step; the OSS benchmark is unchanged at 11 (the real cases are co-guarded / properly keyed). Pinned the npm versions in the benchmark note's reproduce command. Full suite green; ruff + mypy --strict clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVeuchch67CtjSbizYm8gi
1 parent 694221e commit c7a7127

5 files changed

Lines changed: 149 additions & 26 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,6 +1132,16 @@ jobs:
11321132
# cleanup, observer.subscribe/unsubscribe — all released, zero findings.
11331133
[ "$(echo "$rw" | grep -cE '\[(OWN|EFF|DI)[0-9]{3}\]')" -eq 0 ] \
11341134
|| { echo "FAIL: real-world cleanup patterns must not be reported"; exit 1; }
1135+
- name: False-negative controls (wrong controller / args / conditional cleanup) still leak
1136+
run: |
1137+
python frontend/ownts/ownts.py frontend/ownts/examples/EffectLeakControl.tsx \
1138+
-o "$RUNNER_TEMP/leak.facts.json"
1139+
leak=$(python -m ownlang ownir "$RUNNER_TEMP/leak.facts.json" || true)
1140+
echo "$leak"
1141+
# a release-shaped cleanup that does not release THIS resource must not be
1142+
# over-suppressed — all three controls stay OWN001
1143+
[ "$(echo "$leak" | grep -c 'OWN001')" -eq 3 ] \
1144+
|| { echo "FAIL: broadened matchers must not over-suppress real leaks"; exit 1; }
11351145
- name: The clean fixture (cleanups + useMemo'd dep) is silent
11361146
run: |
11371147
python frontend/ownts/ownts.py frontend/ownts/examples/DashboardClean.tsx \

‎docs/notes/ownts-oss-benchmark.md‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,20 @@ false alarms confined to four named, understood patterns — a real step toward
127127

128128
```bash
129129
# (egress to npm only; github.com is blocked in this environment)
130-
npm pack @mantine/hooks @react-hookz/web @uidotdev/usehooks usehooks-ts
130+
# versions pinned to the exact builds benchmarked above, so reruns are reproducible
131+
npm pack @mantine/hooks@9.4.0 @react-hookz/web@25.2.0 \
132+
@uidotdev/usehooks@2.4.1 usehooks-ts@3.1.1
131133
# extract each tarball, then run ownts.extract + extract_effects + check_facts
132134
# over every *.js/*.mjs (skip *.d.ts); triage each finding against the source.
133135
```
136+
137+
### Soundness of the new matchers
138+
139+
The broadened release matchers stay **fail-closed** — a release-shaped cleanup that
140+
does not actually release *this* resource still reports the leak, pinned by
141+
`EffectLeakControl.tsx`: a listener bound to `c1.signal` torn down by `c2.abort()`,
142+
a `ro.subscribe(a, h)` "released" by `ro.unsubscribe(b, h)`, and a cleanup returned
143+
only under `if (enabled)` over an unconditional `setInterval` — all three remain
144+
`OWN001`. The signal is tied to its controller, the observer to its argument list,
145+
and a conditional cleanup only releases acquires it dominates (co-guarded in the
146+
same block).
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
// False-negative controls for the broadened release matchers (Codex/CodeRabbit):
2+
// each uses a release-shaped cleanup that does NOT actually release THIS resource,
3+
// so it must STILL report a leak. Run:
4+
// python frontend/ownts/ownts.py frontend/ownts/examples/EffectLeakControl.tsx --check
5+
// Expect exactly three OWN001 findings.
6+
import { useEffect } from "react";
7+
8+
export function LeakControl({ enabled }: { enabled: boolean }) {
9+
// (1) wrong controller: the listener is bound to c1's signal, but cleanup aborts
10+
// c2 — the original listener stays live.
11+
useEffect(() => {
12+
const c1 = new AbortController();
13+
const c2 = new AbortController();
14+
window.addEventListener("scroll", onScroll, { signal: c1.signal });
15+
return () => c2.abort();
16+
}, []);
17+
18+
// (2) mismatched unsubscribe args: subscribed with (a, h), torn down with (b, h)
19+
// — the (a, h) subscription leaks.
20+
useEffect(() => {
21+
ro.subscribe(a, h);
22+
return () => ro.unsubscribe(b, h);
23+
}, []);
24+
25+
// (3) conditional cleanup: the timer is created unconditionally, but the cleanup
26+
// is only returned when `enabled` — the !enabled path leaks.
27+
useEffect(() => {
28+
const id = setInterval(tick, 1000);
29+
if (enabled) return () => clearInterval(id);
30+
}, [enabled]);
31+
32+
return <div>leak control</div>;
33+
}

‎frontend/ownts/ownts.py‎

Lines changed: 85 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,36 @@ def _receiver(text: str, end: int) -> str:
121121
return m.group(1) if m else ""
122122

123123

124+
def _call_args(s: str) -> str | None:
125+
"""The normalized (whitespace-stripped) argument text of the first call in `s`,
126+
e.g. `target,handler` for `.subscribe(target, handler)`. None if no `(`."""
127+
i = s.find("(")
128+
if i == -1:
129+
return None
130+
close = _match_pair(s, i, "(", ")")
131+
return re.sub(r"\s+", "", s[i + 1:close - 1])
132+
133+
134+
def _signal_controller(setup: str, opts: str) -> str | None:
135+
"""The AbortController whose `signal` an addEventListener options arg uses, so a
136+
cleanup is only credited when THAT controller is aborted. `{ signal: c.signal }`
137+
→ `c`; the shorthand `{ signal }` is resolved back through `const signal =
138+
c.signal` / `const { signal } = c` in setup. None when it can't be tied."""
139+
m = re.search(r"\bsignal\s*:\s*([A-Za-z_$][\w$.]*)", opts)
140+
if m:
141+
return re.sub(r"\.signal$", "", m.group(1))
142+
if re.search(r"\bsignal\b", opts): # shorthand `{ signal }`
143+
m = re.search(r"(?:const|let|var)\s+signal\s*=\s*([A-Za-z_$][\w$]*)\s*\.\s*signal\b",
144+
setup)
145+
if m:
146+
return m.group(1)
147+
m = re.search(r"(?:const|let|var)\s*\{\s*signal\b[^}]*\}\s*=\s*([A-Za-z_$][\w$]*)",
148+
setup)
149+
if m:
150+
return m.group(1)
151+
return None
152+
153+
124154
def _is_released(acq: Acquire, setup: str, pos: int, cleanup: str) -> bool:
125155
"""Whether THIS acquire (at `pos` in `setup`) is released by the effect's cleanup
126156
— matched to its own handle, so two `setInterval`s with one `clearInterval` leave
@@ -135,21 +165,33 @@ def _is_released(acq: Acquire, setup: str, pos: int, cleanup: str) -> bool:
135165
if tok and re.search(
136166
rf"\b{re.escape(tok)}\s*\.\s*(?:unsubscribe|remove)\s*\(", cleanup):
137167
return True
138-
# a bare `recv.subscribe(...)` (no token) is released by `recv.unsubscribe(...)`
139-
# / `recv.disconnect()` on the SAME receiver in cleanup — the observer pattern.
168+
# a bare `recv.subscribe(args)` (no token) is released only by a cleanup
169+
# `recv.unsubscribe(args)` on the SAME receiver AND the same argument list
170+
# (a `disconnect()` with no args tears down everything). Iterating + comparing
171+
# in Python gives an exact receiver match (no `otherro` substring) and no
172+
# interpolated regex.
140173
recv = _receiver(setup, pos)
141-
return bool(recv and re.search(
142-
rf"{re.escape(recv)}\s*\.\s*(?:unsubscribe|disconnect)\s*\(", cleanup))
174+
if not recv:
175+
return False
176+
sub_args = _call_args(setup[pos:])
177+
for um in re.finditer(
178+
r"([A-Za-z_$][\w$.\[\]]*)\s*\.\s*(?:unsubscribe|disconnect)\s*\(", cleanup):
179+
if um.group(1) == recv and _call_args(cleanup[um.end() - 1:]) in ("", sub_args):
180+
return True
181+
return False
143182
if acq.resource == "subscription": # target.addEventListener(event, handler[, opts])
144183
a = _listener_call(setup[pos:])
145184
if a is None:
146185
return False
147-
# AbortController: a listener bound to an AbortSignal is released when the
148-
# effect aborts the controller — `addEventListener(.., {signal})` torn down by
149-
# `controller.abort()` removes every signal-bound listener at once.
186+
# AbortController: a signal-bound listener is released only when THE controller
187+
# backing that signal is aborted in cleanup — tie `{ signal }` back to its
188+
# controller and require `<that controller>.abort()`, not any `.abort()`.
150189
am = _LISTENER.match(setup[pos:].lstrip())
151190
opts = (am.group(3) or "") if am else ""
152-
if re.search(r"\bsignal\b", opts) and re.search(r"\.abort\s*\(", cleanup):
191+
ctrl = _signal_controller(setup, opts)
192+
if ctrl and any(
193+
ab.group(1) == ctrl for ab in
194+
re.finditer(r"([A-Za-z_$][\w$.\[\]]*)\s*\.\s*abort\s*\(", cleanup)):
153195
return True
154196
# Otherwise the full listener key is (receiver, event, handler, capture): a
155197
# listener is released ONLY by a removeEventListener whose whole key matches —
@@ -399,15 +441,20 @@ def _effect_callback(masked: str, after_open: int) -> tuple[str, int, list[str]
399441
return body, i, deps, end
400442

401443

402-
def _cleanup_span(mbody: str) -> tuple[int, int] | None:
403-
"""The (start, end) span of the effect's OWN cleanup within `mbody` (the masked
404-
body), or None. The cleanup is the `return () => …` directly in the effect
405-
callback — possibly inside an `if`/`try`/`.then()` BLOCK, but NOT inside a nested
406-
CALLBACK (an event handler defined in the effect). So the test is FUNCTION depth,
407-
not brace depth: a block `{` keeps us in the effect; an arrow/`function` body `{`
408-
is a new function. A return at function-depth 1 (only the effect body encloses) is
409-
the cleanup; deeper is some inner callback's return. The span is returned (not
410-
sliced) so the caller can cut it from BOTH the masked and the original body."""
444+
def _cleanup_span(mbody: str) -> tuple[int, int, int] | None:
445+
"""`(start, end, cov_start)` for the effect's OWN cleanup within `mbody` (masked),
446+
or None. The cleanup is the `return () => …` directly in the effect callback —
447+
possibly inside an `if`/`try`/`.then()` BLOCK, but NOT inside a nested CALLBACK. So
448+
the test is FUNCTION depth, not brace depth: a block `{` keeps us in the effect; an
449+
arrow/`function` body `{` is a new function. A return at function-depth 1 is the
450+
cleanup; deeper is some inner callback's return.
451+
452+
`cov_start` bounds which acquires the cleanup may credit: only acquires at body
453+
position `cov_start < p < start` are co-guarded with the cleanup. For a top-level
454+
cleanup that is the whole effect body (`cov_start == 0`); for a cleanup inside a
455+
block it is that block's `{` (so an acquire OUTSIDE the branch is NOT released by a
456+
conditional cleanup); for a *braceless* guard (`if (x) return …`) it is `start`
457+
(empty), so a conditionally-returned cleanup releases nothing it does not dominate."""
411458
# function-body braces: the effect body's own `{` (index 0), plus every `=> {`
412459
# and `function (...) {`. Every other `{` is a plain block (if/try/for/object).
413460
fn_braces = {0}
@@ -416,30 +463,35 @@ def _cleanup_span(mbody: str) -> tuple[int, int] | None:
416463
for fm in re.finditer(r"\bfunction\b[^{;]*?\)\s*\{", mbody):
417464
fn_braces.add(fm.end() - 1)
418465
ret_re = re.compile(r"return\s*(?:\(\s*\)|\w+)\s*=>")
419-
stack: list[bool] = [] # True where the open brace started a function body
466+
stack: list[tuple[bool, int]] = [] # (is_function_body, open_index) per open brace
420467
fdepth = 0
421468
i, n = 0, len(mbody)
422469
while i < n:
423470
c = mbody[i]
424471
if c == "{":
425472
is_fn = i in fn_braces
426-
stack.append(is_fn)
473+
stack.append((is_fn, i))
427474
fdepth += 1 if is_fn else 0
428475
i += 1
429476
continue
430477
if c == "}":
431-
if stack and stack.pop():
478+
if stack and stack.pop()[0]:
432479
fdepth -= 1
433480
i += 1
434481
continue
435482
if c == "r" and fdepth == 1:
436483
m = ret_re.match(mbody, i)
437484
if m:
485+
cov_start = stack[-1][1] if stack else 0 # innermost enclosing block
486+
b = max(mbody.rfind(";", 0, i), mbody.rfind("{", 0, i),
487+
mbody.rfind("}", 0, i))
488+
if re.search(r"\b(?:if|else|for|while)\b", mbody[b + 1:i]):
489+
cov_start = m.start() # braceless conditional guard -> covers nothing
438490
rest = mbody[m.end():]
439491
stripped = rest.lstrip()
440492
if stripped.startswith("{"): # block-bodied cleanup: `=> { ... }`
441493
brace = m.end() + (len(rest) - len(stripped))
442-
return (m.start(), _match_block(mbody, brace))
494+
return (m.start(), _match_block(mbody, brace), cov_start)
443495
# single-expression cleanup — consume the WHOLE expression across line
444496
# breaks, stopping at a top-level `;` or the effect body's own close.
445497
j, depth = m.end(), 0
@@ -454,7 +506,7 @@ def _cleanup_span(mbody: str) -> tuple[int, int] | None:
454506
elif cj == ";" and depth == 0:
455507
break
456508
j += 1
457-
return (m.start(), j)
509+
return (m.start(), j, cov_start)
458510
i += 1
459511
return None
460512

@@ -468,11 +520,12 @@ def extract(path: str) -> list[Component]:
468520
body_o = text[body_start:end] # original (unmasked) body — same positions
469521
span = _cleanup_span(body_m)
470522
if span:
471-
cs, ce = span
523+
cs, ce, cov_start = span
472524
setup_m = body_m[:cs] + body_m[ce:]
473525
setup_o = body_o[:cs] + body_o[ce:]
474526
cleanup_o = body_o[cs:ce]
475527
else:
528+
cs = ce = cov_start = -1
476529
setup_m, setup_o, cleanup_o = body_m, body_o, ""
477530
cname = _component_at(masked, eff.start())
478531
comp = comps.setdefault(cname, Component(cname, path))
@@ -481,9 +534,16 @@ def extract(path: str) -> list[Component]:
481534
# decide release on the ORIGINAL setup/cleanup (so event-name strings and
482535
# handles are intact for the full-key comparison).
483536
for hit in acq.pattern.finditer(setup_m):
484-
abs_pos = body_start + hit.start()
537+
# map the setup position back to the body (the cleanup span was cut out)
538+
sp = hit.start()
539+
bp = sp if (cs < 0 or sp < cs) else sp + (ce - cs)
540+
abs_pos = body_start + bp
485541
line = masked.count("\n", 0, abs_pos) + 1
486-
released = _is_released(acq, setup_o, hit.start(), cleanup_o)
542+
# a conditional cleanup only releases acquires it dominates (co-guarded
543+
# in the same block); an acquire outside that block keeps its own cleanup
544+
# empty, so a `return` guarded by a branch cannot silence it.
545+
covered = cs < 0 or cov_start < bp < cs
546+
released = _is_released(acq, setup_o, sp, cleanup_o if covered else "")
487547
# the acquire expression for the tag — from the ORIGINAL text, so the
488548
# message shows the real call (string args intact), trimmed to one line.
489549
snippet = text[abs_pos:].splitlines()[0].strip().rstrip("{").strip()

‎frontend/ownts/test_ownts.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,13 @@ def main() -> int:
6363
real = codes("EffectRealWorld.tsx")
6464
assert real == [], f"EffectRealWorld should be silent -> {real}"
6565

66+
# False-negative controls: a release-shaped cleanup that does NOT release THIS
67+
# resource (wrong AbortController, mismatched unsubscribe args, a conditionally
68+
# returned cleanup over an unconditional acquire) must STILL report the leak —
69+
# the broadened matchers must not over-suppress.
70+
leaks = codes("EffectLeakControl.tsx")
71+
assert leaks == ["OWN001", "OWN001", "OWN001"], f"EffectLeakControl -> {leaks}"
72+
6673
# an expression-bodied cleanup whose removeEventListener carries an options
6774
# object must parse (the `{` belongs to the call, not the cleanup block) — the
6875
# listener is released, so no false-positive leak.

0 commit comments

Comments
 (0)