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 wrong disregarding of not closed markup, such as lone < #679

Merged
merged 6 commits into from
Nov 15, 2023

Conversation

Mingun
Copy link
Collaborator

@Mingun Mingun commented Nov 12, 2023

The 1st commit is a preparation step.
The 2nd adds a regression test.
The 3rd commit fixes #622.

The bug was in two parts:

  • the first one is incorrect returning of Event::Eof in read_until_close! macro. We are inside a markup at this moment, so we cannot return EOF event.
  • the second is an incorrect transition to OpenedTag state from ClosedTag state. It is assumed that in OpenedTag we already have seen and have consumed the < character, but because read_byte_until could return bytes without seeing the searched byte we could go to OpenedTag state when < was not seen. In that case the first part of a bug have masked problem in most cases.

I also noticed other problems near to fixed code, so the commits 4 and 5 fixes them: read_element and read_bang_element are called inside of a tag or bang markup (i. e. after seeing < character), so they must return a corresponding event or an error. The last commit just a refactoring of that fixes, that also removes dead code arised due to a typo.

@Mingun Mingun added the bug label Nov 12, 2023
@Mingun Mingun requested a review from dralley November 12, 2023 17:39
@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2023

Codecov Report

Merging #679 (6d5afed) into master (bbc7bda) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #679      +/-   ##
==========================================
- Coverage   65.16%   65.16%   -0.01%     
==========================================
  Files          38       38              
  Lines       17851    17845       -6     
==========================================
- Hits        11633    11628       -5     
+ Misses       6218     6217       -1     
Flag Coverage Δ
unittests 65.16% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/reader/async_tokio.rs 56.94% <ø> (ø)
src/reader/buffered_reader.rs 85.57% <100.00%> (+0.06%) ⬆️
src/reader/mod.rs 95.12% <100.00%> (+0.01%) ⬆️
src/reader/slice_reader.rs 100.00% <100.00%> (ø)
src/reader/state.rs 98.06% <100.00%> (-1.29%) ⬇️

... and 3 files with indirect coverage changes

src/reader/mod.rs Outdated Show resolved Hide resolved
} else {
Ok(Some(&buf[start..]))
}
Ok((&buf[start..], done))
Copy link
Collaborator

Choose a reason for hiding this comment

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

done ought to be renamed to found

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, all that code will be removed soon, because I rewrite it, so not necessary to polish it. That changes was made only because I plan to make tests first to know that rewriting did not break anything

src/reader/mod.rs Outdated Show resolved Hide resolved
Mingun and others added 6 commits November 15, 2023 20:43
…te was found or not

Co-authored-by: Daniel Alley <dalley@redhat.com>
…, because we inside a tag

An opened `<` character was already read when we call read_element,
so we MUST to find `>` or return a syntax error
… None, because we inside a markup

An opened `<` character was already read when we call read_bang_element,
so we MUST to find `>` or return a syntax error
`read` in read_bang_element was at least 1, but then branch in any case
could be reachable only if .read_buf() returns empty array, but in that
case we have already exited with the correct error
@Mingun Mingun merged commit 64c4249 into tafia:master Nov 15, 2023
6 checks passed
@Mingun Mingun deleted the fix-622 branch November 15, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid close tag is parsed successfully
3 participants