The String Pool now uses just a HashSet, that stores the actual Interned Strings. The old code also stored the str slices that we where looking for as keys, but they were never interned properly, so they were super likely to get freed at some point and cause a Use after Free.

Fixes #47

Is there some way to prevent < > substitution for CDATA text element?

add a text node like <![CDATA[Data]]> will give:

<?xml version=\'1.0\'?><FILE><CONTENT_TYPE>&lt;![CDATA[Data]]&gt;</CONTENT_TYPE></FILE>

In this case I'll get back <![CDATA[Data]]> as CONTENT_TYPE content instead of simply Data.

Can I work around on this behaviour or where I should take a look to implement it?

Thanks

There are some enums like ChildOfRoot or ChildOfElement that basically just differentiate if this particular child is an Element, a Text, a Comment or a ProcessingInstruction. What do you think about adding discriminator functions, á la:

fn is_element(&self) -> bool
fn is_comment(&self) -> bool
...

?

I'd volunteer to implement this ;)

Certain characters (such as /) in the document type declaration (dtd) could cause the parser to fail. This updates sxd_document::parser::parse_external_id() to use a slightly different (more permissive) function to consume the attribute string.

refs #50, #59

Sorry for the lengthy blob of xml, but I'm sort of at a loss for why this might be failing:

#[cfg(test)]
mod tests {
    use self::sxd_document::parser;

    #[test]
    fn test_parse_sample() {
        let xml = r##"<?xml version="1.0"?>
<!DOCTYPE cXML SYSTEM "http://xml.cxml.org/schemas/cXML/1.2.014/cXML.dtd">
<cXML xml:lang="en-US"
      payloadID="933695160894"
      timestamp="2002-08-15T08:47:00-07:00">
    <Header>
        <From>
            <Credential domain="DUNS">
                <Identity>83528721</Identity>
            </Credential>
        </From>
        <To>
            <Credential domain="DUNS">
                <Identity>65652314</Identity>
            </Credential>
        </To>
        <Sender>
            <Credential domain="workchairs.com">
                <Identity>website 1</Identity>
            </Credential>
            <UserAgent>Workchairs cXML Application</UserAgent>
        </Sender>
    </Header>
    <Message>
        <PunchOutOrderMessage>
            <BuyerCookie>1CX3L4843PPZO</BuyerCookie>
            <PunchOutOrderMessageHeader operationAllowed="edit">
                <Total>
                    <Money currency="USD">763.20</Money>
                </Total>
            </PunchOutOrderMessageHeader>
            <ItemIn quantity="3">
                <ItemID>
                    <SupplierPartID>5555</SupplierPartID>
                    <SupplierPartAuxiliaryID>E000028901</SupplierPartAuxiliaryID>
                </ItemID>
                <ItemDetail>
                    <UnitPrice>
                        <Money currency="USD">763.20</Money>
                    </UnitPrice>
                    <Description xml:lang="en">
                        <ShortName>Excelsior Desk Chair</ShortName>
                        Leather Reclining Desk Chair with Padded Arms
                    </Description>
                    <UnitOfMeasure>EA</UnitOfMeasure>
                    <Classification domain="UNSPSC">5136030000</Classification>
                    <LeadTime>12</LeadTime>
                </ItemDetail>
            </ItemIn>
            <ItemIn quantity="1">
                <ItemID>
                    <SupplierPartID>AM2692</SupplierPartID>
                    <SupplierPartAuxiliaryID>A_B:5008937A_B:</SupplierPartAuxiliaryID>
                </ItemID>
                <ItemDetail>
                    <UnitPrice>
                        <Money currency="USD">250.00</Money>
                    </UnitPrice>
                    <Description xml:lang="en-US">ANTI-RNase (15-30 U/ul)</Description>
                    <UnitOfMeasure>EA</UnitOfMeasure>
                    <Classification domain="UNSPSC">41106104</Classification>
                    <ManufacturerName/>
                    <LeadTime>0</LeadTime>
                </ItemDetail>
            </ItemIn>
        </PunchOutOrderMessage>
    </Message>
</cXML>
"##;
        parser::parse(xml).unwrap();

    }
}

The parse call fails with

panicked at 'called `Result::unwrap()` on an `Err` value: (50, [ExpectedClosingQuote("\"")])', /checkout/src/libcore/result.rs:916:5

which by my math is somewhere inside the dtd uri. It parses successfully if I remove the doctype tag entirely, but I can't imagine where the quotes should be tripping this up.

Found by cargo-fuzz:

crash-52cdb28f04f0c80d84609394d18ed2c0b8fedb7f.zip

Caused at:

...
<sxd_document::string_pool::InternedString as core::cmp::PartialEq>::eq
...
std::collections::hash::map::search_hashed
...
sxd_document::string_pool::StringPool::intern
sxd_document::raw::Storage::intern
sxd_document::raw::Storage::create_attribute
sxd_document::dom::Element::set_attribute_value
sxd_document::parser::DomBuilder::finish_opening_tag
sxd_document::parser::DomBuilder::consume
sxd_document::parser::parse

Freed at:

...
alloc::raw_vec::RawVec<...>::dealloc_buffer
RawVec<...>::drop
core::ptr::drop_in_place
core::ptr::drop_in_place
core::ptr::drop_in_place
core::ptr::drop_in_place
sxd_document::parser::DomBuilder::finish_opening_tag
sxd_document::parser::DomBuilder::consume
sxd_document::parser::parse

Allocated at:

...
<alloc::vec::Vec<T>>::extend_from_slice
alloc::string::String::push_str
sxd_document::parser::AttributeValueBuilder::ingest
sxd_document::parser::DomBuilder::finish_opening_tag
sxd_document::parser::DomBuilder::consume
sxd_document::parser::parse

This would allow for better interoperability with other error handling crates, particularly with the error-chain crate.

If you are unhappy with something feel free to tell me, especially if I should change something about the strings.

For now it only supports the SYSTEM variant without the optional intSubset. I only started working with this code today, I'm open to any criticism and modifications.

parsing this:

extern crate sxd_document;

use sxd_document::parser::Parser;

fn main() {
    let s = "<?xml version='1.0' encoding='UTF-8' standalone='no'?><doc/>";

    let parser = Parser::new();
    let doc = parser.parse(s);
    println!("{:?}", doc)
}

yields

Err((2, [InvalidProcessingInstructionTarget]))

yup.

Attribute prefixes should ignore the current default namespace, see https://stackoverflow.com/questions/3312390/xml-default-namespaces-for-unqualified-attribute-names

I've made a quick implementation by adding a bool to the related methods and setting it to true when dealing with attributes, but I feel this is somewhat dirty, if you have any preferences please let me know.

Right now sxd_document::writer::format_document requires a Sized writer. Using it with &mut Write produces a error:

src/crates/by_ad/../../by_ad.rs:901:3: 901:40 error: the trait `core::marker::Sized` is not implemented for the type `std::io::Write`
src/crates/by_ad/../../by_ad.rs:901   sxd_document::writer::format_document (&doc, out);
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/crates/by_ad/../../by_ad.rs:901:3: 901:40 `std::io::Write` does not have a constant size known at compile-time
src/crates/by_ad/../../by_ad.rs:901   sxd_document::writer::format_document (&doc, out);

(The code is along these lines:)

pub fn foo (out: &mut Write) {
    // ...
    sxd_document::writer::format_document (&doc, out);
}

Could you make the W unsized (?Sized)?

Hi! I recently got into my head an idea to play around with offline copies of Wikipedia. The Wikimedia foundation very helpfully provides downloadable dumps of the full content of Wikipedia. The dump itself is a 20GB file that unzips to one .xml file that measures 78 GB in size. You saw that right: 1 xml file, 84,602,863,258 bytes. You can probably see where this is going...

Alas, I am many gigabytes short of fitting this entire mountain of a document in memory at once, let alone twice plus overhead (as a string and parsed). If I have any hope of consuming this thing with precision (as opposed to regex shudder) I believe a streaming parser and query engine will be necessary, however I did not see a streaming interface in the xsd_document::parser docs or in sxd_xpath; is that a correct assessment? Have you considered building a streaming interface to handle such cases? (In my experience, opting for a streaming solution can lead to the fastest implementation even when memory pressure is not a concern; such a use-case may be a valuable just to reach for speed.)

Thoughts?

For any crate that doesn't use #[forbid(unsafe_code)], the very first thing I do before considering it for use it to git clone --depth=1 it and run cargo +nightly miri test on it... this crate didn't pass.

% git clone --depth=1 https://github.com/shepmaster/sxd-document.git
Cloning into 'sxd-document'...
remote: Enumerating objects: 24, done.
remote: Counting objects: 100% (24/24), done.
remote: Compressing objects: 100% (20/20), done.
remote: Total 24 (delta 1), reused 6 (delta 0), pack-reused 0
Unpacking objects: 100% (24/24), done.
Checking connectivity... done.
% cd sxd-document
% cargo +nightly miri test
    Updating crates.io index
  Downloaded peresil v0.3.0
  Downloaded typed-arena v1.7.0
  Downloaded 2 crates (23.5 KB) in 0.63s
   Compiling typed-arena v1.7.0
   Compiling peresil v0.3.0
   Compiling sxd-document v0.3.2 (/home/ssokolow/src/sxd-document)
    Finished test [unoptimized + debuginfo] target(s) in 20.41s
     Running target/x86_64-unknown-linux-gnu/debug/deps/sxd_document-2ca61747ef9df327

running 194 tests
test dom::test::attributes_belong_to_a_document ... ok
test dom::test::attributes_can_be_iterated ... ok
test dom::test::attributes_can_be_removed ... ok
test dom::test::attributes_can_be_removed_from_parent ... ok
test dom::test::attributes_can_be_reset ... ok
test dom::test::attributes_know_their_element ... ok
test dom::test::can_return_a_populated_package ... error: Undefined Behavior: trying to reborrow for SharedReadWrite at alloc260646, but parent tag <695071> does not have an appropriate item in the borrow stack
    --> src/raw.rs:521:9
     |
521  |         parent_r.children.push(child);
     |         ^^^^^^^^^^^^^^^^^ trying to reborrow for SharedReadWrite at alloc260646, but parent tag <695071> does not have an appropriate item in the borrow stack
     |
     = help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
     = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
             
     = note: inside `raw::Connections::append_root_child::<raw::ChildOfRoot>` at src/raw.rs:521:9
note: inside `dom::Root::append_child::<dom::Element>` at src/dom.rs:174:9
    --> src/dom.rs:174:9
     |
174  |         self.document.connections.append_root_child(child.as_raw());
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `dom::test::can_return_a_populated_package::populate` at src/dom.rs:1608:17
    --> src/dom.rs:1608:17
     |
1608 |                 doc.root().append_child(element);
     |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `dom::test::can_return_a_populated_package` at src/dom.rs:1614:23
    --> src/dom.rs:1614:23
     |
1614 |         let package = populate();
     |                       ^^^^^^^^^^
note: inside closure at src/dom.rs:1601:5
    --> src/dom.rs:1601:5
     |
1601 | /     fn can_return_a_populated_package() {
1602 | |         fn populate() -> Package {
1603 | |             let package = Package::new();
1604 | |             {
...    |
1617 | |         assert_qname_eq!(element.name(), "hello");
1618 | |     }
     | |_____^
     = note: inside `<[closure@src/dom.rs:1601:5: 1618:6] as std::ops::FnOnce<()>>::call_once - shim` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
     = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
     = note: inside `test::__rust_begin_short_backtrace::<fn()>` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:516:5
     = note: inside closure at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:507:30
     = note: inside `<[closure@test::run_test::{closure#2}] as std::ops::FnOnce<()>>::call_once - shim(vtable)` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
     = note: inside `<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send> as std::ops::FnOnce<()>>::call_once` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1328:9
     = note: inside `<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>> as std::ops::FnOnce<()>>::call_once` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:322:9
     = note: inside `std::panicking::r#try::do_call::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:379:40
     = note: inside `std::panicking::r#try::<(), std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>>` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:343:19
     = note: inside `std::panic::catch_unwind::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:396:14
     = note: inside `test::run_test_in_process` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:538:18
     = note: inside closure at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:449:39
     = note: inside `test::run_test::run_test_inner` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:474:13
     = note: inside `test::run_test` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:504:28
     = note: inside `test::run_tests::<[closure@test::run_tests_console::{closure#2}]>` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:283:13
     = note: inside `test::run_tests_console` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/console.rs:289:5
     = note: inside `test::test_main` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:121:15
     = note: inside `test::test_main_static` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:140:5
     = note: inside `main`
     = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
     = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:125:18
     = note: inside closure at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:66:18
     = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:259:13
     = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:379:40
     = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:343:19
     = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:396:14
     = note: inside `std::rt::lang_start_internal` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:51:25
     = note: inside `std::rt::lang_start::<()>` at /home/ssokolow/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:65:5
     = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

error: test failed, to rerun pass '--lib'

Miri isn't exhaustive, but I consider it to be the bare minimum that the tests for a crate which uses unsafe or has it in its transitive dependencies must pass and, once you've fixed this, I'd strongly recommend adding it to your CI runs, similar to how I'd want a C or C++ codebase to run their tests under LLVM's various sanitizers (i.e. ASan, UBSan, etc.)

Loading and saving Flat LibreOffice Text document makes it unreadable:

fn main() {
    let xml = std::fs::read_to_string("template.fodt").expect("Failed to open");
    let doc = sxd_document::parser::parse(&xml).expect("Failed to parse");
    
    let mut output = Vec::new();
    sxd_document::writer::format_document(&doc.as_document(), &mut output).expect("unable to output XML");
    std::fs::write("output.fodt", &output).expect("Failed to write");
}

The very minimal template file is attached (but zipped because of Github).

template.fodt.zip

Hello,

I think that should be moved to examples directory because otherwise people can do cargo install sxd-document and it will override open binary which they most likely already have installed.