diff --git a/.travis.yml b/.travis.yml index 886caf7..48649c8 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,8 +4,35 @@ rust: - stable - beta - 1.34.0 +jobs: + include: + - rust: 1.34.0 + env: TEST_MINIMAL_VERSIONS=1 + - rust: 1.34.0 + env: LINT=1 +before_install: + - | + if [ "${LINT:-0}" -ne 0 ] ; then + rustup component add clippy rustfmt + cargo clippy --version + cargo fmt --version + fi + - | + if [ "${TEST_MINIMAL_VERSIONS:-0}" -ne 0 ] ; then + rustup install nightly + fi +before_script: + # Use dependencies with minimal versions. + - | + if [ "${TEST_MINIMAL_VERSIONS:-0}" -ne 0 ] ; then + cargo +nightly update -Z minimal-versions + fi script: - - cargo build --verbose --all-features - - cargo test --verbose --all-features + # TODO: Use `--workspace` instead of `--all` for Rust 1.39 or later. + - if [ "${LINT:-0}" -eq 0 ] ; then cargo build --verbose --all --all-features && cargo test --verbose --all --all-features ; fi + # Fail if the code is correctly formatted. + - if [ "${LINT:-0}" -ne 0 ] ; then cargo fmt --all -- --check ; fi + # Fail if the code has warnings. + - if [ "${LINT:-0}" -ne 0 ] ; then cargo clippy --all-features -- --deny warnings ; fi notifications: email: false diff --git a/CHANGELOG.md b/CHANGELOG.md index f510d79..a3e98c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,37 @@ ## [Unreleased] +## [0.5.0] + +* `pull_parser::error::{DataError, OperationError, Warning}` is now nonexhaustive. + + This would make some of future changes non-breaking. +* Support parsing nodes with missing or extra node end markers. + + Previously, they are ignored or causing critical errors. + Now they are notified as warnings, and users can continue parsing. + + Two new variants `Warning::{ExtraNodeEndMarker, MissingNodeEndMarker}` are added to + `pull_parser::error::Warning` type. + - Note that `Warning` have been nonexhaustive since this release. +* Deprecated items are removed. + + `low::FbxHeader::read_fbx_header()` + + `pull_parser::v7400::attribute::DirectAttributeValue` + +### Breaking changes +* `pull_parser::error::{DataError, OperationError, Warning}` is now nonexhaustive + (d0651118feabf842f9495da626ccb127090db331). + + This would make some of future changes non-breaking. +* Support parsing nodes with missing or extra node end markers + (8c3d8b7f210fe8422784ef86b468e5331bb0c2ee). + + Previously, missing node end markers caused errors, and extra node end markers were silently + ignored. + Now they are notified as warnings. + Users can choose whether to continue or abort processing. + + Two new variants `Warning::{ExtraNodeEndMarker, MissingNodeEndMarker}` are added to + `pull_parser::error::Warning` type. + - Note that `Warning` have been nonexhaustive since this release. +* Deprecated items are removed (9e38b4217d33ed8bca3f7e8b11d210845a4fa8c1). + + `low::FbxHeader::read_fbx_header()` + + `pull_parser::v7400::attribute::DirectAttributeValue` + ## [0.4.4] * Documents are improved a little. @@ -211,7 +242,8 @@ Totally rewritten. -[Unreleased]: +[Unreleased]: +[0.4.4]: [0.4.4]: [0.4.3]: [0.4.2]: diff --git a/Cargo.toml b/Cargo.toml index cc8345e..7ae8a63 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "fbxcel" -version = "0.4.4" +version = "0.5.0" authors = ["YOSHIOKA Takuma "] edition = "2018" license = "MIT OR Apache-2.0" @@ -27,10 +27,10 @@ log = "0.4.4" string-interner = { version = "0.7", optional = true, default-features = false } [dev-dependencies] -env_logger = "0.6" +env_logger = "0.7" [badges] -maintenance = { status = "actively-developed" } +maintenance = { status = "passively-maintained" } travis-ci = { repository = "lo48576/fbxcel" } [[example]] diff --git a/README.md b/README.md index 66fc8b3..be06ba8 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # fbxcel -[![Build Status](https://travis-ci.org/lo48576/fbxcel.svg?branch=develop)](https://travis-ci.org/lo48576/fbxcel) +[![Build Status](https://travis-ci.com/lo48576/fbxcel.svg?branch=develop)](https://travis-ci.com/lo48576/fbxcel) [![Latest version](https://img.shields.io/crates/v/fbxcel.svg)](https://crates.io/crates/fbxcel) [![Documentation](https://docs.rs/fbxcel/badge.svg)](https://docs.rs/fbxcel) ![Minimum rustc version: 1.34](https://img.shields.io/badge/rustc-1.34+-lightgray.svg) diff --git a/src/lib.rs b/src/lib.rs index 79c4713..9c88f72 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,6 +12,7 @@ //! //! `writer` module provides writer types. //! To use `writer` module, enable `writer` feature. +#![forbid(unsafe_code)] #![warn(missing_docs)] #![warn(clippy::missing_docs_in_private_items)] diff --git a/src/low/fbx_header.rs b/src/low/fbx_header.rs index df45ff3..0794b26 100644 --- a/src/low/fbx_header.rs +++ b/src/low/fbx_header.rs @@ -67,12 +67,6 @@ impl FbxHeader { }) } - /// Reads an FBX header from the given reader. - #[deprecated(since = "0.4.1", note = "Renamed to `load`")] - pub fn read_fbx_header(reader: impl io::Read) -> Result { - Self::load(reader) - } - /// Returns FBX version. pub fn version(self) -> FbxVersion { self.version diff --git a/src/pull_parser/error/data.rs b/src/pull_parser/error/data.rs index 70544b8..3b92ec8 100644 --- a/src/pull_parser/error/data.rs +++ b/src/pull_parser/error/data.rs @@ -43,6 +43,8 @@ pub enum DataError { /// /// The former is the expected, the latter is a description of the actual value. UnexpectedAttribute(String, String), + #[doc(hidden)] + __Nonexhaustive, } impl error::Error for DataError { @@ -86,6 +88,7 @@ impl fmt::Display for DataError { "Unexpected attribute value or type: expected {}, got {}", expected, got ), + DataError::__Nonexhaustive => unreachable!("`__Nonexhaustive` should never used"), } } } diff --git a/src/pull_parser/error/operation.rs b/src/pull_parser/error/operation.rs index 731b016..b0e18ab 100644 --- a/src/pull_parser/error/operation.rs +++ b/src/pull_parser/error/operation.rs @@ -13,6 +13,8 @@ pub enum OperationError { AlreadyFinished, /// Attempt to create a parser with unsupported FBX version. UnsupportedFbxVersion(ParserVersion, FbxVersion), + #[doc(hidden)] + __Nonexhaustive, } impl error::Error for OperationError {} @@ -32,6 +34,7 @@ impl fmt::Display for OperationError { "Unsupported FBX version: parser={:?}, fbx={:?}", parser, fbx ), + OperationError::__Nonexhaustive => unreachable!("`__Nonexhaustive` should never used"), } } } diff --git a/src/pull_parser/error/warning.rs b/src/pull_parser/error/warning.rs index 209265b..93a750f 100644 --- a/src/pull_parser/error/warning.rs +++ b/src/pull_parser/error/warning.rs @@ -7,6 +7,8 @@ use std::{error, fmt}; pub enum Warning { /// Node name is empty. EmptyNodeName, + /// Extra (unexpected) node end marker found. + ExtraNodeEndMarker, /// Incorrect boolean representation. /// /// Boolean value in node attributes should be some prescribed value @@ -17,8 +19,12 @@ pub enum Warning { IncorrectBooleanRepresentation, /// Footer padding length is invalid. InvalidFooterPaddingLength(usize, usize), + /// Missing a node end marker where the marker is expected. + MissingNodeEndMarker, /// Unexpected value for footer fields (mainly for unknown fields). UnexpectedFooterFieldValue, + #[doc(hidden)] + __Nonexhaustive, } impl error::Error for Warning {} @@ -27,6 +33,7 @@ impl fmt::Display for Warning { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Warning::EmptyNodeName => write!(f, "Node name is empty"), + Warning::ExtraNodeEndMarker => write!(f, "Extra (unexpected) node end marker found"), Warning::IncorrectBooleanRepresentation => { write!(f, "Incorrect boolean representation") } @@ -35,7 +42,9 @@ impl fmt::Display for Warning { "Invalid footer padding length: expected {} bytes, got {} bytes", expected, got ), + Warning::MissingNodeEndMarker => write!(f, "Missing node end marker"), Warning::UnexpectedFooterFieldValue => write!(f, "Unexpected footer field value"), + Warning::__Nonexhaustive => unreachable!("`__Nonexhaustive` should never used"), } } } diff --git a/src/pull_parser/v7400/attribute.rs b/src/pull_parser/v7400/attribute.rs index d5eb864..87eab5d 100644 --- a/src/pull_parser/v7400/attribute.rs +++ b/src/pull_parser/v7400/attribute.rs @@ -14,15 +14,6 @@ use crate::{ use self::array::{ArrayAttributeValues, AttributeStreamDecoder, BooleanArrayAttributeValues}; pub use self::loader::LoadAttribute; -/// Use [`low::v7400::AttributeValue`] instead. -/// -/// [`low::v7400::AttributeValue`]: ../../../low/v7400/enum.AttributeValue.html -#[deprecated( - since = "0.4.0", - note = "`DirectAttributeValue` is moved to `low::v7400::AttributeValue`" -)] -pub type DirectAttributeValue = crate::low::v7400::AttributeValue; - mod array; pub mod iter; mod loader; diff --git a/src/pull_parser/v7400/event.rs b/src/pull_parser/v7400/event.rs index 93f4aee..2929897 100644 --- a/src/pull_parser/v7400/event.rs +++ b/src/pull_parser/v7400/event.rs @@ -10,7 +10,7 @@ use crate::{ /// Parser event. #[derive(Debug)] -pub enum Event<'a, R: 'a> { +pub enum Event<'a, R> { /// Start of a node. StartNode(StartNode<'a, R>), /// End of a node. diff --git a/src/pull_parser/v7400/parser.rs b/src/pull_parser/v7400/parser.rs index 24ed73d..7d6dcd2 100644 --- a/src/pull_parser/v7400/parser.rs +++ b/src/pull_parser/v7400/parser.rs @@ -2,8 +2,6 @@ use std::{fmt, io}; -use log::debug; - use crate::{ low::{ v7400::{FbxFooter, NodeHeader}, @@ -304,13 +302,9 @@ impl Parser { // It's odd, the current node should have a node end marker // at the ending, but `node_end_offset` data tells that the // node ends without node end marker. - debug!( - "DataError::NodeLengthMismatch, node_end_offset={}, event_start_offset={}", - current_node.node_end_offset, event_start_offset - ); - return Err( - DataError::NodeLengthMismatch(current_node.node_end_offset, None).into(), - ); + self.warn(Warning::MissingNodeEndMarker, self.position())?; + self.state.started_nodes.pop(); + return Ok(EventKind::EndNode); } } } @@ -332,6 +326,11 @@ impl Parser { ) .into()); } + if closing.attributes_count != 0 && closing.known_children_count == 0 { + // It's odd, the current node should not have a node end + // marker at the ending, but found. + self.warn(Warning::ExtraNodeEndMarker, self.position())?; + } Ok(EventKind::EndNode) } None => Ok(EventKind::EndFbx), diff --git a/src/tree/v7400/node/handle.rs b/src/tree/v7400/node/handle.rs index 2877ece..e1d0799 100644 --- a/src/tree/v7400/node/handle.rs +++ b/src/tree/v7400/node/handle.rs @@ -82,7 +82,7 @@ impl<'a> NodeHandle<'a> { .node_name_sym(name) .map(|sym| self.children().filter(move |child| child.name_sym() == sym)) .into_iter() - .flat_map(|iter| iter) + .flatten() } /// Compares nodes strictly. diff --git a/tests/missing-and-extra-node-end-marker.rs b/tests/missing-and-extra-node-end-marker.rs new file mode 100644 index 0000000..25c4d0a --- /dev/null +++ b/tests/missing-and-extra-node-end-marker.rs @@ -0,0 +1,256 @@ +//! Tests for writer, tree, and parser. +#![cfg(all(feature = "tree", feature = "writer"))] + +use std::{cell::RefCell, io::Cursor, iter, rc::Rc}; + +use fbxcel::{ + low::FbxVersion, + pull_parser::{ + any::{from_seekable_reader, AnyParser}, + error::Warning, + }, +}; + +use self::v7400::writer::{ + expect_fbx_end, expect_node_end, expect_node_start, CUSTOM_UNKNOWN1, MAGIC, UNKNOWN3, +}; + +mod v7400; + +/// Parses a node which lacks necessary node end marker. +#[test] +fn missing_node_end_marker() -> Result<(), Box> { + let data = { + let raw_ver = 7400_u32; + let mut vec = Vec::new(); + // Header. + { + // Magic. + vec.extend(MAGIC); + // Version. + vec.extend(&raw_ver.to_le_bytes()); + } + // Nodes. + { + // Container node. + { + const CONTAINER: &[u8] = b"Container"; + let container_start = vec.len(); + // End offset. + vec.extend(&[0; 4]); + // Number of node properties. + vec.extend(&[0; 4]); + // Length of node properties in bytes. + vec.extend(&[0; 4]); + // Node name length. + vec.push(CONTAINER.len() as u8); + // Node name. + vec.extend(CONTAINER); + + // Invalid node. + { + const INVALID_NODE: &[u8] = b"InvalidNode"; + let invalid_node_start = vec.len(); + // End offset. + vec.extend(&[0; 4]); + // Number of node properties. + vec.extend(&[0; 4]); + // Length of node properties in bytes. + vec.extend(&[0; 4]); + // Node name length. + vec.push(INVALID_NODE.len() as u8); + // Node name. + vec.extend(INVALID_NODE); + let end_pos = (vec.len() as u32).to_le_bytes(); + vec[invalid_node_start..(invalid_node_start + 4)].copy_from_slice(&end_pos); + } + + // Node end marker. + vec.extend(&[0; 13]); + let end_pos = (vec.len() as u32).to_le_bytes(); + vec[container_start..(container_start + 4)].copy_from_slice(&end_pos); + } + // End of implicit root. + { + vec.extend(iter::repeat(0).take(4 * 3 + 1)); + } + } + // Footer. + { + // Footer: unknown1. + vec.extend(&CUSTOM_UNKNOWN1); + // Footer: padding. + { + let len = vec.len().wrapping_neg() % 16; + assert_eq!((vec.len() + len) % 16, 0); + vec.extend(iter::repeat(0).take(len)); + } + // Footer: unknown2. + vec.extend(&[0; 4]); + // Footer: FBX version. + vec.extend(&raw_ver.to_le_bytes()); + // Footer: 120 zeroes. + vec.extend(iter::repeat(0).take(120)); + // Footer: unknown3. + vec.extend(&UNKNOWN3); + } + vec + }; + + assert_eq!(data.len() % 16, 0); + + let mut parser = match from_seekable_reader(Cursor::new(data))? { + AnyParser::V7400(parser) => parser, + _ => panic!("Generated data should be parsable with v7400 parser"), + }; + let warnings = Rc::new(RefCell::new(Vec::new())); + parser.set_warning_handler({ + let warnings = warnings.clone(); + move |warning, _pos| { + warnings.borrow_mut().push(warning); + Ok(()) + } + }); + assert_eq!(parser.fbx_version(), FbxVersion::V7_4); + + { + let attrs = expect_node_start(&mut parser, "Container")?; + assert_eq!(attrs.total_count(), 0); + } + { + let attrs = expect_node_start(&mut parser, "InvalidNode")?; + assert_eq!(attrs.total_count(), 0); + } + expect_node_end(&mut parser)?; + expect_node_end(&mut parser)?; + + let _: Box = expect_fbx_end(&mut parser)??; + + match &warnings.borrow()[..] { + [Warning::MissingNodeEndMarker] => {} + v => panic!("Unexpected warnings: {:?}", v), + } + + Ok(()) +} + +/// Parses a node which has extra node end marker. +#[test] +fn extra_node_end_marker() -> Result<(), Box> { + let data = { + let raw_ver = 7400_u32; + let mut vec = Vec::new(); + // Header. + { + // Magic. + vec.extend(MAGIC); + // Version. + vec.extend(&raw_ver.to_le_bytes()); + } + // Nodes. + { + // Container node. + { + const CONTAINER: &[u8] = b"Container"; + let container_start = vec.len(); + // End offset. + vec.extend(&[0; 4]); + // Number of node properties. + vec.extend(&[0; 4]); + // Length of node properties in bytes. + vec.extend(&[0; 4]); + // Node name length. + vec.push(CONTAINER.len() as u8); + // Node name. + vec.extend(CONTAINER); + + // Invalid node. + { + const INVALID_NODE: &[u8] = b"InvalidNode"; + let invalid_node_start = vec.len(); + // End offset. + vec.extend(&[0; 4]); + // Number of node properties. + vec.extend(&1_u32.to_le_bytes()); + // Length of node properties in bytes. + vec.extend(&2_u32.to_le_bytes()); + // Node name length. + vec.push(INVALID_NODE.len() as u8); + // Node name. + vec.extend(INVALID_NODE); + // An attribute. + vec.extend(&[b'C', b'T']); + // Extra node end marker. + vec.extend(&[0; 13]); + let end_pos = (vec.len() as u32).to_le_bytes(); + vec[invalid_node_start..(invalid_node_start + 4)].copy_from_slice(&end_pos); + } + + // Node end marker. + vec.extend(&[0; 13]); + let end_pos = (vec.len() as u32).to_le_bytes(); + vec[container_start..(container_start + 4)].copy_from_slice(&end_pos); + } + // End of implicit root. + { + vec.extend(iter::repeat(0).take(4 * 3 + 1)); + } + } + // Footer. + { + // Footer: unknown1. + vec.extend(&CUSTOM_UNKNOWN1); + // Footer: padding. + { + let len = vec.len().wrapping_neg() % 16; + assert_eq!((vec.len() + len) % 16, 0); + vec.extend(iter::repeat(0).take(len)); + } + // Footer: unknown2. + vec.extend(&[0; 4]); + // Footer: FBX version. + vec.extend(&raw_ver.to_le_bytes()); + // Footer: 120 zeroes. + vec.extend(iter::repeat(0).take(120)); + // Footer: unknown3. + vec.extend(&UNKNOWN3); + } + vec + }; + + assert_eq!(data.len() % 16, 0); + + let mut parser = match from_seekable_reader(Cursor::new(data))? { + AnyParser::V7400(parser) => parser, + _ => panic!("Generated data should be parsable with v7400 parser"), + }; + let warnings = Rc::new(RefCell::new(Vec::new())); + parser.set_warning_handler({ + let warnings = warnings.clone(); + move |warning, _pos| { + warnings.borrow_mut().push(warning); + Ok(()) + } + }); + assert_eq!(parser.fbx_version(), FbxVersion::V7_4); + + { + let attrs = expect_node_start(&mut parser, "Container")?; + assert_eq!(attrs.total_count(), 0); + } + { + let attrs = expect_node_start(&mut parser, "InvalidNode")?; + assert_eq!(attrs.total_count(), 1); + } + expect_node_end(&mut parser)?; + expect_node_end(&mut parser)?; + + let _: Box = expect_fbx_end(&mut parser)??; + + match &warnings.borrow()[..] { + [Warning::ExtraNodeEndMarker] => {} + v => panic!("Unexpected warnings: {:?}", v), + } + + Ok(()) +}