Skip to content

core: Upgrade quick_xml and improve error handling when parsing xml #21107

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

Open
SuchAFuriousDeath wants to merge 1 commit into
base: master
Choose a base branch
from
Open

core: Upgrade quick_xml and improve error handling when parsing xml #21107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ num-traits = "0.2.19"
serde = "1.0.228"
thiserror = "2.0.17"
url = "2.5.7"
quick-xml = "0.37.5"
quick-xml = "0.38"
regress = { git = "https://github.com/ruffle-rs/regras3", rev = "5fcb02513c5ab4e00df4346459f5a8d0521d8fed" }
# Make sure to match wasm-bindgen-cli version to this everywhere.
wasm-bindgen = "=0.2.101"
Expand Down
233 changes: 160 additions & 73 deletions core/src/avm1/globals/xml.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,91 +140,178 @@
) -> Result<(), quick_xml::Error> {
let data_utf8 = data.to_utf8_lossy();
let mut parser = Reader::from_str(&data_utf8);
// Flash is lenient with lone ampersands in text content
parser.config_mut().allow_dangling_amp = true;
let mut open_tags = vec![self.root()];
// Buffer for accumulating text content across Text, CData, and GeneralRef events.
// quick-xml emits these as separate events, but Flash joins them into a single text node.
let mut text_buffer: Vec<u8> = Vec::new();

self.0.status.set(XmlStatus::NoError);

loop {
let event = parser.read_event().map_err(|error| {
self.0.status.set(match error {
quick_xml::Error::Syntax(_)
| quick_xml::Error::InvalidAttr(AttrError::ExpectedEq(_))
| quick_xml::Error::InvalidAttr(AttrError::Duplicated(_, _)) => {
XmlStatus::ElementMalformed
let event = match parser.read_event() {
Ok(event) => event,
Err(error) => {

Check warning on line 155 in core/src/avm1/globals/xml.rs

View workflow job for this annotation

GitHub Actions / Coverage Report

Coverage 

Uncovered line (155)
// Flush any buffered text before returning on error
if !text_buffer.is_empty() {
Self::handle_text_cdata(&text_buffer, ignore_white, &open_tags, activation);
text_buffer.clear();

Check warning on line 159 in core/src/avm1/globals/xml.rs

View workflow job for this annotation

GitHub Actions / Coverage Report

Coverage 

Uncovered lines (157–159)
}
quick_xml::Error::IllFormed(
IllFormedError::MismatchedEndTag { .. }
| IllFormedError::UnmatchedEndTag { .. },
) => XmlStatus::MismatchedEnd,
quick_xml::Error::IllFormed(IllFormedError::MissingDeclVersion(_)) => {
XmlStatus::DeclNotTerminated
}
quick_xml::Error::InvalidAttr(AttrError::UnquotedValue(_)) => {
XmlStatus::AttributeNotTerminated
}
_ => XmlStatus::OutOfMemory,
// Not accounted for:
// quick_xml::Error::UnexpectedToken(_)
// quick_xml::Error::UnexpectedBang
// quick_xml::Error::TextNotFound
// quick_xml::Error::EscapeError(_)
});
error
})?;
self.0.status.set(match error {

Check warning on line 161 in core/src/avm1/globals/xml.rs

View workflow job for this annotation

GitHub Actions / Coverage Report

Coverage 

Uncovered line (161)
quick_xml::Error::Syntax(_)
| quick_xml::Error::InvalidAttr(AttrError::ExpectedEq(_))
| quick_xml::Error::InvalidAttr(AttrError::Duplicated(_, _)) => {
XmlStatus::ElementMalformed

Check warning on line 165 in core/src/avm1/globals/xml.rs

View workflow job for this annotation

GitHub Actions / Coverage Report

Coverage 

Uncovered line (165)
}
quick_xml::Error::IllFormed(
IllFormedError::MismatchedEndTag { .. }
| IllFormedError::UnmatchedEndTag { .. },
) => XmlStatus::MismatchedEnd,

Check warning on line 170 in core/src/avm1/globals/xml.rs

View workflow job for this annotation

GitHub Actions / Coverage Report

Coverage 

Uncovered line (170)
quick_xml::Error::IllFormed(IllFormedError::MissingDeclVersion(_)) => {
XmlStatus::DeclNotTerminated

Check warning on line 172 in core/src/avm1/globals/xml.rs

View workflow job for this annotation

GitHub Actions / Coverage Report

Coverage 

Uncovered line (172)
}
quick_xml::Error::InvalidAttr(AttrError::UnquotedValue(_)) => {
XmlStatus::AttributeNotTerminated
}
_ => XmlStatus::OutOfMemory,
// Not accounted for:
// quick_xml::Error::UnexpectedToken(_)
// quick_xml::Error::UnexpectedBang
// quick_xml::Error::TextNotFound
// quick_xml::Error::EscapeError(_)
});
return Err(error);
}
};

match event {
Event::Start(bs) => {
let child =
XmlNode::from_start_event(activation, bs, self.id_map(), parser.decoder())?;
open_tags
.last()
.unwrap()
.append_child(activation.gc(), child);
open_tags.push(child);
}
Event::Empty(bs) => {
let child =
XmlNode::from_start_event(activation, bs, self.id_map(), parser.decoder())?;
open_tags
.last()
.unwrap()
.append_child(activation.gc(), child);
}
Event::End(_) => {
open_tags.pop();
}
// Text and GeneralRef events are joined into a single text node,
// since GeneralRef is now emitted separately for entity references.
Event::Text(bt) => {
Self::handle_text_cdata(
custom_unescape(&bt.into_inner(), parser.decoder())?.as_bytes(),
ignore_white,
&open_tags,
activation,
);
let unescaped = custom_unescape(&bt.into_inner(), parser.decoder())?;
text_buffer.extend(unescaped.as_bytes());
}
Event::CData(bt) => {
// This is already unescaped
Self::handle_text_cdata(&bt.into_inner(), ignore_white, &open_tags, activation);
}
Event::Decl(bd) => {
let mut xml_decl = WString::from_buf(b"<?".to_vec());
xml_decl.push_str(WStr::from_units(&*bd));
xml_decl.push_str(WStr::from_units(b"?>"));
let xml_decl = Some(AvmString::new(activation.gc(), xml_decl));
unlock!(Gc::write(activation.gc(), self.0), XmlData, xml_decl).set(xml_decl);
Event::GeneralRef(br) => {
// Resolve character references: numeric (&#229;) and named (&amp;)
match br.resolve_char_ref() {
Ok(Some(ch)) => {
// Numeric ref resolved to a character
let mut buf = [0u8; 4];
text_buffer.extend(ch.encode_utf8(&mut buf).as_bytes());
}
Ok(None) | Err(_) => {
// Named entity ref - try to unescape standard XML entities
let inner = br.into_inner();
let mut entity = Vec::with_capacity(inner.len() + 2);
entity.push(b'&');
entity.extend_from_slice(inner.as_ref());
entity.push(b';');
match quick_xml::escape::unescape(
std::str::from_utf8(&entity).unwrap_or_default(),
) {
Ok(unescaped) => text_buffer.extend(unescaped.as_bytes()),
Err(_) => text_buffer.extend(&entity),
}
}
}
}
Event::DocType(bt) => {
// TODO: `quick-xml` is case-insensitive for DOCTYPE declarations,
// but it doesn't expose the whole tag, only the inner portion of it.
// Flash is also case-insensitive for DOCTYPE declarations. However,
// the `.docTypeDecl` property preserves the original case.
let mut doctype = WString::from_buf(b"<!DOCTYPE ".to_vec());
doctype.push_str(WStr::from_units(&*bt.escape_ascii().collect::<Vec<_>>()));
doctype.push_byte(b'>');
let doctype = Some(AvmString::new(activation.gc(), doctype));
unlock!(Gc::write(activation.gc(), self.0), XmlData, doctype).set(doctype);

// All other events: flush text buffer first, then handle
_ => {
if !text_buffer.is_empty() {
Self::handle_text_cdata(&text_buffer, ignore_white, &open_tags, activation);
text_buffer.clear();
}

match event {
Event::Start(bs) => {
let child = XmlNode::from_start_event(
activation,
bs,
self.id_map(),
parser.decoder(),
)?;
open_tags
.last()
.unwrap()
.append_child(activation.gc(), child);
open_tags.push(child);
}
Event::Empty(bs) => {
let child = XmlNode::from_start_event(
activation,
bs,
self.id_map(),
parser.decoder(),
)?;
open_tags
.last()
.unwrap()
.append_child(activation.gc(), child);
}
Event::End(_) => {
open_tags.pop();
}
Event::CData(bt) => {
// CDATA is kept as a separate text node (not joined with Text)
Self::handle_text_cdata(
&bt.into_inner(),
ignore_white,
&open_tags,
activation,
);
}
Event::Decl(bd) => {
let xml_decl: WString = b"<?"
.iter()
.chain(bd.iter())
.chain(b"?>")
.map(|&b| b as u16)
.collect();
unlock!(Gc::write(activation.gc(), self.0), XmlData, xml_decl)
.set(Some(AvmString::new(activation.gc(), xml_decl)));
}
Event::PI(bp) => {
// Per XML spec (https://www.w3.org/TR/xml11/#sec-pi), PITarget names
// matching [Xx][Mm][Ll] are reserved and not valid PI targets.
// quick-xml only recognizes lowercase `<?xml` as an XML declaration,
// treating `<?XML` or `<?XmL` as processing instructions.
// Flash is lenient and treats any case variant as an XML declaration,
// so we check for reserved targets and handle them like declarations.
if matches!(bp.target(), [b'X' | b'x', b'M' | b'm', b'L' | b'l']) {
let xml_decl: WString = b"<?"
.iter()
.chain(bp.iter())
.chain(b"?>")
.map(|&b| b as u16)
.collect();
unlock!(Gc::write(activation.gc(), self.0), XmlData, xml_decl)
.set(Some(AvmString::new(activation.gc(), xml_decl)));
}
// Other PIs are ignored by Flash in avm1
}
Event::DocType(bt) => {
// TODO: `quick-xml` is case-insensitive for DOCTYPE declarations,
// but it doesn't expose the whole tag, only the inner portion of it.
// Flash is also case-insensitive for DOCTYPE declarations. However,
// the `.docTypeDecl` property preserves the original case.
let mut doctype = WString::from_buf(b"<!DOCTYPE ".to_vec());
doctype.push_str(WStr::from_units(
&*bt.escape_ascii().collect::<Vec<_>>(),
));
doctype.push_byte(b'>');
let doctype = Some(AvmString::new(activation.gc(), doctype));
unlock!(Gc::write(activation.gc(), self.0), XmlData, doctype)
.set(doctype);
}
Event::Eof => break,
// Flash ignores XML comments in avm1
Event::Comment(_) => {}
// Text and GeneralRef are handled in the outer match
Event::Text(_) | Event::GeneralRef(_) => unreachable!(),
}
}
Event::Eof => break,
_ => {}
}
}

Expand Down
Loading
Loading