Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't set prefix for li element outside list #45

Merged
merged 1 commit into from
Feb 26, 2022

Conversation

wcalandro
Copy link
Contributor

Our internal fuzzer found that this package would timeout when handling the following testcase: testcase-timeout.txt. It appears to be caused by the assumption that, if a <li> doesn't have a <ul> as a parent, the parent must be a <ol>. However, with malformed HTML this may not be the case. This issue is remedied by also checking if the parent is a <ol>, and if the parent is neither a <ul> nor a <ol> returning no list prefix.

@JohannesKaufmann
Copy link
Owner

@wcalandro Happy to merge this, since the output seems like a better solution if the list item has no ul/ol parent. I really appreciate that you took the time to create a PR 🎉

Thanks for contributing and let me know if you find anything else.


Wow, this txt file takes quite a lot of time!

Probably with a different constructed HTML that contains an ol and more than 10k li elements we get into the same range of execution time again (~60sec on my laptop).

There is definitely room for performance improvements in quite a few parts of the library. And I should definitely add some benchmarks and fuzzing. Wonder what other inputs might cause a problem...

@JohannesKaufmann JohannesKaufmann merged commit 396b6b1 into JohannesKaufmann:master Feb 26, 2022
@wcalandro wcalandro deleted the li-fix branch February 28, 2022 18:46
AnastasiaShemyakinskaya pushed a commit to anyproto/html-to-markdown that referenced this pull request Mar 14, 2023
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.

2 participants