Skip to content

Commit d9eef8a

Browse files
on-keydayclaude
andcommitted
codeql: add maybe-missing.ql to detect MAYBE-wrap gaps
Flags calls to rebrgen's fallible accessor APIs (`ctx.get`, `ctx.get_field`, `ctx.visit`, `ctx.identifier`) whose result has `operator!` semantics (pointer or `expected<>`) but is neither MAYBE-wrapped nor explicitly checked via `if (...)` / `!` / `.has_value()` / `.has_error()`. Heuristic also accepts the "save then check" idiom: auto x = ctx.get(ref); if (x) { ... use *x ... } is treated as checked. Test-scaffolding functions (`test_*`) are excluded; the rebrgen codebase has one such smoke-test that intentionally discards a `visit()` return. Baseline on the ebm2zig DB after filters: total findings: 1 (`includes.hpp:22`, likely an intentional bypass where the callee handles null input gracefully) versus 222 before filters (most being saved-then-checked locals in the generated main.cpp). Each finding is a review candidate, not a definite bug — some bypasses are deliberate. README updated accordingly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 198bb12 commit d9eef8a

2 files changed

Lines changed: 106 additions & 7 deletions

File tree

rebrgen/codeql-queries/README.md

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -91,14 +91,20 @@ for run in d['runs']:
9191

9292
## Empirical baseline (taken from ebm2zig on 2026-05-26)
9393

94-
| Query | Hits | What it means |
95-
| ---------------------------------- | ---- | ------------------------------------------------------ |
96-
| `escaped-ref-capture.ql` | 0 | No truly dangerous escapes in current code. |
97-
| `escaped-ref-capture-review.ql` | 13 | All `[&]ctx` patterns — safe today via dispatch_entry. |
94+
| Query | Hits | What it means |
95+
| ---------------------------------- | ---- | -------------------------------------------------------------------------- |
96+
| `escaped-ref-capture.ql` | 0 | No truly dangerous escapes in current code. |
97+
| `escaped-ref-capture-review.ql` | 13 | All `[&]ctx` patterns — safe today via dispatch_entry. |
98+
| `maybe-missing.ql` | 1 | Review candidate at `includes.hpp:22`; likely an intentional bypass. |
9899

99-
If the strict query starts firing in the future, that's a real bug. If the
100-
review-aid count changes, audit what changed — new patterns may break the
101-
dispatch_entry lifetime invariant.
100+
If the strict capture query starts firing in the future, that's a real bug.
101+
If the review-aid count changes, audit what changed — new patterns may break
102+
the dispatch_entry lifetime invariant.
103+
104+
`maybe-missing.ql` will produce some false positives by design (the codebase
105+
has intentional bypasses where the callee handles null input gracefully).
106+
Treat its output as a review queue: each hit is either a real propagation
107+
gap or an intentional bypass to document/annotate.
102108

103109
## Iterating on a query
104110

@@ -133,6 +139,7 @@ results.
133139
| `codeql-pack.lock.yml` | Pinned dependency versions (generated by `codeql pack install`) |
134140
| `escaped-ref-capture.ql` | Strict: definitely-escaped auto-storage locals (0 hits on ebm2zig) |
135141
| `escaped-ref-capture-review.ql` | Loose: includes reference captures, for human lifetime audit |
142+
| `maybe-missing.ql` | Fallible accessor calls (get / get_field / visit / identifier) that are neither MAYBE-wrapped nor explicitly checked |
136143

137144
## Adding a new query
138145

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
/**
2+
* @name Fallible accessor used outside MAYBE / explicit check
3+
* @description Detects calls to rebrgen accessor APIs (`ctx.get`,
4+
* `ctx.get_field<>`, `ctx.visit`, `ctx.identifier`) whose result
5+
* has `operator!` semantics (pointer or `expected<T>`) but is
6+
* not consumed through the `MAYBE(name, expr)` macro nor through
7+
* an explicit `if (!x)` / `if (x)` / `x.has_value()` style check.
8+
*
9+
* In rebrgen the convention is that every fallible accessor goes
10+
* through MAYBE so nullptr / unexpected propagates as an error
11+
* return instead of crashing. AI-generated code often skips this.
12+
*
13+
* @kind problem
14+
* @problem.severity warning
15+
* @id rebrgen/maybe-missing
16+
* @tags correctness
17+
* rebrgen-custom
18+
*/
19+
20+
import cpp
21+
22+
/** A call to one of the rebrgen accessor APIs whose result is nullable
23+
* (raw pointer) or expected-of-something. */
24+
class FallibleAccessorCall extends FunctionCall {
25+
FallibleAccessorCall() {
26+
this.getTarget().getName() in ["get", "get_field", "visit", "identifier"] and
27+
(
28+
this.getType().getUnspecifiedType() instanceof PointerType
29+
or this.getType().getUnspecifiedType().toString().matches("expected<%>")
30+
)
31+
}
32+
}
33+
34+
/** An invocation of the MAYBE / MAYBE_VOID macro. */
35+
class MaybeMacroInvocation extends MacroInvocation {
36+
MaybeMacroInvocation() { this.getMacroName() = ["MAYBE", "MAYBE_VOID"] }
37+
}
38+
39+
/** True if `e` lives inside the expansion of a MAYBE / MAYBE_VOID macro. */
40+
predicate insideMaybe(Expr e) {
41+
exists(MaybeMacroInvocation m | m.getAnExpandedElement() = e)
42+
}
43+
44+
/** True if `e` (or its surrounding sub-expression chain) is consumed by a
45+
* truthiness check: `!e`, `if (e)`, `while (e)`, `e ? a : b`,
46+
* or `e.has_value()` / `e.has_error()` / `e.value()` / `e.error()`. */
47+
predicate isCheckedExpr(Expr e) {
48+
exists(NotExpr n | n.getOperand() = e.getParent*())
49+
or
50+
exists(IfStmt s | s.getCondition() = e.getParent*())
51+
or
52+
exists(WhileStmt s | s.getCondition() = e.getParent*())
53+
or
54+
exists(ConditionalExpr s | s.getCondition() = e.getParent*())
55+
or
56+
exists(FunctionCall chk |
57+
chk.getQualifier() = e.getParent*() and
58+
chk.getTarget().getName() in ["has_value", "has_error", "error", "value"]
59+
)
60+
}
61+
62+
/** True if the call result reaches an explicit check, either directly or
63+
* via a local variable that captures it (`auto x = call(); if (x) ...`). */
64+
predicate explicitlyChecked(Expr call) {
65+
isCheckedExpr(call)
66+
or
67+
// Local variable initialized from the call, later checked.
68+
exists(LocalVariable v |
69+
v.getInitializer().getExpr().getFullyConverted() = call.getFullyConverted() and
70+
isCheckedExpr(v.getAnAccess())
71+
)
72+
}
73+
74+
/** True if the call is the direct return value (`return call;`) — no
75+
* unwrap needed when propagating verbatim. */
76+
predicate directlyReturned(Expr call) {
77+
exists(ReturnStmt r | r.getExpr() = call.getParent*())
78+
}
79+
80+
from FallibleAccessorCall call
81+
where
82+
not insideMaybe(call) and
83+
not explicitlyChecked(call) and
84+
not directlyReturned(call) and
85+
// skip obvious test scaffolding
86+
not call.getEnclosingFunction().getName().matches("test_%")
87+
select call,
88+
"[REVIEW] Fallible call `" + call.getTarget().getName() +
89+
"(...)` -> `" + call.getType().getUnspecifiedType().toString() +
90+
"`: not MAYBE-wrapped and not explicitly checked. Some bypasses are " +
91+
"intentional (e.g. when the callee handles null input gracefully); " +
92+
"treat as a review candidate, not a definite bug."

0 commit comments

Comments
 (0)