diff --git a/rsvg/src/xml/mod.rs b/rsvg/src/xml/mod.rs index 8eb8e1c37e7c28c7e7ec37339c1626cb541fd4eb..897f22ce38ac32da8dc7bb36622d379da09285f6 100644 --- a/rsvg/src/xml/mod.rs +++ b/rsvg/src/xml/mod.rs @@ -8,7 +8,7 @@ use gio::{ use glib::object::Cast; use markup5ever::{ExpandedName, LocalName, Namespace, QualName, expanded_name, local_name, ns}; use std::cell::RefCell; -use std::collections::HashMap; +use std::collections::{HashMap, hash_map::Entry}; use std::rc::Rc; use std::str; use std::string::ToString; @@ -32,14 +32,13 @@ use crate::session::Session; use crate::style::StyleType; use crate::url_resolver::AllowedUrl; -use xml2_load::Xml2Parser; +use xml2::xmlEntityPtr; +use xml2_load::{EntityData, Xml2Parser, XmlEntity}; mod attributes; mod xml2; mod xml2_load; -use xml2::xmlEntityPtr; - pub use attributes::Attributes; #[derive(Clone)] @@ -74,31 +73,6 @@ struct XIncludeContext { need_fallback: bool, } -unsafe extern "C" { - // The original function takes an xmlNodePtr, but that is compatible - // with xmlEntityPtr for the purposes of this function. - fn xmlFreeNode(node: xmlEntityPtr); -} - -/// This is to hold an xmlEntityPtr from libxml2; we just hold an opaque pointer -/// that is freed in impl Drop. -struct XmlEntity(xmlEntityPtr); - -impl Drop for XmlEntity { - fn drop(&mut self) { - unsafe { - // Even though we are freeing an xmlEntityPtr, historically the code has always - // used xmlFreeNode() because that function actually does allow freeing entities. - // - // See https://gitlab.gnome.org/GNOME/libxml2/-/issues/731 - // for a possible memory leak on older versions of libxml2 when using - // xmlFreeNode() instead of xmlFreeEntity() - the latter just became public - // in librsvg-2.12.0. - xmlFreeNode(self.0); - } - } -} - // Creates an ExpandedName from the XInclude namespace and a local_name // // The markup5ever crate doesn't have built-in namespaces for XInclude, @@ -127,14 +101,6 @@ struct XmlStateInner { load_limiter: LoadingDepthLimiter, context_stack: Vec, current_node: Option, - - // Note that neither XmlStateInner nor Xmlstate implement Drop. - // - // An XmlState is finally consumed in XmlState::build_document(), and that - // function is responsible for freeing all the XmlEntityPtr from this field. - // - // (The structs cannot impl Drop because build_document() - // destructures and consumes them at the same time.) entities: HashMap, } @@ -367,12 +333,21 @@ impl XmlState { .map(|entity| entity.0) } - pub fn entity_insert(&self, entity_name: &str, entity: xmlEntityPtr) { + pub fn entity_insert(&self, entity_data: EntityData) { let mut inner = self.inner.borrow_mut(); - inner - .entities - .insert(entity_name.to_string(), XmlEntity(entity)); + let name = entity_data.name(); + + match inner.entities.entry(name.to_string()) { + Entry::Occupied(_) => { + // Ignore the case where an entity is declared twice with the + // same name. + } + + Entry::Vacant(v) => { + v.insert(entity_data.into_xml_entity()); + } + } } fn element_creation_start_element(&self, name: &QualName, attrs: Attributes) -> Context { diff --git a/rsvg/src/xml/xml2.rs b/rsvg/src/xml/xml2.rs index 0012cedb718b99d5f97733d606172846cc8e1780..84947e37bc382494eb6c0b9f08fe78dfec3e9243 100644 --- a/rsvg/src/xml/xml2.rs +++ b/rsvg/src/xml/xml2.rs @@ -211,4 +211,8 @@ unsafe extern "C" { system_id: *const libc::c_char, content: *const libc::c_char, ) -> xmlEntityPtr; + + // The original function takes an xmlNodePtr, but that is compatible + // with xmlEntityPtr for the purposes of this function. + pub fn xmlFreeNode(node: xmlEntityPtr); } diff --git a/rsvg/src/xml/xml2_load.rs b/rsvg/src/xml/xml2_load.rs index eb30f5fc492cfccad99612de9fc08615a2498513..9837ee59788f8182f33c8ce1c1e737085a90ab9b 100644 --- a/rsvg/src/xml/xml2_load.rs +++ b/rsvg/src/xml/xml2_load.rs @@ -135,6 +135,60 @@ unsafe extern "C" fn sax_get_entity_cb( .unwrap_or(ptr::null_mut()) } +/// This is to hold an xmlEntityPtr from libxml2; we just hold an opaque pointer +/// that is freed in impl Drop. +pub struct XmlEntity(pub xmlEntityPtr); + +impl Drop for XmlEntity { + fn drop(&mut self) { + unsafe { + // Even though we are freeing an xmlEntityPtr, historically the code has always + // used xmlFreeNode() because that function actually does allow freeing entities. + // + // See https://gitlab.gnome.org/GNOME/libxml2/-/issues/731 + // for a possible memory leak on older versions of libxml2 when using + // xmlFreeNode() instead of xmlFreeEntity() - the latter just became public + // in librsvg-2.12.0. + xmlFreeNode(self.0); + } + } +} + +/// Temporary wrapper for libxml2 entity data, which can be turned into an actual XmlEntity later. +pub struct EntityData { + name: *const libc::c_char, + type_: libc::c_int, + content: *const libc::c_char, +} + +impl EntityData { + pub fn into_xml_entity(self) -> XmlEntity { + let EntityData { + name, + type_, + content, + } = self; + + let entity = unsafe { + xmlNewEntity( + ptr::null_mut(), + name, + type_, + ptr::null(), + ptr::null(), + content, + ) + }; + assert!(!entity.is_null()); + + XmlEntity(entity) + } + + pub fn name(&self) -> &str { + unsafe { utf8_cstr(self.name) } + } +} + unsafe extern "C" fn sax_entity_decl_cb( user_data: *mut libc::c_void, name: *const libc::c_char, @@ -154,18 +208,13 @@ unsafe extern "C" fn sax_entity_decl_cb( return; } - let entity = xmlNewEntity( - ptr::null_mut(), + let entity_data = EntityData { name, type_, - ptr::null(), - ptr::null(), content, - ); - assert!(!entity.is_null()); + }; - let name = utf8_cstr(name); - xml2_parser.state.entity_insert(name, entity); + xml2_parser.state.entity_insert(entity_data); } unsafe extern "C" fn sax_unparsed_entity_decl_cb(