Fix catastrophic backtracking in implicit link detection - #613
Open
jizhi0v0 wants to merge 2 commits into
Open
Conversation
The implicit link pattern contains two nested quantifiers that make
NSRegularExpression (ICU) backtrack exponentially, blocking whichever
thread runs the match. Both are reachable from ordinary terminal output
via linkForClick / updateHoverLink / reportLink, i.e. from mouseUp,
mouseMoved and flagsChanged.
1. The IPv6 literal was `\[[:0-9a-fA-F]+(?:[:0-9a-fA-F]*)+\]`. The inner
`(?:X*)+` is pure redundancy -- it accepts exactly the language of
`X+` -- but it explodes on a scheme followed by an unterminated
bracket, which real URLs produce: a truncated address such as
`ssh://[2001:0db8:85a3:0000:0000` takes 1.3s, and the RFC 6874 form
`https://[2001:db8::1%25eth0]/x` never terminates, because the `%25`
zone-ID escape is not in the hex class and so blocks the closing `]`.
2. The scheme body was `(?:IPV6|CHARS+SUFFIX?)+`, a `+` directly inside a
`+`, so a run of N body characters could be split across iterations in
exponentially many ways. ICU normally escapes early because the
trailing `(?<![,.])` rejects only one end position, but when the match
ends in a *run* of `.` or `,` -- a URL followed by an ellipsis, dot
leaders, or empty CSV fields -- every end position fails and the full
enumeration is forced. Measured on the old pattern:
https://example.com/a/b/c + 16 dots (41 chars) -> 547 ms
https://example.com/a/b/c + 18 dots (43 chars) -> 3,689 ms
mailto:a@b.com + 19 dots (33 chars) -> 9,698 ms
Each additional character multiplies the time by ~2.6.
Consuming one token per iteration removes the ambiguity: there is now
exactly one way to match a given run. Two details preserve the original
grammar -- the first token can never be a bracketed suffix, and a suffix
is admitted only directly after a CHARS character (the lookbehind);
without it a suffix could also follow an IPv6 literal or another suffix,
which `CHARS+SUFFIX?` cannot produce.
All the inputs above now complete in under 0.05 ms. Differential testing
of the old and new patterns over 300,000 random terminal-like strings
plus hand-picked edge cases (IPv6 with port and path, trailing-punctuation
trimming, parenthesised suffixes, every path branch) reports zero
difference in the matches produced.
linkForClick always asked for `.explicitAndImplicit` and only afterwards consulted linkVisibleForClick, which -- depending on linkHighlightMode -- discards implicit matches unconditionally. On macOS the default is `.hoverWithModifier`, so every plain click and every drag-selection release ran the implicit regex over the whole wrapped line group and then threw the result away. Decide up front whether an implicit match could be accepted, and ask for `.explicitOnly` when it could not. This mirrors the early-out that updateHoverLink already performs on the hover path. The gate is a necessary condition, never narrower than linkVisibleForClick: `.always` and `.alwaysWithModifier` both return `match.isExplicit`, so an implicit match can never qualify there; the hover modes additionally require linkHighlightRange to be non-nil, since comparing it against a non-optional array of row ranges is false when it is nil. Explicit hyperlinks are unaffected -- they are still resolved in every mode.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The implicit link pattern can backtrack exponentially, blocking the main thread for seconds to minutes on ordinary terminal output. I hit this in a SwiftUI terminal app: releasing a drag-selection froze the whole UI, and
sampleshowed 2933 of 2933 main-thread frames inThe two bombs
1.
ipv6URLPattern—\[[:0-9a-fA-F]+(?:[:0-9a-fA-F]*)+\]The inner
(?:X*)+is pure redundancy (it accepts exactly the language ofX+) but adds an exponential when a scheme is followed by an unterminated bracket. Both of these are shapes real output produces:ssh://[2001:0db8:85a3:0000:0000(truncated address, 31 chars)https://[2001:db8::1%25eth0]/x(valid RFC 6874 zone-ID URL)The RFC 6874 case never terminates because
%25is not in the hex class, so it blocks the closing].2. Scheme body —
(?:IPV6|CHARS+SUFFIX?)+A
+directly inside a+: a run of N body characters can be split across iterations in exponentially many ways. ICU normally escapes early, because the trailing(?<![,.])rejects only the one end position whose preceding character is.or,— backing off by one succeeds immediately. But when the match ends in a run of./,, every end position fails and the full enumeration is forced. That shape is common: a URL followed by an ellipsis, dot leaders, or empty CSV fields.https://example.com/a/b/c+ 16 dotshttps://example.com/a/b/c+ 18 dotsmailto:a@b.com+ 19 dotshttps://example.com/a/b/c+ 20 dots~2.6× per additional character. Note how short these are — a length cap is not a viable mitigation, and
NSRegularExpressionexposes no time or step limit (ICU'suregex_setTimeLimitis not surfaced by Foundation, andenumerateMatches'stopflag only fires between matches, so it cannot interrupt backtracking inside one).The fix
Consume one token per iteration, so there is exactly one way to match a given run. Two details keep the grammar identical to
CHARS+SUFFIX?:Without that lookbehind a suffix could also follow an IPv6 literal or another suffix, which the original cannot produce — differential testing found exactly those cases and nothing else, which is what motivated adding it.
All inputs above now complete in under 0.05 ms.
Verification
BENCHMARK_DISABLE_JEMALLOC=true swift test; the Benchmarks target otherwise needs jemalloc installed).Second commit
linkForClickalways requested.explicitAndImplicitand only afterwards consultedlinkVisibleForClick, which discards implicit matches outright in several modes. On macOS the default is.hoverWithModifier, so every plain click and every drag-selection release ran the implicit regex over the whole wrapped line group and then threw the result away. This decides up front whether an implicit match could be accepted, mirroring the early-outupdateHoverLinkalready performs on the hover path. The gate is a necessary condition and never narrower thanlinkVisibleForClick; explicit hyperlinks are unaffected.Not included
updateHoverLinkandreportLinkstill run implicit detection on everymouseMovedwhile a modifier is held. With the pattern fixed that is no longer a hang risk, just steady avoidable work — it seemed better suited to a separate change than folded in here.