Skip to content
This repository has been archived by the owner on Mar 25, 2024. It is now read-only.

unsafe-yaml-backed serde-yaml panics on lexer errors #293

Closed
Mrmaxmeier opened this issue Jul 29, 2022 · 1 comment · Fixed by #295
Closed

unsafe-yaml-backed serde-yaml panics on lexer errors #293

Mrmaxmeier opened this issue Jul 29, 2022 · 1 comment · Fixed by #295

Comments

@Mrmaxmeier
Copy link

Mrmaxmeier commented Jul 29, 2022

YAML payloads with invalid syntax can trigger this branch: sys::YAML_NO_EVENT => unreachable!():

let _ = serde_yaml::from_str::<serde_yaml::Value>(">\n@");
thread 'main' panicked at 'internal error: entered unreachable code', src/libyaml/parser.rs:142:31
stack backtrace:
   0: rust_begin_unwind
             [...]
   3: serde_yaml::libyaml::parser::convert_event
             at ./src/libyaml/parser.rs:142:31
   4: serde_yaml::libyaml::parser::Parser::next
             at ./src/libyaml/parser.rs:91:23

This happens because libyaml's yaml_parser_parse seems to expect the caller to handle parser->error states:
https://github.com/dtolnay/unsafe-libyaml/blob/5d421b89e946d70b52a1b8cf026d97244e182f39/src/parser.rs#L75

This patch fixes the panic, but I'm not sure if it's the proper fix:

diff --git a/src/libyaml/parser.rs b/src/libyaml/parser.rs
index 9f43ced..ae8f375 100644
--- a/src/libyaml/parser.rs
+++ b/src/libyaml/parser.rs
@@ -84,6 +84,9 @@ impl<'input> Parser<'input> {
         let mut event = MaybeUninit::<sys::yaml_event_t>::uninit();
         unsafe {
             let parser = addr_of_mut!((*self.pin.ptr).sys);
+            if (*parser).error != sys::YAML_NO_ERROR {
+                return Err(Error::parse_error(parser));
+            }
             let event = event.as_mut_ptr();
             if sys::yaml_parser_parse(parser, event).fail {
                 return Err(Error::parse_error(parser));
@dtolnay
Copy link
Owner

dtolnay commented Jul 29, 2022

Thanks! Fixed in 0.9.1.

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

Successfully merging a pull request may close this issue.

2 participants