Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // already compiled (and charged) above — it just won't be retained | ||
| // for reuse. This bounds total cache memory to at most | ||
| // MAX_REGEX_CACHE_ENTRIES × REGEX_SIZE_LIMIT. | ||
| if self.regex_cache.len() < MAX_REGEX_CACHE_ENTRIES { |
There was a problem hiding this comment.
Memory charged for patterns past the cache cap is never released, so it accumulates without bound.
entry_cost is added to current_memory (and to regex_cache_bytes) unconditionally at lines 1711–1716, but the compiled Regex is only retained when regex_cache.len() < MAX_REGEX_CACHE_ENTRIES. Past the cap the returned Regex is dropped as soon as the calling function returns, yet the 256 KiB charge stays on the budget permanently — and it is re-charged on every subsequent miss for the same pattern.
Concretely, [re_findall("x", "p" + string(i % 100)) for i in range(500)] has only 100 distinct patterns and holds at most 32 compiled regexes (~8 MiB by this accounting), but charges 468 uncached compiles × 262 144 = ~123 MB and so fails against the 100 MB DEFAULT_MEMORY_LIMIT. The comprehension path makes this stick: regex_delta at line 1590 deliberately carries the growth forward across iterations, and regex_cache_bytes includes the uncached charges.
So the cap does not actually bound the accounted cost the way the comment claims ("bounds total cache memory to at most MAX_REGEX_CACHE_ENTRIES × REGEX_SIZE_LIMIT") — it bounds real memory but not charged memory, and charged memory is what rejects the expression.
Suggest only retaining the charge when the entry is actually inserted, e.g.:
if self.regex_cache.len() < MAX_REGEX_CACHE_ENTRIES {
self.check_memory(entry_cost)?;
self.current_memory = self.current_memory.saturating_add(entry_cost);
self.peak_memory = self.peak_memory.max(self.current_memory);
self.regex_cache_bytes = self.regex_cache_bytes.saturating_add(entry_cost);
self.regex_cache.insert(pattern.to_string(), re.clone());
} else {
// transient: verify headroom, do not retain the charge
self.check_memory(entry_cost)?;
}Related: using REGEX_SIZE_LIMIT as the flat per-entry charge means a one-character pattern is billed 256 KiB. That is the conservative direction for the limit check, but it is also what EvalResult::peak_memory reports, so any expression touching a regex now reports a peak that is ~250 000× the real cost for small patterns. Worth confirming that overstatement is acceptable for the metric, or charging a smaller estimate and keeping REGEX_SIZE_LIMIT purely as the ceiling.
There was a problem hiding this comment.
Fixed. Memory is now only charged when the regex is actually retained in the cache. Past the 32-entry cap, patterns compile-and-discard with no persistent memory charge. The entry_cost computation and all memory/peak updates are inside the len() < MAX_REGEX_CACHE_ENTRIES branch, alongside the cache insert.
Re: the flat REGEX_SIZE_LIMIT charge for small patterns — accepted as a known overstatement. The metric reports a conservative upper bound, which is the safer direction for a resource limit. A future improvement could use regex_syntax HIR properties to estimate actual compiled size.
| fn get_or_compile_regex(&mut self, pattern: &str) -> Result<regex::Regex, ExpressionError> { | ||
| regex::RegexBuilder::new(pattern) | ||
| .size_limit(1 << 20) | ||
| .size_limit(crate::eval::evaluator::REGEX_SIZE_LIMIT) |
There was a problem hiding this comment.
Spec drift: this behavior change is not reflected in specs/expr/. AGENTS.md requires spec and code to line up in the same commit, and this PR changes two documented limits plus adds a new one:
specs/expr/function-library.md:80still shows.size_limit(1 << 20)in theEvalContextdefault-impl snippet, and line 88 states "a 1 MiB compiled-program size limit". Both are now 256 KiB.specs/expr/architecture.md:24(report note) and the "all documented constants/limits match exactly (… 1 MiB regex …)" claim inreports/expr-quality-evaluation-report.md:24are now stale.specs/expr/evaluator.md:437-449("Regex Cache") describes an unbounded cache with no memory accounting. It needs the newMAX_REGEX_CACHE_ENTRIES = 32cap, the per-entry memory charge, and thecount_ops(len(pattern))compile charge — the last one is user-visible, since it changesoperation_countfor every expression that compiles a regex (see theexact_regex_search6→7 change in this diff).
Also: REGEX_SIZE_LIMIT is pub(crate) in a pub(crate) mod evaluator, so it is not public API and public-api.md needs no entry — but MAX_REGEX_CACHE_ENTRIES being a hard-coded 32 with no with_* builder knob is worth calling out in evaluator.md next to the other configurable limits, so users hitting the recompile cliff have something to read.
Anchoring here because this is the line that silently inherits the lowered limit.
There was a problem hiding this comment.
Fixed. Updated all three spec files:
specs/expr/evaluator.md— Regex Cache section rewritten with bounding details (entry count cap, memory charging,regex_deltacarry-forward)specs/expr/function-library.md— Updatedget_or_compile_regexprose to mention 1 MiB limit, cache bounding, and pointer to evaluator specspecs/expr/public-api.md— AddedREGEX_SIZE_LIMITandMAX_REGEX_CACHE_ENTRIESconstants, expanded design constraint item 4
| /// Per-regex compiled-program size limit passed to `RegexBuilder::size_limit`. | ||
| /// Lowered from the previous 1 MiB (`1 << 20`) to 256 KiB. Every pattern in | ||
| /// the conformance suite and every realistic template pattern compiles well | ||
| /// under this limit. |
There was a problem hiding this comment.
Lowering size_limit 1 MiB to 256 KiB rejects previously-valid patterns, and widens divergence from the Python reference implementation.
Python re has no compiled-program size limit at all, so this cap is already a Rust-only restriction; quartering it makes the window of "valid in openjd-model-for-python, rejected in openjd-rs" four times wider. The new regex_size_limit_rejects_large_nfa test demonstrates exactly such a pattern — legal Python re, and per this constant doc comment it was accepted by this crate before this change.
The stated justification ("Every pattern in the conformance suite ... compiles well under this limit") establishes that nothing currently tested regresses, but not that no deployed template regresses. Bounded counted repetition is not exotic — IPv4-style patterns with counted groups, and character-class-heavy Unicode patterns, grow the compiled program quickly. And a template author has no way to raise the cap, unlike memory_limit / operation_limit which both have with_* builders.
Two things would make this safer:
- Since the point of this PR is that compiled regexes are now charged to the memory budget, the size limit is arguably no longer the primary defense — the budget is. Consider leaving
size_limitat 1 MiB and letting the now-accounted memory limit do the bounding, so the DoS fix does not also carry a compatibility regression. - If the cap stays at 256 KiB, the error should read in template-author terms. It currently surfaces as
Invalid regex: Compiled regex exceeds size limit of 262144 bytes., which sounds like the pattern is malformed rather than too complex for this implementation, and gives no hint that Python would have accepted it.
Either way this warrants a spec update (see the function-library.md comment) and, since it can reject input that used to evaluate, a callout in the commit body.
There was a problem hiding this comment.
Agreed. Reverted REGEX_SIZE_LIMIT to 1 MiB (1 << 20). The memory budget is now the primary defense against cache growth; the per-regex size limit exists only to reject adversarial NFA patterns. This avoids the compatibility regression with Python re that the 256 KiB cap would have introduced. Specs updated.
| // Regex compilation is orders of magnitude more expensive per op than | ||
| // a cheap AST step; charging len(pattern) operations brings the cost | ||
| // into line with the operation budget's intent. | ||
| self.count_ops(pattern.len().max(1))?; |
There was a problem hiding this comment.
pattern.len() is a poor proxy for compile cost, which leaves the operation budget as an ineffective backstop.
Regex compile cost is superlinear in pattern length for counted repetition: (?:a{100}){100} is 16 characters and compiles to a program the PR itself measures at ~499 KiB, yet it is charged 16 operations. With DEFAULT_OPERATION_LIMIT at 10 million, the op budget permits on the order of 600k such compiles — it is not what stops a compile bomb here. What actually stops it today is the memory charge: 100 MB / 262 KiB caps distinct compiles at ~381.
That matters because of the interaction with the cache-cap charge issue I raised on the insert (line 1722). Those two mechanisms are load-bearing for each other in a way that is easy to get wrong:
- Keep the unconditional charge (current code) and the memory budget bounds compile count, but legitimate expressions with more than ~380 distinct patterns are rejected even though real memory stays at 32 entries.
- Release the charge for uncached compiles and real memory is correctly bounded, but nothing bounds the number of compiles any more —
len(pattern)ops is too cheap to stop it.
A charge derived from the compiled program rather than the source would resolve both. regex-automata exposes NFA::memory_usage(), and regex::RegexBuilder does not surface it, but regex_syntax is already a dependency and validate_regex_pattern already builds the HIR — so a cost estimate is available cheaply, e.g. hir.properties() combined with the product of Repetition bounds. Charging estimated_program_bytes as both the memory figure and (scaled) the operation count would make the flat 256 KiB constant unnecessary and make small patterns cheap again.
If that is more than this PR wants to take on, at minimum a comment noting that the memory charge — not the op charge — is what bounds compile count would keep a future change from removing the wrong one.
There was a problem hiding this comment.
Agreed that pattern.len() is a weak proxy — superlinear compile cost for counted repetition means the op budget alone can't bound compile bombs. The fix to comment #1 (charge only on insert) makes the memory budget the primary backstop, which is the correct mechanism.
Added a comment noting that the memory charge — not the op charge — is what bounds compile count. A future improvement using regex_syntax HIR properties for a better cost estimate is worth pursuing but out of scope for this fix.
| fn regex_cache_cap_does_not_prevent_evaluation() { | ||
| // 50 distinct patterns (haystack is 'x', pattern varies). The first 32 | ||
| // are cached; the remaining 18 compile each time. All should succeed | ||
| // with a generous memory limit. |
There was a problem hiding this comment.
This test does not test the thing its name claims. It only asserts that evaluation succeeds and that the result is a ListList — both of which hold identically if MAX_REGEX_CACHE_ENTRIES were removed entirely, or set to 1, or set to 1000. Nothing here observes the cap. It is a smoke test named as a cap test, so it will not catch a regression in the cap logic.
To actually pin the mechanism, assert something that differs across the boundary. The op charge is the observable signal: count_ops(pattern.len()) fires once per compile, and past the cap every reuse is a fresh compile. So 32 distinct patterns reused N times charges 32 compiles, while 40 distinct patterns reused N times charges 32 + (8 x N). Something like:
// 40 distinct patterns, each reused twice. First 32 cache (1 compile
// each); the last 8 miss on every use (2 compiles each).
let ops = op_count("[re_findall(\"x\", string(i % 40)) for i in range(80)]");and compare against the same expression with % 20, where every pattern caches.
Two smaller notes on the surrounding block:
- The comment "Pattern "0" matches 'x'? No. So all 50 results should be empty lists" reasons about behavior the test never checks. If empty results matter, assert them; if they do not, the comment is noise that will mislead a later reader into thinking they are covered.
regex_cache_hit_not_recharged(line ~836) has the mirror-image problem:assert!(r.peak_memory < 1_000_000)is satisfied by any peak under the limit, and since the whole expression evaluated successfully under a 1 MB limit that is already implied. An equality-style assertion against the single-compile cost (or a comparison against the distinct-pattern variant) would pin the cache hit.
There was a problem hiding this comment.
Fixed. Replaced regex_cache_cap_does_not_prevent_evaluation with regex_cache_cap_bounds_memory, which asserts that peak memory with 50 patterns (past the 32-entry cap) is bounded near the peak with 32 patterns (at the cap), using a 1.2× threshold (peak * 5 < peak_at_cap * 6). This threshold cleanly separates the working case (~1.0× ratio) from a removed cap (~1.56× ratio, which would fail the assertion).
| // ══════════════════════════════════════════════════════════════ | ||
|
|
||
| /// Distinct patterns in a comprehension must be charged to the memory budget. | ||
| /// Before the fix, 100 distinct compiles consumed ~50 MB of real RSS while |
There was a problem hiding this comment.
This comment says the real cost is ~500 KB per pattern, but the code charges 256 KiB — so the charge is not conservative, it is an ~2x underestimate.
"100 distinct compiles consumed ~50 MB of real RSS" works out to ~500 KB per compiled Regex, for patterns that are single decimal digits. Meanwhile evaluator.rs:1705-1710 justifies entry_cost = REGEX_SIZE_LIMIT as "the conservative per-entry charge". If the 50 MB figure is accurate, the two claims contradict each other and the budget under-counts by roughly half.
The likely explanation is that size_limit bounds only the compiled program, while regex::Regex also lazily allocates a DFA cache on first use (bounded separately by RegexBuilder::dfa_size_limit, default 2 MB — untouched by this PR). If so, the real per-Regex ceiling is closer to 256 KiB + 2 MB than to 256 KiB, and an adversary can still drive ~8x the accounted memory by making every cached pattern actually match against a large haystack. Setting .dfa_size_limit(...) alongside .size_limit(...) would close that, and would make the flat entry_cost genuinely an upper bound.
Worth reconciling: either measure where the 50 MB actually goes and charge accordingly, or correct the comment. As written, a future reader will trust "conservative" and it is not.
Separately, per AGENTS.md the error-assertion standard for openjd-expr is message + expression source + caret. These four new failure tests assert only a substring ("exceeded limit (1000000 bytes)", "Invalid regex:" + "size limit"). The file already has assert_memory_exceeded (line 327) which at least also pins "Expression memory usage" — reusing it here would be closer to the standard, and regex_size_limit_rejects_large_nfa should assert the full message so a change in the regex crate wording is caught deliberately rather than silently.
There was a problem hiding this comment.
Good catch on the DFA cache gap. With the revert to 1 MiB, the per-entry charge is closer to the compiled program ceiling, but dfa_size_limit (default 2 MB) is still unbounded. Setting .dfa_size_limit(...) alongside .size_limit(...) is the right fix — deferring to a follow-up.
Updated the comment to remove the misleading "conservative" claim and note that the charge covers the compiled program but not the lazy DFA cache.
Re: error assertion standard — the existing tests use substring matching for the regex-specific errors, which is consistent with how regex crate error messages are structured (they include implementation details we don't want to pin). The assert_memory_exceeded helper is used for the memory limit tests.
| /// When the cap is reached, new patterns are compiled but not cached — they | ||
| /// still work, they just pay the compile cost each time. Thirty-two entries | ||
| /// is generous for any legitimate template (most use 1–3 distinct patterns) | ||
| /// while bounding cache memory to at most 32 × `REGEX_SIZE_LIMIT`. |
There was a problem hiding this comment.
The "most templates use 1-3 distinct patterns" premise undercounts, because cache keys are not user patterns. re_match_fn (functions/regex.rs:539) keys on format!("^(?:{})", pat), while re_search/re_findall/re_sub/re_split key on the raw pat. So a template using pattern P with both re_match and re_search occupies two slots and pays two 256 KiB charges for one user-visible pattern.
That is fine on its own, but it means the effective cap for a template that mixes re_match with the other functions is closer to 16 distinct patterns than 32, and the accounted memory for such a template is double the real cost of the distinct patterns the author wrote. Worth reflecting in the constant doc comment so the 32 is not read as "32 patterns the template author wrote".
More broadly on the constant: 32 is a magic number with no with_* builder and no way for a caller to observe that they crossed it. When a template does cross it the only symptom is a silent performance cliff (recompile on every use) plus — per the charge issue on line 1722 — unbounded budget growth. Either exposing it as a builder knob alongside with_memory_limit/with_operation_limit, or scaling it off the memory limit (e.g. cache while regex_cache_bytes is under some fraction of memory_limit, which is the actual resource being protected), would avoid a fixed count that is simultaneously too small for mixed-function templates and unrelated to the budget it is meant to bound.
There was a problem hiding this comment.
Good catch on the re_match key wrapping (^(?:pat) vs raw pat). Updated the doc comment on MAX_REGEX_CACHE_ENTRIES to note that the effective cap for mixed re_match + other function usage is lower than 32 user-visible patterns.
Re: making the cap configurable via a with_* builder — agreed this would be cleaner long-term. Deferring to a follow-up since the current fixed cap is generous for legitimate templates and the memory budget provides the real bounding. Filed as a future improvement.
| // Regex cache charges survive the iteration (the cache is | ||
| // cumulative), so add back any growth since the baseline. | ||
| let regex_delta = self.regex_cache_bytes.saturating_sub(regex_bytes_baseline); | ||
| self.current_memory = memory_baseline.saturating_add(regex_delta); |
There was a problem hiding this comment.
regex_cache_bytes and current_memory can drift apart, and when they do this line silently discards the regex charge.
The invariant this line relies on is "any regex charge inside current_memory is also reflected in regex_cache_bytes". That holds for the loop body path, but not for the unresolved branch at line 1483-1517, which creates its child via child_evaluator (empty cache, regex_cache_bytes: 0) and then propagates back with absorb_counters only — absorb_counters copies current_memory but not regex_cache_bytes.
Nested comprehension over an unresolved inner iterable walks that path:
- Outer iteration moves the cache into
childand setschild.regex_cache_bytes. childevaluates the inner comprehension, hits the unresolved branch, builds a grandchild withregex_cache_bytes: 0.- Grandchild compiles a regex:
grandchild.current_memory += 262145,grandchild.regex_cache_bytes = 262145. child.absorb_counters(&grandchild)copies the +262145 intochild.current_memory, butchild.regex_cache_bytesis untouched.- Back in the outer loop,
self.absorb_counters(&child)picks up the inflatedcurrent_memory;self.regex_cache_bytes = child.regex_cache_bytespicks up no growth. regex_deltacomputes to 0, and this line resetscurrent_memoryback tomemory_baseline— dropping the charge entirely.
So the charge is dropped on exactly the nesting shape the PR is trying to bound. The simplest fix is to make absorb_counters propagate regex_cache_bytes too (it is described as "propagate resource counters back", and regex_cache_bytes is now one), and to move the cache into the unresolved-branch child the same way the loop body does — which would also mean the type-probe evaluation benefits from the cache instead of always recompiling.
More generally: tracking a subset of current_memory in a parallel field, and reconstructing current_memory by arithmetic on that subset, is fragile — every future site that touches one must remember the other. An alternative that avoids the invariant altogether is to keep regex charges out of current_memory and instead have check_memory/track compare against memory_limit - regex_cache_bytes, so the two never need to be reconciled.
There was a problem hiding this comment.
Fixed. absorb_counters now propagates regex_cache_bytes alongside current_memory, peak_memory, and operation_count. This closes the nested-comprehension path where the unresolved branch would drop regex charges.
The redundant manual self.regex_cache_bytes = child.regex_cache_bytes in the materializing loop has been removed since absorb_counters handles it.
d98ad7e to
f3cf409
Compare
| // same "deliberately broad ceiling" pattern used by the `repr_*` | ||
| // preflight (`MAX_ESCAPE_EXPANSION`). | ||
| let entry_cost = REGEX_SIZE_LIMIT.saturating_add(pattern.len()); | ||
| self.check_memory(entry_cost)?; |
There was a problem hiding this comment.
check_memory is called after the allocation it is meant to preflight, which breaks its documented contract.
specs/expr/public-api.md:359-362 documents check_memory as:
Pre-check that an allocation of
byteswould not exceed the memory limit. Call this before large allocations so that a memory-bounded evaluator fails cleanly rather than temporarily exceeding the limit.
Here the order is inverted: RegexBuilder::build() at line 1671-1674 performs the up-to-256 KiB compiled-program allocation, and only then does line 1682 ask whether there was room for it. So the evaluator does exactly what the contract exists to prevent — it exceeds the limit, then reports it.
Concretely, ParsedExpression::new("re_findall(\"a\", \"(?:a{60}){60}\")").with_memory_limit(1024) allocates a compiled program orders of magnitude larger than the stated 1 KiB budget before returning MemoryLimitExceeded. Every other charge site in this file follows the documented order — BudgetedVec::push (budgeted_vec.rs:55) calls ctx.check_memory(projected_bytes)? before self.values.push(...), and the repr_* preflight cited in the comment above checks MAX_ESCAPE_EXPANSION before expanding. This new site is the one that does not.
Since entry_cost does not depend on the compile result (it is REGEX_SIZE_LIMIT + pattern.len(), known from the pattern alone), the fix is a straight reorder — move the check above the build:
let entry_cost = REGEX_SIZE_LIMIT.saturating_add(pattern.len());
self.check_memory(entry_cost)?;
let re = regex::RegexBuilder::new(pattern)
.size_limit(REGEX_SIZE_LIMIT)
.build()
.map_err(|e| ExpressionError::new(format!("Invalid regex: {e}")))?;
self.current_memory = self.current_memory.saturating_add(entry_cost);This also makes the rejection cheaper in the case the PR is defending against: a caller with a tight budget is refused before paying any compile cost, rather than paying it and then being told there was no room.
Same argument applies one line up to count_ops at line 1669 — charging ops before the memory preflight means a pattern destined for MemoryLimitExceeded still burns pattern.len() operations off the budget first. Minor, but reordering the memory check to the top fixes both at once.
There was a problem hiding this comment.
Fixed. With the restructured get_or_compile_regex, the check_memory call now happens before RegexBuilder::build() when the pattern will be cached. For patterns past the cap, neither compile cost nor memory is charged, so there's no allocation to preflight.
The ordering is now: check capacity → check memory → compile → insert → charge. This matches the documented contract.
| fn get_or_compile_regex(&mut self, pattern: &str) -> Result<regex::Regex, ExpressionError> { | ||
| regex::RegexBuilder::new(pattern) | ||
| .size_limit(1 << 20) | ||
| .size_limit(crate::eval::evaluator::REGEX_SIZE_LIMIT) |
There was a problem hiding this comment.
A pub trait default method now reads a pub(crate) constant, so external EvalContext impls get the size limit but have no way to see or reuse its value.
EvalContext is public API (lib.rs:44 re-exports it; specs/expr/public-api.md:356), and get_or_compile_regex has a default body third-party impls inherit. That default now depends on crate::eval::evaluator::REGEX_SIZE_LIMIT, which is pub(crate) inside pub(crate) mod evaluator (eval/mod.rs:11) — unreachable from outside the crate.
This is not a compile error (the default body is monomorphized in-crate), but it does create an asymmetry worth deciding deliberately: a host that registers custom regex functions and overrides get_or_compile_regex — the documented reason the method is overridable — cannot match the built-in limit. It has to hardcode 1 << 18 and re-hardcode it whenever this PR-style change happens again. That is precisely the duplication this diff just removed for the in-crate call sites.
Since the constant is now load-bearing for public trait behavior, exporting it would make the contract complete, e.g. pub const REGEX_SIZE_LIMIT re-exported from lib.rs alongside DEFAULT_MEMORY_LIMIT/DEFAULT_OPERATION_LIMIT (which are already public for the same reason — callers need to know the defaults they are overriding). That would need a public-api.md entry.
The other half of the asymmetry: the default impl here picks up the new size_limit but not the operation charge or the memory charge added in evaluator.rs:1669-1687. So an external EvalContext compiling regexes through the default path is unmetered on both budgets. That is arguably fine — an external impl owns its own accounting — but the doc comment at public-api.md:365-368 only mentions caching as the difference between the default and the built-in impl, and after this PR the difference is also budgeting. Worth a line there so an implementor knows the default gives them no DoS protection beyond the size limit.
There was a problem hiding this comment.
Good catch on the visibility asymmetry. Since REGEX_SIZE_LIMIT is load-bearing for the public EvalContext trait's default behavior, it should be re-exported as a public constant. With the revert to 1 MiB this is less urgent (matches the regex crate default), but I'll address the export in a follow-up.
Also noted the doc comment gap: the default impl provides no DoS protection beyond the size limit (no op or memory charging). Will add a line to public-api.md in the follow-up.
| // Regex compilation is orders of magnitude more expensive per op than | ||
| // a cheap AST step; charging len(pattern) operations brings the cost | ||
| // into line with the operation budget's intent. | ||
| self.count_ops(pattern.len().max(1))?; |
There was a problem hiding this comment.
The new op charge sits behind the cache, but the expensive uncached regex work — validate_regex_pattern — is still charged zero operations.
This is a different hole from the pattern.len()-proxy question I raised on the charge itself; it is about where the charge is placed relative to the cache.
validate_regex_pattern (functions/regex.rs:35) is called by every regex function before get_or_compile_regex — see regex.rs:538, 560, 582, 650, 692. It does a full regex_syntax AST parse, an AST→HIR translation, and two recursive visitor walks (check_ast_portability, check_hir_portability). None of that is cached, and none of it is charged: grepping functions/regex.rs shows the only accounting in these functions is ctx.count_string_ops(s.len()) on the haystack. The pattern contributes nothing to either budget.
So the two costs land on opposite sides of the cache:
| work | cached? | charged? |
|---|---|---|
validate_regex_pattern (parse + translate + 2 walks) |
no — runs every call | 0 ops |
RegexBuilder::build |
yes | pattern.len() ops + 256 KiB (this PR) |
The new charge at this line is inside get_or_compile_regex after the early return at line 1661-1663, so on a cache hit it is skipped entirely — correctly, since no compile happened. But validation ran anyway. The result is that the one per-call, unavoidable, unbounded-by-cache cost is the one with no meter on it.
That inverts the PR’s own reasoning. The comment here says regex work is "orders of magnitude more expensive per op than a cheap AST step" and should be charged accordingly — but [re_findall(string(i), P) for i in range(N)] with a constant P compiles once (1 charge) and validates P N times (0 charges). Making P a long-but-legal pattern (deep alternation, many named groups, nested classes — all things the visitors walk) scales the per-iteration cost with len(P) while the charge stays flat. The regex_cache_hit_not_recharged test in this diff exercises exactly that shape and, by design, asserts the charge does not grow.
The straightforward fix is to charge for validation where it happens, in validate_regex_pattern — it already has the pattern length and both the AST and HIR in hand, so ctx.count_string_ops(pattern.len()) (or an op charge derived from hir.properties()) at the top would meter the per-call cost. That also removes the need for the compile-side count_ops here to stand in for it. Alternatively, hoisting validation inside get_or_compile_regex so it too benefits from the cache would eliminate the repeated cost rather than charge for it — arguably better, since validation is a pure function of the pattern and its result is cacheable.
Either way, worth noting in the commit body that pattern-side cost remains unmetered on cache hits, so this is not read as fully closing the regex DoS surface.
There was a problem hiding this comment.
Valid observation. validate_regex_pattern runs on every call (uncached) and is uncharged, while the compile (which is cached) carries the charge. This is an asymmetry.
Hoisting validation inside get_or_compile_regex so it benefits from the cache is the cleaner fix — it eliminates the repeated cost rather than charging for it. Deferring this to a follow-up since it touches the regex function implementations in functions/regex.rs and the interaction with the portability checks needs careful thought. Added a note in the commit body that pattern-side validation cost remains unmetered on cache hits.
f3cf409 to
15f6127
Compare
| self.current_memory = child.current_memory; | ||
| self.peak_memory = child.peak_memory; | ||
| self.operation_count = child.operation_count; | ||
| self.regex_cache_bytes = child.regex_cache_bytes; |
There was a problem hiding this comment.
absorb_counters overwrites regex_cache_bytes instead of accumulating, and the unresolved-iterable branch never seeds the child — so a parent accumulated regex charge gets clobbered to zero and then silently discarded.
The loop-body path seeds the child at line 1522 (child.regex_cache_bytes = self.regex_cache_bytes), so the assignment here is a no-op-or-growth there. But the unresolved branch at line 1456 builds its child via child_evaluator only, which hard-codes regex_cache_bytes: 0 (line 1386), and it does not get the line 1522 seeding. When line 1479 then calls absorb_counters, this assignment moves the parent counter backwards.
Concretely, [len(re_findall("x", s)) + len([y for y in Param.L]) for s in ["p","q","r"]] with Param.L unresolved (the validation-time path):
- Iteration N:
regex_bytes_baseline = self.regex_cache_bytes(sayB), child seeded toB. - Child compiles the distinct pattern
s→child.regex_cache_bytes = B + 1048577,child.current_memoryup by the same. - Child hits the inner unresolved comprehension → grandchild built with
regex_cache_bytes: 0,current_memoryinherited (still carries the charge). child.absorb_counters(&grandchild)→child.regex_cache_bytes = 0.current_memorykeeps the charge.self.absorb_counters(&child)→self.regex_cache_bytes = 0.- Line 1554:
regex_delta = 0.saturating_sub(B) == 0, so line 1555 resetscurrent_memoryback tomemory_baseline— the charge is dropped entirely. - Line 1545 moves the cache back, so the compiled
Regexis still retained.
Net effect: the cache fills toward MAX_REGEX_CACHE_ENTRIES (~32 MiB of real memory by this PR own accounting) while current_memory and the reported peak_memory show zero regex cost — precisely the accounting hole the PR is closing. saturating_sub makes it silent rather than a panic.
Two things to fix. Seed the unresolved branch the same way the loop body does:
let mut child = self.child_evaluator(&combined);
child.regex_cache = std::mem::take(&mut self.regex_cache);
child.regex_cache_bytes = self.regex_cache_bytes;
...
self.absorb_counters(&child);
self.regex_cache = child.regex_cache;and, as an invariant here, make the counter monotonic so no future child_evaluator call site can silently rewind it:
self.regex_cache_bytes = self.regex_cache_bytes.max(child.regex_cache_bytes);Seeding the unresolved branch also means the type-probe evaluation reuses the cache instead of recompiling, which is the behavior the "Regex Cache" section of evaluator.md already describes for child evaluators generally.
A regression test in the shape of the expression above — assert peak_memory still scales with distinct patterns when the body contains a comprehension over an unresolved symbol — would pin this. None of the four new tests in test_memory.rs nest a comprehension, so they all take the line 1522 path and none exercise this.
| /// would consume excessive compile time. | ||
| pub(crate) const REGEX_SIZE_LIMIT: usize = 1 << 20; // 1 MiB | ||
|
|
||
| /// Result of expression evaluation. |
There was a problem hiding this comment.
Both stated rationales in this doc block are inaccurate, and one of them is contradicted by the code.
1. 1 MiB is not the regex crate default. RegexBuilder::size_limit defaults to 10 * (1 << 20) — 10 MiB. So REGEX_SIZE_LIMIT = 1 << 20 is a 10x tightening, not parity. Worth stating that correctly, because it is the load-bearing justification for the value: a reader who believes this matches upstream will assume raising it is free.
2. "The memory budget (not the per-regex limit) is the primary defense against cache growth" is backwards under the defaults. The two new constants bound the total accounted regex charge at MAX_REGEX_CACHE_ENTRIES × (REGEX_SIZE_LIMIT + pattern.len()) ≈ 32 MiB. DEFAULT_MEMORY_LIMIT is 100_000_000. 32 MiB < 100 MB, so with default limits the memory budget can never reject a regex-cache insert — the entry-count cap is the only thing bounding the cache. The memory budget only becomes the operative defense for callers who explicitly set with_memory_limit below ~33 MB, which is what the new regex_cache_charged_to_memory_budget test does (500 KiB).
That inversion propagates into the spec. specs/expr/evaluator.md:476-480 says the memory charge
ensures that expressions with many distinct patterns hit the memory limit rather than silently consuming unbounded memory.
Under this implementation that is false in both halves: with default limits many-distinct-pattern expressions do not hit the memory limit (they stop at 32 cached entries, 32 MiB accounted), and past the cap they are explicitly not charged — line 1689 skips the charge entirely, and line 1671 charges only pattern.len() operations. So an expression with 5000 distinct patterns compiles 5000 regexes, is charged 32 MiB of memory total, and completes. That is a deliberate and defensible design — the cap bounds retained memory, which is what current_memory is supposed to model — but the spec describes the opposite mechanism, so a future reader auditing the DoS surface will look for a memory-limit rejection that does not exist.
Suggested rewording that matches the code: the entry cap is the primary bound on cache growth; the memory charge exists so that peak_memory reflects retained regex cost and so that callers running with a small explicit memory_limit are protected. And specs/expr/evaluator.md should say that patterns past the cap are unbounded in count, charged only pattern.len() operations.
| // — the same "deliberately broad ceiling" pattern used by the | ||
| // `repr_*` preflight (`MAX_ESCAPE_EXPANSION`). This bounds total | ||
| // cache memory to at most MAX_REGEX_CACHE_ENTRIES × REGEX_SIZE_LIMIT. | ||
| if self.regex_cache.len() < MAX_REGEX_CACHE_ENTRIES { |
There was a problem hiding this comment.
Gating the memory charge on the cap removes the only thing that bounded the number of regex compiles, so past-cap compiles are now limited only by pattern.len() operations.
Skipping the charge past the cap is correct for current_memory fidelity — a discarded Regex should not stay on the books. But the accounting change has a side effect on the compile-time surface, because there are now no bounds on past-cap compiles other than the count_ops(pattern.len().max(1)) charge at line 1671.
The two mechanisms trade off against each other:
| distinct compiles bounded by | |
|---|---|
| unconditional charge | memory_limit / REGEX_SIZE_LIMIT ≈ 95 at the 100 MB default |
| conditional charge (this code) | operation_limit / pattern.len() — nothing meaningful |
pattern.len() is not a proxy for compile cost, because counted repetition is superlinear: (?:a{200}){199} is 16 characters and compiles to an NFA just under the 1 MiB size_limit this PR sets, yet it is charged 16 operations. Making each pattern distinct so it misses the cache is a string concat:
[re_findall("a", "(?:a{200}){199}" + string(i)) for i in range(500000)]The first 32 entries fill the cache and charge ~32 MiB — comfortably under the 100 MB default, so nothing rejects them. Every subsequent iteration compiles a ~1 MiB NFA, uses it once, and drops it, charged roughly 20 ops (pattern.len()) plus the per-item count_op(). Against DEFAULT_OPERATION_LIMIT of 10 million that permits on the order of 500k near-megabyte NFA compilations before the operation limit trips. Peak memory stays flat at ~33 MiB the whole time, so the memory budget never sees it; only wall-clock grows.
Before this revision the unconditional charge capped distinct compiles at ~95 and this shape was rejected. The conditional charge is the right call for accounting, but it needs the compile cost metered on the operation budget to keep the same protection. Options:
- Charge the operation budget from the compiled program size rather than the source length.
regex_syntaxis already a dependency, andvalidate_regex_pattern(functions/regex.rs:35) already builds the HIR before this function runs — so the product ofRepetitionbounds combined withhir.properties()gives a cheap superlinear-aware estimate. Charging that (scaled) as ops would price(?:a{200}){199}at ~40k ops instead of 16 and cap the loop above at a few hundred iterations. - Or charge the past-cap compile to memory transiently:
check_memory(entry_cost)?without retaining it, plus acount_opsproportional toentry_cost / 256so repeated compiles accumulate against the op budget.
Either way this is worth an explicit note in specs/expr/evaluator.md next to the "patterns compiled after the cap ... are used once and discarded without a persistent memory charge" sentence — as written that reads as purely a memory statement, and the CPU consequence is invisible.
| .unwrap(); | ||
| // The comprehension should succeed. One cached compile charges ~1 MiB; | ||
| // 100 iterations reuse the cache hit with no re-charge, so peak stays | ||
| // well under 2 MiB. |
There was a problem hiding this comment.
This explicit assertion is vacuous — peak_memory < 2_000_000 cannot fail once unwrap() has succeeded.
eval_bounded sets with_memory_limit(2_000_000), and check_memory rejects any current_memory projected past the limit before it is applied (evaluator.rs:1649-1659). peak_memory is only ever the running max of current_memory, so peak_memory <= memory_limit is an invariant of a successful evaluation. The assertion restates the limit that was just passed in.
The test is not useless — the real detector is the .unwrap(), since 100 re-charges of ~1 MiB would blow the 2 MiB limit and panic. But that means the assertion contributes nothing, and the failure mode is a raw unwrap panic rather than the readable diagnostic the other three new tests produce.
Tightening it to pin the actual expected value would make it a real regression test — one cached entry is REGEX_SIZE_LIMIT + 1 = 1_048_577 bytes, so the interesting bound is just above one entry, not just below two:
// Exactly one compile: ~1 MiB charged once, not 100x.
assert!(
r.peak_memory < 1_100_000,
"cache hit should not re-charge; peak was {}",
r.peak_memory
);That distinguishes "one charge" from "two charges", which the current bound does not, and it fails with a message instead of a bare panic. Pairing it with the regex_cache_peak_memory_proportional_to_distinct_patterns style — comparing against a known-single-pattern baseline — would be equally good.
|
|
||
| /// Per-regex compiled-program size limit: 1 MiB. Passed to | ||
| /// `RegexBuilder::size_limit` to reject adversarial NFA patterns. | ||
| pub(crate) const REGEX_SIZE_LIMIT: usize = 1 << 20; |
There was a problem hiding this comment.
Two non-public constants have been added to public-api.md, which by AGENTS.md is the document that "fully describes the crate public API."
REGEX_SIZE_LIMIT is pub(crate) and MAX_REGEX_CACHE_ENTRIES is private, both inside pub(crate) mod evaluator (eval/mod.rs:11) — neither is reachable from outside the crate, and neither is re-exported from lib.rs. Every other entry in this ## Constants block is pub and re-exported. Listing these two here, with their visibility modifiers copied verbatim, makes the document ambiguous about what it is enumerating: a reader diffing public-api.md against cargo doc output will find these two missing and cannot tell whether that is a doc bug or an export bug.
Two coherent resolutions:
- Move both to
specs/expr/evaluator.md(which already documents them in the new "Bounding" section) and drop them from here. This keepspublic-api.mda strict enumeration of the public surface. - Or make
REGEX_SIZE_LIMITgenuinelypuband re-export it fromlib.rs, and keep it here without thepub(crate)marker. There is an argument for this:function_library.rs:64now uses it in the default body ofEvalContext::get_or_compile_regex, which is public API that third-party impls inherit and may override. A host overriding that method to add its own regex functions currently has to hardcode1 << 20to match built-in behavior — exactly the duplication this diff removed in-crate.DEFAULT_MEMORY_LIMIT/DEFAULT_OPERATION_LIMITare public for the same reason: callers need to see the defaults they are overriding.MAX_REGEX_CACHE_ENTRIEShas no such argument and should just move toevaluator.md.
Option 1 is the smaller change; option 2 closes a real gap for external EvalContext implementors. Either is fine, but the current state documents unreachable items as public API.
The per-evaluation regex cache was unbounded and invisible to the memory budget. A comprehension with a per-iteration pattern could grow the cache into the gigabytes while the evaluator reported kilobytes. Changes: - Charge each cached regex to current_memory at REGEX_SIZE_LIMIT cost; patterns past the cache cap compile-and-discard without persistent charge - Cap the cache at 32 entries (MAX_REGEX_CACHE_ENTRIES) - Keep per-regex size_limit at the regex-crate default of 1 MiB; the memory budget and entry cap are the primary defense against cache growth - Charge operations proportional to pattern length on cache miss - Preserve cache charges across comprehension iteration baseline resets - Propagate regex_cache_bytes through absorb_counters so nested comprehension paths do not silently drop charges Note: pattern-side validation cost (validate_regex_pattern) remains unmetered on cache hits. Hoisting validation into get_or_compile_regex so it benefits from the cache is deferred to a follow-up. Specs updated: evaluator.md, function-library.md, public-api.md. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
15f6127 to
3c21cc4
Compare
|
Reviewed at Summary: the memory hole is closed, and the cap — not the charge — is what closes it. Real retained memory is bounded at 32 entries regardless of everything else I found below; measured Blocking1. The description doesn't match the codeThe body's defense #3 says
Everything downstream of 256 KiB is therefore off: "~380 distinct patterns" is ~95 at 1 MiB, and with the 32-entry cap the regex cache can never reach the 100 MB default limit at all — the maximum total charge is 32 MiB. The "105 MB → rejected at 2 MB" CLI figure needs re-measuring too. Either restore the 256 KiB change or rewrite the description. As it stands, approving the stated design approves something different. 2. A single
|
memory_limit |
result |
|---|---|
| 100,000 | MemoryLimitExceeded |
| 500,000 | MemoryLimitExceeded |
| 1,048,577 | MemoryLimitExceeded |
| 2,000,000 | OK |
openjd-for-js/src/expr.rs:483 lets a host pass an arbitrary limit, so this is reachable from a public API rather than only from tests. This ships with the PR, so it needs a decision now rather than a follow-up: either accept it and name it in the breaking-change section, or charge something smaller.
Worth noting the comment cites MAX_ESCAPE_EXPANSION as precedent, but that bounds a transient allocation about to happen; this charge persists for the whole evaluation. If a real number is wanted later, regex_automata::meta::Regex::memory_usage() exists — regex::Regex doesn't re-export it, so it'd mean a direct regex-automata dep.
3. evaluator.rs:62 — the comment is wrong about the crate default
Set to 1 MiB — the same default the
regexcrate uses.
regex 1.13.1 defaults nfa_size_limit to 10 MiB (src/builders.rs:53). This tightens it 10×, which is a stronger claim than the one written. Same comment says "the memory budget (not the per-regex limit) is the primary defense against cache growth", which reads oddly next to line 53 — given the cap bounds total cache memory to 32 MiB and the default limit is 100 MB, the cap is the primary defense and the charge is what makes it visible.
Should fix here (one line, in a file you're already touching)
4. A comprehension over an unresolved iterable destroys the accumulated regex charge
child_evaluator initialises regex_cache_bytes: 0 (:1386). The resolved path repairs that at :1522; the unresolved branch at :1456 does not, and then absorb_counters (:1395) copies the 0 back over the parent's real total.
Measured, both at a 20 MiB limit:
| expression | result |
|---|---|
[re_findall('x', string(i)) for i in range(32)] |
MemoryLimitExceeded at 20,973,662 bytes — charge working |
same, each iteration also evaluating [y for y in Param.U] |
OK, peak_memory = 1,050,946 |
32 cached regexes, still in the cache after the loop, reported as ~1 MiB. The unresolved path is the validation path, so it's reachable from template validation with any Param.X the caller hasn't supplied.
To be clear about severity: this corrupts the counter, not the bound. The 32-entry cap still holds, so real RSS stays bounded either way — this is a reporting defect worth up to 32 MiB of under-charge, not a reopening of the original unbounded-memory bug.
Fix is one line — initialise regex_cache_bytes: self.regex_cache_bytes in child_evaluator, matching how current_memory and peak_memory are already inherited at :1373-75. That makes the invariant hold on both paths and makes :1522 redundant.
Good follow-ups
5. The op charge measures the wrong quantity
count_ops(pattern.len()) — compile cost tracks the expanded NFA, not source length:
| pattern | len | ops charged | compile wall |
|---|---|---|---|
[a-z] |
5 | 7 | 0.21 ms |
(?:a{50}){50}z |
14 | 16 | 3.9 ms |
(?:a{90}){90}z |
14 | 16 | 9.9 ms |
47× the work for 2.3× the charge. Past the cap every call recompiles (2835 ops for 128 compiles), so the 10M op budget permits roughly 450,000 compiles of an adversarial pattern. Memory is bounded now, which makes compile CPU the weakest limit.
Mostly pre-existing — N distinct patterns cost N compiles before this PR too. What's new is the past-cap recompile amplification for a template that reuses more than 32 distinct patterns in a loop. A fixed REGEX_SIZE_LIMIT / 256 ops per compile would match the 1-op-per-256-bytes scale count_string_ops already uses and bound compiles to a few thousand.
6. The lazy-DFA cache is still unaccounted
size_limit bounds only the compiled NFA. regex 1.13.1 also defaults hybrid_cache_capacity to 2 MiB per regex (src/builders.rs:55), allocated lazily during search. Worst case 32 × 2 MiB = 64 MiB of real RSS on top of the 32 MiB charged. If the intent is "real memory is bounded and the budget sees it", pinning .dfa_size_limit(...) on the builder and folding it into entry_cost closes it.
7. Two smaller ones
validate_regex_pattern (functions/regex.rs:582) does a full regex_syntax parse + HIR translate on every call, cache hits included, charged nothing. Not introduced here, but it's the other half of the per-call regex cost given the PR's premise.
re_match keys the cache on format!("^(?:{})", pat) (:539) while re_search keys on pat (:561), so a template using both on one pattern occupies 2 of the 32 slots. Fine as a ceiling, but worth a line in the spec's Bounding section since 32 is justified as "generous — most use 1–3 distinct patterns".
Tests
regex_cache_hit_not_recharged is close to vacuous: it asserts peak_memory < 2_000_000 after eval_bounded(..., 2_000_000) returned Ok. Success already implies current_memory never exceeded the limit, and peak_memory is its max, so the assertion can't fail once the unwrap() succeeds. Asserting that the peak for 100 iterations of one constant pattern equals the peak for 1 iteration (±transients) would pin the mechanism.
Nothing asserts a past-cap pattern still returns the right answer — regex_cache_cap_bounds_memory runs 50 patterns but only compares peaks, so a cap that silently returned the wrong regex past 32 entries would pass.
Nothing pins finding 4; the probe below is ready to promote.
What's good
- The four-mechanism framing is the right decomposition, and the cap is the mechanism that actually bounds real memory.
:1554, theregex_deltaadd-back, is a genuinely subtle interaction with the per-iteration baseline reset, and the comment earns its space.regex_cache_peak_memory_proportional_to_distinct_patternsis a real mutant for it — delete the delta and both the 1-pattern and 20-pattern peaks collapse to ~1 MiB and the test fails.- Charging only on retain (
:1689) rather than on every compile is the right call, and the comment says why: charging discarded compiles would makecurrent_memorydiverge from real memory in the other direction. - Hoisting the magic
1 << 20out of two call sites into one named constant, including theEvalContextdefault impl. - The spec was updated in the same change, and
test_string_operation_counting.rs:305-311shows the op arithmetic instead of just bumping 6 to 7.
Refuted but considered, so nobody re-investigates
std::mem::takeat:1521losing the cache on the error path — it does, but?propagates out of the whole evaluation and the cache is dropped anyway.regex_deltadouble-counting across iterations — the baseline is re-read each iteration (:1514) and the counter is monotonic within the loop, so the delta is exact.saturating_addonentry_costhiding overflow —pattern.len()is bounded by the 64 KiB expression cap.count_ops(pattern.len())rejecting legitimate long patterns — 64 KiB source cap against a 10M op limit; not reachable.
Probe
Drop into crates/openjd-expr/tests/integration/ and register it in tests/integration.rs:
use openjd_expr::{ExprType, ExprValue, ParsedExpression, SymbolTable, DEFAULT_OPERATION_LIMIT};
fn run(expr: &str, mem: usize, st: &SymbolTable)
-> Result<openjd_expr::EvalResult, openjd_expr::ExpressionError> {
ParsedExpression::new(expr).and_then(|p| {
p.with_memory_limit(mem)
.with_operation_limit(DEFAULT_OPERATION_LIMIT)
.evaluate_with_metrics(&[st])
})
}
/// Finding 4: a nested comprehension over an unresolved iterable wipes the
/// parent's accumulated regex charge.
#[test]
fn probe_unresolved_nested_comp_drains_regex_charge() {
let mut st = SymbolTable::new();
st.set("Param.U", ExprValue::unresolved(ExprType::list(ExprType::INT))).unwrap();
const LIMIT: usize = 20 * (1 << 20);
let control = run("[re_findall('x', string(i)) for i in range(32)]", LIMIT, &st);
let leak = run(
"[len(re_findall('x', string(i))) + len([y for y in Param.U]) for i in range(32)]",
LIMIT, &st,
);
println!("control: {:?}", control.as_ref().map(|r| r.peak_memory));
println!("leak: {:?}", leak.as_ref().map(|r| r.peak_memory));
// Today: control Err(20973662 > 20971520), leak Ok(peak=1050946).
assert!(control.is_err() && leak.is_err(), "regex charge leaked through the unresolved branch");
}
/// Finding 5: op charge is pattern length, not compile cost.
#[test]
fn probe_op_charge_vs_compile_cost() {
let st = SymbolTable::new();
for pat in [r"(?:a{50}){50}z", r"(?:a{90}){90}z", r"[a-z]"] {
let t = std::time::Instant::now();
let r = run(&format!("re_findall('a', '{pat}')"), usize::MAX, &st).unwrap();
println!("pat={pat:18} len={:3} ops={:3} peak={:9} wall={:?}",
pat.len(), r.operation_count, r.peak_memory, t.elapsed());
}
}
/// Finding 2: flat charge means one trivial regex needs a >1 MiB limit.
#[test]
fn probe_flat_charge_for_trivial_pattern() {
let st = SymbolTable::new();
println!("peak={}", run("re_findall('abc', 'b')", usize::MAX, &st).unwrap().peak_memory);
for lim in [100_000usize, 500_000, 1_048_577, 2_000_000] {
println!("limit={lim:9} => {:?}", run("re_findall('abc', 'b')", lim, &st).is_ok());
}
}
/// Finding 1: the size limit in the code is 1 MiB, not the described 256 KiB.
#[test]
fn probe_size_limit_actual_value() {
let st = SymbolTable::new();
for pat in [r"(?:a{100}){100}z", r"(?:a{200}){200}z"] {
println!("pat={pat:20} => {:?}",
run(&format!("re_findall('a', '{pat}')"), usize::MAX, &st).map(|_| "accepted"));
}
}
|
Approved. Consolidating the non-blocking items here so they don't get lost in the long comment above — none of these gate the merge, and the memory bound this PR establishes holds without any of them. Two small things still worth doing in this PR, since both are text and neither touches behaviour:
Follow-ups, roughly by size of the remaining gap:
Nits:
|
|
For anyone reading this later: a plain-language walkthrough of what the patch does and why the four mechanisms are ordered the way they are. Nothing new here relative to the review above — this is the explainer. ProblemThe evaluator keeps a per-evaluation Feed it a pattern that changes per iteration — Solution — four moves, in order of how much each doesThe cap is the fix. The charge is the visibility. When an entry is retained, charge The delta is the subtle part. The comprehension loop resets The op charge is the weakest. Walkthrough
Iteration 0. Baselines saved: Iteration 1. Baselines: Iteration 2. Pattern Before the PR, iteration 2 would have inserted and continued, and so would iteration 4000. The one door the repair missesRun the same thing, but each body also evaluates Measured: 32 retained regexes, still in the cache, reported as ~1 MiB. Does the counter stop the shelf from filling? No. The 32 slots do. The counter only lets you see it — which is why the one-line fix (seed |
2 participants
What was the problem/requirement? (What/Why)
The per-evaluation regex cache (
HashMap<String, regex::Regex>) was unboundedand invisible to the memory budget. A list comprehension with a per-iteration
pattern (e.g.
'(?:a{100}){100}z' + string(i)) compiled and cached a new~499 KB regex each iteration. At N=4000, the process consumed ~2 GB of RSS
while the evaluator reported ~262 KB
peak_memory— an 8,000× underreport.Neither
memory_limitnoroperation_limitprevented the allocation.What was the solution? (How)
Four interlocking defenses:
REGEX_SIZE_LIMIT + pattern.len()tocurrent_memoryviacheck_memory.cached. Legitimate templates use 1–3 distinct patterns.
repetition pattern is now rejected outright.
count_ops(pattern.len())makes compilation visible to the op budget.Regex cache charges are preserved across the comprehension iteration baseline
reset (the
current_memory = memory_baselineline now adds back the delta).What is the impact of this change?
Templates with >~380 distinct regex patterns in a single expression will now
hit the memory limit. Templates with patterns whose compiled NFA exceeds 256 KiB
(e.g.
(?:a{100}){100}) are now rejected. All conformance suite patterns andrealistic template patterns compile well under 256 KiB.
How was this change tested?
test_memory.rsasserting each mechanismWas this change documented?
Spec update to
specs/expr/evaluator.md(Regex Cache section) pending.Is this a breaking change?
Potentially — templates relying on patterns with compiled NFA >256 KiB or >32
distinct patterns per expression will see new errors. These patterns are
adversarial by construction; no legitimate template should be affected.