🏗️🔧：read a commit message in linear time - #1821
Merged
Merged
Conversation
CodeQL was right, and the input really is uncontrolled: a commit message comes from whoever opened the pull request, and the queue reads it while holding credentials that can write here. `/[\r\n]+$/` takes time proportional to the square of the run of newlines it is asked about -- 80,000 of them took 3.7 seconds, so a million would hang the job for minutes. Sweeping the rest of what I had written found a second one CodeQL did not report, and worse: `\S` matches a colon, so `^\S+:\S+( \S+)*$` let the engine try every colon in an Assisted-by value as the split point. Same verdicts either way, 8000 times faster. A third of the time was going on building an Intl.Segmenter for every line rather than once. Both are held to a budget by tests that fail in forty-odd seconds if either spelling comes back. Signed-off-by: Derek Lewis <DerekNonGeneric@inf.is> Assisted-by: Claude-Code:claude-opus-5
✅ Deploy Preview for gh-pages-openinf ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
DerekNonGeneric
added
the
commit-queue
openinf-commit-queue
Bot
removed
the
commit-queue
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
1 participant
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.
From the CodeQL alert on #1814. Unlike the semgrep one, this is a true positive, and the input really is uncontrolled: a commit message comes from whoever opened the pull request, and the commit queue reads it while holding credentials that can write to this repository.
Measured before fixing
Four times the work for twice the input. A million newlines — trivial to put in a commit message — would hang the queue for minutes.
A second one CodeQL did not report
Sweeping every regex I had written turned up another, and worse:
/[\r\n]+$//^\S+:\S+( \S+)*$//\s#\d+$//^_?[a-z0-9]+(?:-[a-z0-9]+)*$//^[A-Z][A-Z0-9_-]*$/\Smatches a colon, so\S+:\S+let the engine try every colon in anAssisted-byvalue as the split point./^[^\s:]+:\S+( \S+)*$/gives identical verdicts on every real and rejected value, and runs 8000 times faster.And a third of the time was elsewhere
countGraphemesbuilt a newIntl.Segmenterper call — once per line of the message. Hoisting it to one instance cut 200,000 calls from 1174 ms to 294 ms. The three call sites for stripping trailing newlines are now one sharedlinesOf, written as a scan.After: 50k → 104 ms, 100k → 243 ms, 200k → 507 ms. Linear.
The guards
Two tests with a 3-second budget, on input where the old spellings are unmistakable. Verified by putting each one back:
/[\r\n]+$//^\S+:\S+( \S+)*$/Verification
nps test— 16/16 tasks, 65/65 tests