From af55ad92381d8b7bbb1accf5511542190994046e Mon Sep 17 00:00:00 2001 From: FadeHack Date: Sun, 23 Aug 2026 05:30:56 +0530 Subject: [PATCH] Keep question marks in processing instruction data The PI after state was throwing away the '?' that got it there whenever that '?' turned out not to be the start of a '?>'. So a processing instruction like came out with the href pointing at the wrong resource, and no error was raised. XML 1.0 section 2.6 says a processing instruction runs up to the first '?>', so a '?' anywhere else is ordinary data and belongs in the data string. That is what libxml2 and Python's minidom do too. The same state had a second problem. On any other character it pushed that character and stayed in the PI after state, so the next '>' ended the instruction even though the character before it was not a '?'. That made c?> stop early with data "ab". It now goes back to the PI data state, which is what the XML5 draft says to do anyway. At EOF the pending '?' was dropped for the same reason, so an unterminated instruction lost its last character. It is kept now. One xml5lib test, "PI tag with char in PiAfter state", expects the old output. It follows the XML5 draft, whose PI after state appends the '?' when it sees another '?' but silently drops it for anything else. That asymmetry looks like an oversight in the draft rather than something intended, so I skipped that test with a note instead of matching it. Fixes #774 --- rcdom/tests/xml-tokenizer.rs | 25 +++++++-- xml5ever/src/tokenizer/mod.rs | 99 +++++++++++++++++++++++++++++++++-- 2 files changed, 116 insertions(+), 8 deletions(-) diff --git a/rcdom/tests/xml-tokenizer.rs b/rcdom/tests/xml-tokenizer.rs index 150dc6ed..af1e1797 100644 --- a/rcdom/tests/xml-tokenizer.rs +++ b/rcdom/tests/xml-tokenizer.rs @@ -283,10 +283,26 @@ fn json_to_tokens(js: &Value, exact_errors: bool) -> Vec { sink.get_tokens() } -fn mk_xml_test(name: String, input: String, expect: Value, opts: XmlTokenizerOpts) -> Test { +/// Descriptions of xml5lib tests that we do not run. +/// +/// The XML5 draft's "PI after state" throws away a '?' that turns out not to be +/// the start of a '?>', instead of treating it as ordinary data. XML 1.0 §2.6 +/// says a processing instruction runs up to the first '?>', so such a '?' +/// belongs in the data, and that is what libxml2 and other parsers produce. +/// This test bakes in the draft's behaviour, so it disagrees with us now that +/// we keep the character. See https://github.com/servo/html5ever/issues/774 +const IGNORED_TESTS: &[&str] = &["PI tag with char in PiAfter state"]; + +fn mk_xml_test( + name: String, + skip: bool, + input: String, + expect: Value, + opts: XmlTokenizerOpts, +) -> Test { Test { name, - skip: false, + skip, test: Box::new(move || { // Split up the input at different points to test incremental tokenization. let insplits = splits(&input, 3); @@ -308,7 +324,9 @@ fn mk_xml_test(name: String, input: String, expect: Value, opts: XmlTokenizerOpt fn mk_xml_tests(tests: &mut Vec, filename: &str, js: &Value) { let input: &str = &js.find("input").get_str(); let expect = js.find("output"); - let desc = format!("tok: {}: {}", filename, js.find("description").get_str()); + let description = js.find("description").get_str(); + let skip = IGNORED_TESTS.contains(&&*description); + let desc = format!("tok: {filename}: {description}"); // Some tests want to start in a state other than Data. let state_overrides = vec![None]; @@ -326,6 +344,7 @@ fn mk_xml_tests(tests: &mut Vec, filename: &str, js: &Value) { tests.push(mk_xml_test( newdesc, + skip, String::from(input), expect.clone(), XmlTokenizerOpts { diff --git a/xml5ever/src/tokenizer/mod.rs b/xml5ever/src/tokenizer/mod.rs index 0d87e637..5f9d14c5 100644 --- a/xml5ever/src/tokenizer/mod.rs +++ b/xml5ever/src/tokenizer/mod.rs @@ -742,8 +742,11 @@ impl XmlTokenizer { XmlState::PiAfter => loop { match get_char!(self, input) { '>' => go!(self: emit_pi Data), - '?' => go!(self: to XmlState::PiAfter), - cl => go!(self: push_pi_data cl), + // The '?' that got us here is not part of a '?>', so it is + // ordinary data. The one we just read may still close the + // processing instruction, so stay in this state. + '?' => go!(self: push_pi_data '?'; to XmlState::PiAfter), + cl => go!(self: push_pi_data '?'; push_pi_data cl; to XmlState::PiData), } }, //§ markup-declaration-state @@ -1196,7 +1199,9 @@ impl XmlTokenizer { go!(self: error_eof; to XmlState::Data) }, XmlState::Pi => go!(self: error_eof; to XmlState::BogusComment), - XmlState::PiTargetAfter | XmlState::PiAfter => go!(self: reconsume XmlState::PiData), + XmlState::PiTargetAfter => go!(self: reconsume XmlState::PiData), + // The '?' we consumed to get here never became a '?>', so keep it. + XmlState::PiAfter => go!(self: push_pi_data '?'; reconsume XmlState::PiData), XmlState::MarkupDecl => go!(self: error_eof; to XmlState::BogusComment), XmlState::TagName | XmlState::TagAttrNameBefore @@ -1319,9 +1324,93 @@ impl XmlTokenizer { #[cfg(test)] mod test { - use super::process_qname; - use crate::tendril::SliceExt; + use super::{process_qname, ProcessResult, Token, TokenSink, XmlTokenizer}; + use crate::tendril::{SliceExt, StrTendril}; use crate::{LocalName, Prefix}; + use markup5ever::buffer_queue::BufferQueue; + use std::cell::RefCell; + + struct PiCollector { + pis: RefCell>, + } + + impl TokenSink for PiCollector { + type Handle = (); + + fn process_token(&self, token: Token) -> ProcessResult<()> { + if let Token::ProcessingInstruction(pi) = token { + self.pis + .borrow_mut() + .push((pi.target.to_string(), pi.data.to_string())); + } + ProcessResult::Continue + } + } + + fn tokenize_pis(input: &str) -> Vec<(String, String)> { + let sink = PiCollector { + pis: RefCell::new(Vec::new()), + }; + let queue = BufferQueue::default(); + queue.push_back(StrTendril::from(input)); + let tokenizer = XmlTokenizer::new(sink, Default::default()); + let _ = tokenizer.feed(&queue); + tokenizer.end(); + tokenizer.sink.pis.into_inner() + } + + #[test] + fn pi_data_keeps_question_marks() { + assert_eq!( + tokenize_pis(r#""#), + vec![( + "xml-stylesheet".to_owned(), + r#"href="style.xsl?v=2""#.to_owned() + )] + ); + + assert_eq!( + tokenize_pis(""), + vec![("target".to_owned(), "a?b?c".to_owned())] + ); + + // A run of question marks only ends the instruction when one of them + // is followed by '>'. + assert_eq!( + tokenize_pis(""), + vec![("target".to_owned(), "a??".to_owned())] + ); + } + + #[test] + fn pi_only_ends_on_question_mark_gt() { + // The '>' here is data, because the character before it is not a '?'. + assert_eq!( + tokenize_pis("c?>"), + vec![("target".to_owned(), "a?b>c".to_owned())] + ); + } + + #[test] + fn pi_without_data_is_empty() { + assert_eq!( + tokenize_pis(""), + vec![("target".to_owned(), String::new())] + ); + + assert_eq!( + tokenize_pis(""), + vec![("target".to_owned(), String::new())] + ); + } + + #[test] + fn unterminated_pi_keeps_trailing_question_mark() { + assert_eq!( + tokenize_pis("