Repository navigation
OpenID discovery: make the HTML pre-strip in parseLinkAttrs linear - #78
Merged
Merged
Conversation
removed_re.sub() is quadratic in the length of the fetched page when a
comment, CDATA or script opener has no closer: every unclosed opener
rescans to the end of the document. OpenID discovery passes it the body
of the identifier URL the end user supplies.
Replace the sub() with one forward scan using fixed-string searches.
Once a closer is absent from the rest of the string, that kind of
opener is retired, which keeps the scan linear. Output is identical to
removed_re.sub('', html); removed_re stays defined for importers.
128 KB of unclosed comment openers: 12.70 s before, 0.0008 s after.
Consumer test suites: 294 tests, 0 failures on both trees.
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
3 participants
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.
As agreed by email with @necaris, this is the fix for the report I sent privately, opened as a public PR.
parseLinkAttrs()spends time quadratic in the length of the page it is given, and OpenID discovery handsit the body of whatever URL the end user typed. Measured on 3.2.0 through
parseLinkAttrs, the publicentry point
openid/consumer/discover.py:163calls:<!--with no--><![CDATA[with no]]><script >with no</script><scriptwith no>anywhereRoughly fifteen times the cost per quadrupling before; linear after; no measurable cost on an ordinary page.
Who controls the input. Discovery runs before the user has proved anything: the relying party fetches
the identifier URL the user supplied and parses the response. The page author therefore chooses the body,
and a few hundred kilobytes is an ordinary page size.
Cause.
openid/consumer/html_parse.py:81, applied at:209:Each alternative is a lazy pair. When an opener has no closer, the engine scans to the end of the document
and then tries again from the next opener, so every unclosed opener costs the rest of the page. The fourth
row is the same shape one level in:
[^>]*>scans to the end from every<scriptwhen there is no>.Fix - one forward scan with fixed-string searches. A lazy pair means "the first opener, then the
first closer after it", which
search()on a literal gives directly. The step that makes it linear ratherthan merely faster: once a closer is absent from the rest of the string, no later opener of that kind can
complete either, so that kind is retired. The script branch keeps the exact
<script\b(?!:)test bymatching that fragment at the fixed position of each
<scriptliteral, then takes the first>and thefirst
</script>after it.with
removeMarkupand its helpers added abovetag_expr(see the diff in this PR).removed_restaysdefined so that anything importing it keeps working.
Verification.
parseLinkAttrs()and of the strip itself: 351,092 inputs - everysequence of 0 to 3 tokens drawn from comment, CDATA and script openers and closers,
<script:ns>,<scriptx>, upper-case variants, stray<,>,-,], newlines, a non-ASCII letter whoselower-casing changes string length, 150,000 random longer token strings, every string of length 0 to 4
over
<>-![]/scripta:\n, 120,000 random longer ones, and 34 hand-written pages. The SHA-256 of allinput/output pairs is identical on both trees.
openid/test/linkparse.txt: 73 cases pass on both trees, and the wider consumer suite(
linkparse,test_htmldiscover,test_discover,test_consumer,test_examples,test_fetchers,test_parsehtml): 294 tests, 0 failures, 0 errors on both.hiding a link, upper-case
SCRIPT, a CDATA block, an unclosed<script- which the module docstring saysis ignored - a
script:namespace, and server plus delegate) plus a timing assertion on 128 KB ofcomment openers. On 3.2.0 the seven pass and the timing fails at 11.66 s; patched, all pass in 0.006 s.
<script>, unclosed comments and unclosed<![CDATA[are ignored.Two things the fix does not change.
html_findandhead_findrun on the stripped text and arelinear on the same shapes (0.0853 s at 2 MB of
<htmlopeners), so nothing after the strip takes overthe cost. And the rewrite deliberately avoids
str.lower()for the case-insensitive searches, becauselower-casing can change a string's length for some characters and would desynchronise the offsets;
re.IGNORECASEon a literal is linear and keeps the indices honest.Prior art. The maintained fork
python-openid2shipsopenid/consumer/with nohtml_parse.pyat all,and the PHP port's
Auth_OpenID_Parsedocumentation states this regex "is quite ineffective and may failwith the default
pcre.backtrack_limit" on large HTML and should usestrposinstead - which is the fixabove, reached independently in another language.
Found with AI assistance (Claude) and verified by hand.