Skip to content

Avoid rescanning for a comment close after unclosed comments - #1676

Merged
waylan merged 2 commits into
Python-Markdown:masterfrom
naaz234:html-comment-close-rescan
Oct 27, 2026
Merged

waylan merged 2 commits into
Python-Markdown:masterfrom
naaz234:html-comment-close-rescan

Conversation

@naaz234

@naaz234 naaz234 commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Description

HTMLExtractor.parse_comment searches for a comment close from each <!-- to the end of the raw data. When there isn't one, it passes < through as data and moves forward one character, so the next <!-- runs the same failing search again. n unclosed comments cost O(n^2) with no extensions enabled:

input before after
'<!--' * 8000 (31 KB) 0.27 s 0.014 s
'<!--' * 16000 (62 KB) 1.09 s 0.025 s
'<!--' * 32000 (125 KB) 4.96 s 0.052 s
'<!--' * 64000 (250 KB) 21.8 s 0.10 s

If no close exists after some position in rawdata, none exists after any later position in the same string. The parser now records the last failed search (the rawdata string, compared by identity, and the start position) and skips any later search that would start at or after it. The rest of the method is unchanged. Processing instructions, declarations, CDATA and end tags don't show this behavior, so only parse_comment needed the change.

Output is unchanged. I compared the rendered HTML with master on about 115k random documents built from comment, tag, code span and blank line fragments, using no extensions, md_in_html and extra, on Python 3.11 and 3.14. The full test suite passes on 3.11, 3.13 and 3.14, and with Pygments 2.7.1. flake8 is clean. The new test takes about 13 s on master and 0.06 s with this change.

AI Assistance Disclosure

  • No AI tools were used in preparing this PR.

  • AI tools were used and I have disclosed which ones, and have fully reviewed and verified their output.

    AI tools were use to assist with the following:

    • Code
    • Code Comments
    • Documentation
    • Changelog Entry
    • Commit Message
    • The Description Above
    • Other: finding the issue, the regression test, and the timing and output comparison scripts

Checklist

`HTMLExtractor.parse_comment` searches for `-->` (or `--!>`) from each
`<!--` to the end of the raw data. When there is none, it passes `<`
through as data and returns `i + 1`, so the next `<!--` repeats the
same failing scan. A run of n unclosed comments therefore costs O(n^2):
`'<!--' * 32000` (125 KB) takes about 5 seconds to convert and 250 KB
takes about 22 seconds.

If no close exists after some position in `rawdata`, none exists after
any later position in the same string either. The parser now records
the string and position of the last failed search and skips the search
when it would start at or after that position. The string is compared
by identity so the record is never reused for different raw data.
Output is unchanged.
@waylan

waylan commented Oct 24, 2026

Copy link
Copy Markdown
Member

Sigh. Some of these optimizations you submit seem so convoluted to me. I think: No one in the real world would ever actually try that. But, now that it is a known issue, someone could use it in a DOS attach on a server which parses untrusted Markdown. So I guess we should apply the patch.

As a reminder to myself, the spellcheck failure is for rescan which is potentially being added to the dictionary in #1664. If/when that is merged, this can be rebased and should pass. If that is not merged for some reason, then the word will need to be added here. Otherwise, this looks ready to go.

Maybe we could slow down to one outstanding PR at a time? The constant stream of PRs to review can be overwhelming.

@waylan waylan added the work-in-progress A partial solution. More changes will be coming. label Oct 24, 2026
@facelessuser

Copy link
Copy Markdown
Collaborator

I agree about the constant onslaught of PRs. Now with AI driven PRs, it's so low cost for people to pump them out, but more expensive (time wise) for projects that are doing thorough analysis of the PRs before merging. I've probably had more PRs across my projects in the past month then I normally receive for an entire year.

@naaz234

naaz234 commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Fair point on the pace, sorry about the pile-up. I'll hold off on opening anything new until #1664, #1673 and this one are through, and keep it to one outstanding PR at a time after that.

On the spellcheck: if #1664 ends up stalling, I can add rescan to the dictionary here instead so this one passes on its own.

@waylan

waylan commented Oct 26, 2026

Copy link
Copy Markdown
Member

Except for the spelling error, this one is ready to go. If you want, you can apply the change here, and I'll merge this. Then after its merged you could address the merge conflict in #1664. Or you can leave it as-is. Your choice.

@naaz234

naaz234 commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Done, added rescan to .spell-dict here (same line #1664 adds, so whichever lands first the other should rebase cleanly). I'll sort out the conflict in #1664 once this is merged.

@waylan
waylan merged commit 3f2c551 into Python-Markdown:master Oct 27, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

work-in-progress A partial solution. More changes will be coming.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants