Skip to content

fix: skip empty TightParagraph without panicking - #1128

Closed
teddytennant wants to merge 2 commits into
pulldown-cmark:mainfrom
teddytennant:fix/tasklist-strikethrough-panic-1084
Closed

teddytennant wants to merge 2 commits into
pulldown-cmark:mainfrom
teddytennant:fix/tasklist-strikethrough-panic-1084

Conversation

@teddytennant

Copy link
Copy Markdown
Contributor

Problem

Parsing certain Markdown with Options::ENABLE_TASKLISTS and Options::ENABLE_STRIKETHROUGH could panic while iterating the parser:

called `Option::unwrap()` on a `None` value
  at parse.rs (TightParagraph handling in next_event_range)

Reproducer from #1084:

let markdown_input = "* [ ] ~![=?\\*\x0c\x00\x00  \x0d* [  1=1\x00\x0d<!]:[=?\\\x0d\x0c\n* [ ] \x0d\x0c%    ";
let mut options = Options::empty();
options.insert(Options::ENABLE_TASKLISTS);
options.insert(Options::ENABLE_STRIKETHROUGH);
for _ in Parser::new_ext(markdown_input, options) {}

A related empty-TightParagraph panic was reported in #1095.

Root cause

Tight list items use ItemBody::TightParagraph nodes that intentionally emit neither Start nor End. The iterator always did tree.push() then tree.cur().unwrap() to descend into the paragraph body. When the tight paragraph had no children, cur was None and the unwrap panicked.

Fix

When encountering a TightParagraph:

  1. push() into it.
  2. If it has children, continue iteration from the first child (same as before).
  3. If it is empty, pop(), advance to the next sibling, and recurse into next_event_range so remaining events are still emitted.

This is intentionally different from returning None on an empty tight paragraph, which would end iteration early and drop later siblings or ancestor ends.

Note: a similar fix landed on branch_0.13 via #1096 / #1101, but not on main after the ParserInner refactor. This ports the correct empty-paragraph handling to current main.

Test plan

Fixes #1084

When iterating events, empty TightParagraph nodes previously called
tree.cur().unwrap() after push, which panics if the paragraph has no
children. That showed up with ENABLE_TASKLISTS + ENABLE_STRIKETHROUGH
on fuzz input (pulldown-cmark#1084) and related cases (pulldown-cmark#1095).

Skip empty tight paragraphs by popping and advancing to the next
sibling, then continue iteration, instead of unwrapping or returning
None early (which would drop remaining events).

Add regression tests for the pulldown-cmark#1084 and pulldown-cmark#1095 inputs.

Fixes pulldown-cmark#1084
Nightly (and -D warnings) rejects `use core::... u8` because the
std/core integer modules are deprecated. u8::MAX still resolves via the
primitive type's associated const once the module is not imported.

@notriddle notriddle left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please follow C-TEST and make a separate commit with the test changes. Make sure that the regression tests actually fail on main before attempting to "fix" the bug.

let parser = Parser::new_ext(s, Options::all());
for _ in parser {}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey, I copied these two tests over to the current main branch, and they don't fail.

@teddytennant

Copy link
Copy Markdown
Contributor Author

You're right, sorry about that. I confirmed the panic against v0.13.3 and assumed it still applied to main, but the tests pass there. Looks like the NUL handling work fixed this one along the way, so there's nothing left to fix here.

Closing this. I did find a separate panic that still reproduces on main, opened it as #1133 with the test in its own commit.

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.

Panic (Option::unwrap() on None) in parse.rs:2367 when Tasklists and Strikethrough are enabled

2 participants