Validate names during loading

src/error.rs

@@ -488,6 +488,8 @@ pub enum ErrorKind {
     BadImage,
     /// Has an invalid identifier.
     BadIdentifier,
+    /// Name is not a valid [`Name`](crate::Name).
+    InvalidName,
     /// Has an invalid lib.
     BadLib,
     /// Has an unexected duplicate value.
@@ -559,6 +561,7 @@ impl std::fmt::Display for ErrorKind {
             BadComponent => write!(f, "bad component"),
             BadImage => write!(f, "bad image"),
             BadIdentifier => write!(f, "an identifier must be at most 100 characters long and contain only ASCII characters in the range 0x20 to 0x7E"),
+            InvalidName => write!(f, "name is empty or contains control characters"),
             BadLib => write!(f, "bad lib"),
             UnexpectedDuplicate => write!(f, "unexpected duplicate"),
             UnexpectedMove => {

src/glyph/parse.rs

@@ -34,7 +34,7 @@ impl<'names> GlifParser<'names> {
         let mut buf = Vec::new();
         reader.trim_text(true);
 
-        start(&mut reader, &mut buf).and_then(|glyph| {
+        start(&mut reader, &mut buf, names).and_then(|glyph| {
             GlifParser { glyph, seen_identifiers: HashSet::new(), names }.parse_body(
                 &mut reader,
                 xml,
@@ -269,12 +269,8 @@ impl<'names> GlifParser<'names> {
                     return Err(ErrorKind::ComponentEmptyBase.into());
                 }
                 b"base" => {
-                    //FIXME: error if malformed
-                    let name = Name::new_raw(value);
-                    let name = match self.names.as_ref() {
-                        Some(names) => names.get(&name),
-                        None => name,
-                    };
+                    let name = Name::new(value).map_err(|_| ErrorKind::InvalidName)?;
+                    let name = self.names.as_ref().map(|n| n.get(&name)).unwrap_or(name);
                     base = Some(name);
                 }
                 b"identifier" => {
@@ -564,34 +560,37 @@ impl<'names> GlifParser<'names> {
     }
 }
 
-fn start(reader: &mut Reader<&[u8]>, buf: &mut Vec<u8>) -> Result<Glyph, GlifLoadError> {
+fn start(
+    reader: &mut Reader<&[u8]>,
+    buf: &mut Vec<u8>,
+    names: Option<&NameList>,
+) -> Result<Glyph, GlifLoadError> {
     loop {
         match reader.read_event(buf)? {
             Event::Comment(_) => (),
             Event::Decl(_decl) => (),
             Event::Start(ref start) if start.name() == b"glyph" => {
-                let mut name = String::new();
+                let mut name: Option<Name> = None;
                 let mut format: Option<GlifVersion> = None;
                 //let mut pos
                 for attr in start.attributes() {
                     let attr = attr?;
+                    let value = attr.unescaped_value()?;
+                    let value = reader.decode(&value)?;
                     // XXX: support `formatMinor`
                     match attr.key {
                         b"name" => {
-                            name = attr.unescape_and_decode_value(reader)?;
+                            let value = Name::new(value).map_err(|_| ErrorKind::InvalidName)?;
+                            name = Some(names.as_ref().map(|n| n.get(&value)).unwrap_or(value));
                         }
                         b"format" => {
-                            let value = attr.unescaped_value()?;
-                            let value = reader.decode(&value)?;
                             format = Some(value.parse()?);
                         }
                         _other => return Err(ErrorKind::UnexpectedAttribute.into()),
                     }
                 }
-                if !name.is_empty() && format.is_some() {
-                    //FIXME: error here
-                    return Ok(Glyph::new(Name::new_raw(&name), format.take().unwrap()));
-                    //return Ok(GlyphBuilder::new(Name::new_raw(&name), format.take().unwrap()));
+                if let (Some(name), Some(format)) = (name.take(), format.take()) {
+                    return Ok(Glyph::new(name, format));
                 } else {
                     return Err(ErrorKind::WrongFirstElement.into());
                 }

src/glyph/tests.rs

@@ -639,6 +639,22 @@ fn unexpected_line_after_offcurve2() {
     let _ = parse_glyph(data.as_bytes()).unwrap();
 }
 
+#[test]
+#[should_panic(expected = "InvalidName")]
+fn invalid_name() {
+    let data = "
+        <?xml version=\"1.0\" encoding=\"UTF-8\"?>
+        <glyph name=\"\x01\" format=\"2\">
+            <outline>
+                <contour>
+                    <point x=\"572\" y=\"667\"/>
+                    <point x=\"479\" y=\"714\"/>
+                </contour>
+            </outline>
+        </glyph>
+    ";
+    let _ = parse_glyph(data.as_bytes()).unwrap();
+}
 #[test]
 #[should_panic(expected = "UnexpectedPointAfterOffCurve")]
 fn unexpected_line_after_offcurve3() {

src/name.rs

@@ -2,6 +2,8 @@
 
 use std::sync::Arc;
 
+use serde::{Deserialize, Deserializer};
+
 use crate::error::NamingError;
 
 /// A name used to identify a [`Glyph`] or a [`Layer`].
@@ -15,7 +17,7 @@ use crate::error::NamingError;
 ///
 /// [`Glyph`]: crate::Glyph
 /// [`Layer`]: crate::Layer
-#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize)]
 #[cfg_attr(feature = "druid", derive(druid::Data))]
 pub struct Name(Arc<str>);
 
@@ -86,7 +88,21 @@ impl std::borrow::Borrow<str> for Name {
     }
 }
 
-//FIXME: custom deserialize that errors
+impl<'de> Deserialize<'de> for Name {
+    fn deserialize<D>(deserializer: D) -> Result<Name, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        // we go directly to Arc<str> and validate manually so we don't need
+        // to allocate twice
+        let s: Arc<str> = Deserialize::deserialize(deserializer)?;
+        if is_valid(&s) {
+            Ok(Name(s))
+        } else {
+            Err(serde::de::Error::custom(NamingError::Invalid(s.to_string())))
+        }
+    }
+}
 
 #[cfg(test)]
 mod tests {