Skip to content

Fix quadratic parsing of leading whitespace - #50

Open
theRizwan wants to merge 1 commit into
errwischt:masterfrom
theRizwan:fix/quadratic-leading-whitespace
Open

Fix quadratic parsing of leading whitespace#50
theRizwan wants to merge 1 commit into
errwischt:masterfrom
theRizwan:fix/quadratic-leading-whitespace

Conversation

@theRizwan

Copy link
Copy Markdown

Fixes #49.

parse() was quadratic in the length of a leading whitespace run, so a long run of spaces blocked
the event loop for seconds.

input before after
8,000 spaces 231 ms 0.16 ms
16,000 spaces 924 ms 0.37 ms
32,000 spaces ~3,700 ms 0.63 ms

Before: ~4× per doubling. After: ~2×.

Cause

geckoRe and javaScriptCoreRe began with ^\s* followed immediately by a group that also matches
whitespace — (.*?) and ([^@]*). A leading run of spaces could be divided between the two in n
ways, and a non-matching line forced the engine through every split.

chromeRe, winjsRe and nodeRe are untouched: each has a literal at right after ^\s*, which
anchors the boundary and prevents the ambiguity.

Why it is worth fixing

err.stack embeds the message, so attacker-influenced text reaches the parser directly:

const err = new Error('Invalid value: \n' + ' '.repeat(30000));
parse(err.stack);   // ~3.3s of blocked event loop

Availability only — nothing is disclosed or corrupted — but a single request can stall a service
that parses stack traces from errors carrying user-supplied values.

The change

Dropping the leading \s* is what makes the match linear. It was doing one useful thing, keeping
leading whitespace out of the captured methodName, so that moved to where the capture is read:

methodName: (parts[1] && parts[1].trim()) || UNKNOWN_FUNCTION,

Behaviour is unchanged — including the UNKNOWN_FUNCTION fallback, since an all-whitespace capture
now trims to '', which is still falsy.

I checked this rather than assumed it: removing ^\s* alone broke parses JavaScriptCore errors,
which expects methodName: '_exampleFunction' and was getting ' _exampleFunction'. The trim
is what keeps that fixture green.

Tests

All 35 existing fixtures pass unchanged, and one timing test added.

Confirmed it catches the regression — on current main the suite reports 35 passing, 1 failing
and takes 4 seconds; with the fix, 36 passing in 17 ms.

The threshold is deliberately loose (8× headroom plus 100 ms) so it will not flake on a slow or noisy
CI machine, while still failing decisively on the quadratic behaviour, which is ~16× for the same
input growth.

`geckoRe` and `javaScriptCoreRe` began with `^\s*` followed immediately by
a group that also matches whitespace — `(.*?)` and `([^@]*)` respectively.
A leading run of spaces could therefore be divided between the two in n
ways, and a line that did not match forced the engine through every split:

    n= 1000     4.2 ms
    n= 2000    14.6 ms
    n= 4000    57.9 ms
    n= 8000   231.3 ms
    n=16000   924.0 ms

`err.stack` embeds the message, so attacker-influenced text reaches this
directly: a 30k-space message took ~3.3s of blocked event loop.

The leading `\s*` is redundant, since the following group already matches
whitespace. Its only real effect was keeping leading whitespace out of the
captured methodName, so that is handled where the capture is read.

After: 8000 -> 0.16ms, 16000 -> 0.37ms, 32000 -> 0.63ms, ~2x per doubling.

`chromeRe`, `winjsRe` and `nodeRe` are untouched: each has a literal `at `
after `^\s*`, which anchors the boundary and prevents the ambiguity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parse() is quadratic in leading whitespace: 30k-space error message blocks the event loop for ~3s

1 participant