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.

Sign up for GitHub

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

Jump to bottom

mdcat panics on inline markup in image descriptions #285

Closed
cherryblossom000 opened this issue Jul 25, 2024 · 1 comment
Closed

mdcat panics on inline markup in image descriptions #285

cherryblossom000 opened this issue Jul 25, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@cherryblossom000
Copy link
Contributor

Markdown:

![`a`](https://example.com)

Crash report:

"name" = "mdcat"
"operating_system" = "Mac OS 14.5.0 [64-bit]"
"crate_version" = "2.1.2"
"explanation" = """
Panic occurred in file '/private/tmp/nix-build-mdcat-2.1.2.drv-0/source/pulldown-cmark-mdcat/src/render.rs' at line 756
"""
"cause" = "Event Code(Borrowed(\"a\")) impossible in state Stacked(StateStack { top_level: TopLevelAttrs { margin_before: Margin }, states: [Inline(InlineText, InlineAttrs { style: Style { fg: None, bg: None, underline: None, effects: Effects() }, indent: 0 })] }, RenderedImage)"
"method" = "Panic"
"backtrace" = """

   0: 0x1035211c0 - core::panicking::panic_fmt::h5e7349beebf2ad6e
   1: 0x10312f21c - pulldown_cmark_mdcat::render::write_event::h4fd0628c0060b7ba
   2: 0x10314dd50 - mdcat::process_file::h2c61c8fc78c3f1df
   3: 0x103159658 - core::iter::traits::iterator::Iterator::try_fold::h01e9f86fbe1110e5
   4: 0x10315876c - mdcat::main::h958474e82a8f2ceb
   5: 0x103163084 - std::sys_common::backtrace::__rust_begin_short_backtrace::h279725f565e6ca66
   6: 0x103153e78 - _main"""
@swsnr
Copy link
Owner

swsnr commented Jul 26, 2024 •
edited
Loading

Indeed, according to the common mark spec image descriptions follow the same rules as link texts, meaning mdcat definitely shouldn't crash here.

That said I don't have much time to fix this currently. I'd appreciate a pull request.

The same issue was already reported in #194

@swsnr swsnr added the bug Something isn't working label Jul 26, 2024
@swsnr swsnr changed the title mdcat Crash Report mdcat crashes on inline markup in image descriptions Jul 26, 2024
@swsnr swsnr changed the title mdcat crashes on inline markup in image descriptions mdcat panics on inline markup in image descriptions Jul 26, 2024
pengjiz added a commit to pengjiz/mdcat that referenced this issue Aug 2, 2024
@pengjiz
Ignore all descriptions for rendered images
79e2034 
CommonMark allows all inline elements in the image description part[1],
not only plain text. So in this commit we change to ignore all such
elements instead of panicking.

To correctly handle nested images inside the image description, here we
push dummy 'RenderedImage' states to the stack. An alternative solution
would be maintaining a counter for nested image level and checking the
counter when dealing with end of image events, but that feels less
tasteful to me.

Note that this is a bit more lenient than the spec because it allows
block elements as well. However, this behaviour aligns with the upstream
library[2], and the more correct version is a bit tedious to implement
because we have to list all inline tags ourselves.

Closes swsnrGH-285.

[1] https://spec.commonmark.org/0.31.2/#images
[2] https://docs.rs/pulldown-cmark/0.9.6/src/pulldown_cmark/html.rs.html#377-399
@pengjiz pengjiz mentioned this issue Aug 2, 2024
Merged
pengjiz added a commit to pengjiz/mdcat that referenced this issue Aug 3, 2024
@pengjiz
Add tests for markups in image descriptions
8107719 
Those cases are actually covered in the 'commonmark-spec' test suite.
However, those tests use non-existent images so they are not checked for
rendered images. As such, in this commit we add a new case in
'samples/images.md', primarily to test markups in descriptions of
rendered images.

Re: swsnrGH-194, swsnrGH-285.
swsnr pushed a commit to pengjiz/mdcat that referenced this issue Aug 4, 2024
@pengjiz @swsnr
Ignore all descriptions for rendered images
392fd94 
CommonMark allows all inline elements in the image description part[1],
not only plain text. So in this commit we change to ignore all such
elements instead of panicking.

To correctly handle nested images inside the image description, here we
push dummy 'RenderedImage' states to the stack. An alternative solution
would be maintaining a counter for nested image level and checking the
counter when dealing with end of image events, but that feels less
tasteful to me.

Note that this is a bit more lenient than the spec because it allows
block elements as well. However, this behaviour aligns with the upstream
library[2], and the more correct version is a bit tedious to implement
because we have to list all inline tags ourselves.

Closes swsnrGH-285.

[1] https://spec.commonmark.org/0.31.2/#images
[2] https://docs.rs/pulldown-cmark/0.9.6/src/pulldown_cmark/html.rs.html#377-399
swsnr pushed a commit to pengjiz/mdcat that referenced this issue Aug 4, 2024
@pengjiz @swsnr
Add tests for markups in image descriptions
c996f55 
Those cases are actually covered in the 'commonmark-spec' test suite.
However, those tests use non-existent images so they are not checked for
rendered images. As such, in this commit we add a new case in
'samples/images.md', primarily to test markups in descriptions of
rendered images.

Re: swsnrGH-194, swsnrGH-285.
@swsnr swsnr closed this as completed in 3ed060b Aug 4, 2024
swsnr added a commit that referenced this issue Aug 4, 2024
@swsnr
Merge pull request #287 from pengjiz/fix/gh-285
f67b44d 
Ignore all descriptions for rendered images

Closes GH-285
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@swsnr @cherryblossom000