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

Add line count check to avoid out of range #710

Merged
merged 3 commits into from
Apr 22, 2023
Merged

Add line count check to avoid out of range #710

merged 3 commits into from
Apr 22, 2023

Conversation

valterc
Copy link
Contributor

@valterc valterc commented Apr 18, 2023

IndexOutOfRangeException is thrown in StringLineGroup when calling Iterator.NextChar

public char NextChar()

The exception occurs when trying to advance to the next line when the StringLineGroup is at capacity (capacity == line_count)

_currentSlice = _lines.Lines[SliceIndex];

This exception is kind of an edge case and does not happen often because LeafBlock and other parts that use the StringLineGroup provision some extra capacity in advance and when capacity is needed it is increased by a factor of 2, so it's not very common that the needed capacity matches the line count.

Copy link
Collaborator

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you hit this edge-case with any real-world input while parsing?

Looks like some tests aren't happy with this change, mind looking at those?

src/Markdig.Tests/TestStringLineGroup.cs Outdated Show resolved Hide resolved
@valterc
Copy link
Contributor Author

valterc commented Apr 18, 2023

My bad, I didn't really run all the tests locally.

I did hit this case when I was parsing a yaml front matter which contained exactly 8 lines inside of it. Looking through the code, LeafBlock reserves 4 lines up-front:

Lines = new StringLineGroup(4, ProcessInlines);

And then the StringLineGroup increases the capacity to 8, when using the CharIterator this bug comes up.

In my case I was building a subclass of TextReader based on the CharIterator (avoiding allocations) to use in some other frameworks.

I've added a sort of end-to-end test in TestYamlFrontMatterExtension, let me know if that is OK or should I add in the RoundTrip tests. Without the fix, the test fails with exception:
image

Example markdown that breaks the CharIterator:

---
key1: value1
key2: value2
key3: value3
key4: value4
key5: value5
key6: value6
key7: value7
key8: value8
---

# Content

@coveralls
Copy link

coveralls commented Apr 18, 2023

Pull Request Test Coverage Report for Build 4734877669

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 48 of 49 (97.96%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 93.323%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Markdig.Tests/TestYamlFrontMatterExtension.cs 15 16 93.75%
Totals Coverage Status
Change from base Build 4279648756: 0.03%
Covered Lines: 25978
Relevant Lines: 27233

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants