Federico's Blog

Since a few weeks ago, librsvg is now in oss-fuzz — Google's constantly-running fuzz-testing for OSS projects — and the crashes have started coming in. I'll have a lot more to say soon about crashes in Cairo, which is where the majority of the bugs are so far, but for now I want to tell you about a little bug I just fixed.

The fuzzer found a memory leak that happens when librsvg tries to parse an invalid XML document that has definitions for XML entities — the things that you normally reference like &foo; in the middle of the XML.

For example, this invalid document causes librsvg to leak:

<!DOCTYPEY[<!ENTITY a ''

Valgrind reports this:

$ valgrind --leak-check=full ./target/debug/rsvg-convert leak.svg 
...
Error reading SVG leak.svg: XML parse error: Error domain 1 code 37 on line 2 column 1 of data: xmlParseEntityDecl: entity a not terminated

==3750== 
==3750== HEAP SUMMARY:
==3750==     in use at exit: 78,018 bytes in 808 blocks
==3750==   total heap usage: 1,405 allocs, 597 frees, 205,161 bytes allocated
==3750== 
==3750== 247 (144 direct, 103 indirect) bytes in 1 blocks are definitely lost in loss record 726 of 750
==3750==    at 0x4845794: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==3750==    by 0x4BD857F: xmlCreateEntity (entities.c:158)
==3750==    by 0x4BD932B: xmlNewEntity (entities.c:451)
==3750==    by 0x2EBC75: rsvg::xml::xml2_load::sax_entity_decl_cb (xml2_load.rs:152)
==3750==    by 0x4BED6D8: xmlParseEntityDecl (parser.c:5647)
==3750==    by 0x4BEF4F3: xmlParseMarkupDecl (parser.c:7024)
==3750==    by 0x4BEFB95: xmlParseInternalSubset (parser.c:8558)
==3750==    by 0x4BF50E9: xmlParseDocument (parser.c:11072)
==3750==    by 0x2ED266: rsvg::xml::xml2_load::Xml2Parser::parse (xml2_load.rs:466)
==3750==    by 0x4A8C49: rsvg::xml::XmlState::parse_from_stream::{{closure}} (mod.rs:628)
==3750==    by 0x2ACA92: core::result::Result<T,E>::and_then (result.rs:1316)
==3750==    by 0x34D4E2: rsvg::xml::XmlState::parse_from_stream (mod.rs:627)
==3750== 
==3750== LEAK SUMMARY:
==3750==    definitely lost: 144 bytes in 1 blocks
==3750==    indirectly lost: 103 bytes in 3 blocks
==3750==      possibly lost: 0 bytes in 0 blocks
==3750==    still reachable: 73,947 bytes in 746 blocks
==3750==         suppressed: 0 bytes in 0 blocks

Let's see what happened.

The code in question

Even after the port to Rust, librsvg still uses libxml2 for parsing XML. So, librsvg has to deal with raw pointers incoming from libxml2 and it must do their memory management itself, since the Rust compiler doesn't know what to do with them automatically.

Librsvg uses the SAX parser, which involves setting up callbacks to process events like "XML element started", or "an entity was defined".

If you have a valid document that has entity definitions like these:

<!ENTITY foo "#aabbcc">
<!ENTITY bar "some text here">

Then libxml2's SAX parser will emit two events to instruct your code that it should define entities, one for foo and one for bar, with their corresponding content. Librsvg stores these in a hash table, since it has to be able to retrieve them later when the SAX parser requests it. In detail, libxml2 requires that you create an xmlEntityPtr by calling xmlNewEntity() and then keep it around.

xmlEntityPtr xmlNewEntity (xmlDocPtr      doc,
                           const xmlChar *name,
                           int            type,
                           const xmlChar *ExternalID,
                           const xmlChar *SystemID,
                           const xmlChar *content);

Later, you must free each of your stored entities with xmlFreeNode() (it supports different data types, including entities), or if you are using libxml2 2.12.0 or later, with xmlFreeEntity().

void xmlFreeNode (xmlNodePtr node);
void xmlFreeEntity (xmlEntityPtr entity);

Librsvg creates a SAX parser from libxml2, calls it to do the parsing, and then frees the entities at the end. In the following code, XmlState is the struct that librsvg uses to hold the temporary state during parsing: a partially-built XML tree, some counters on the number of loaded elements, the current element being processed, things like that. The build_document() method is called at the very end of XmlState's lifetime; it consumes the XmlState and returns either a fully-parsed and valid Document, or an error.

struct XmlState {
    inner: RefCell<XmlStateInner>,  // the mutable part

    // ... other immutable fields here
}

type XmlEntityPtr = *mut libc::c_void;

struct XmlStateInner {
    // ... a few fields for the partially-built XML tree, current element, etc.
    document_builder: DocumentBuilder,

    // 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<String, XmlEntityPtr>,
}

impl XmlState {
    fn build_document(
        self,
        stream: &gio::InputStream,
        cancellable: Option<&gio::Cancellable>,
    ) -> Result<Document, LoadingError> {
        // does the actual parsing with a libxml2 SAX parser
        self.parse_from_stream(stream, cancellable)?;

        // consume self, then consume inner, then consume document_builder by calling .build()
        let XmlState { inner, .. } = self;
        let mut inner = inner.into_inner();

        // Free the hash of XmlEntityPtr.  We cannot do this in Drop because we will
        // consume inner by destructuring it after the for() loop.
        for (_key, entity) in inner.entities.drain() {
            unsafe {
                xmlFreeNode(entity);
            }
        }

        let XmlStateInner {
            document_builder, ..
        } = inner;
        document_builder.build()
    }
}

There are many Rust-isms in this code.

After doing the actual parsing with parse_from_stream(), self is destructured to consume it and extract its inner field, which is the actual mutable part of the XML loading state.
The code frees each xmlEntityPtr stored in the hash table of entities.
The inner value, which is an XmlStateInner, is destructured to extract the document_builder field, which gets asked to .build() the final document tree.

Where's the bug?

The bug is in this line at the beginning of the build_document() function:

        self.parse_from_stream(stream, cancellable)?;

The ? after the function call is to return errors to the caller. However, if there is an error during parsing, we will exit the function here, and it will not have a chance to free the values in the key-value pairs among the entities ! Memory leak!

This code had already gone through a few refactorings. Initially I had an impl Drop for XmlState which did the obvious thing of freeing the entities by hand:

impl Drop for XmlState {
    fn drop(&mut self) {
        unsafe {
            let mut inner = self.inner.borrow_mut();

            for (_key, entity) in inner.entities.drain() {
                // entities are freed with xmlFreeNode(), believe it or not
                xmlFreeNode(entity);
            }
        }
    }
}

But at one point, I decided to clean up the way the entire inner struct was to be handled, and decided to destructure it at the end of its lifetime, since that made the code simpler. However, destructuring an object means that you cannot have an impl Drop for it, since then some fields are individually moved out and some are not during the destructuring. So, I changed the code to free the entities directly into build_document() as above.

I missed the case where the parser can exit early due to an error.

The Rusty solution

Look again at how the entities hash table is declared in the struct fields:

type XmlEntityPtr = *mut libc::c_void;

struct XmlStateInner {
    entities: HashMap<String, XmlEntityPtr>,
}

That is, we are storing a hash table with raw pointers in the value part of the key-value pairs. Rust doesn't know how to handle those external resources, so let's teach it how to do that.

The magic of having an impl Drop for a wrapper around an unmanaged resource, like xmlEntityPtr, is that Rust will automatically call that destructor at the appropriate time — in this case, when the hash table is freed.

So, let's use a wrapper around XmlEntityPtr, and add an impl Drop for the wrapper:

struct XmlEntity(xmlEntityPtr);

impl Drop for XmlEntity {
    fn drop(&mut self) {
        unsafe {
            xmlFreeNode(self.0);
        }
    }
}

And then, let's change the hash table to use that wrapper for the values:

    entities: HashMap<String, XmlEntity>,

Now, when Rust has to free the HashMap, it will know how to free the values. We can keep using the destructuring code in build_document() and it will work correctly even with early exits due to errors.

Valgrind's evidence without the leak

# valgrind --leak-check=full ./target/debug/rsvg-convert leak.svg 
==5855== Memcheck, a memory error detector
==5855== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.
==5855== Using Valgrind-3.23.0 and LibVEX; rerun with -h for copyright info
==5855== Command: ./target/debug/rsvg-convert leak.svg
==5855== 
Error reading SVG leak.svg: XML parse error: Error domain 1 code 37 on line 2 column 1 of data: xmlParseEntityDecl: entity a not terminated

==5855== 
==5855== HEAP SUMMARY:
==5855==     in use at exit: 77,771 bytes in 804 blocks
==5855==   total heap usage: 1,405 allocs, 601 frees, 205,161 bytes allocated
==5855== 
==5855== LEAK SUMMARY:
==5855==    definitely lost: 0 bytes in 0 blocks
==5855==    indirectly lost: 0 bytes in 0 blocks
==5855==      possibly lost: 0 bytes in 0 blocks
==5855==    still reachable: 73,947 bytes in 746 blocks
==5855==         suppressed: 0 bytes in 0 blocks

Moral of the story

Resources that are external to Rust really work best if they are wrapped at the lowest level, so that destructors can run automatically. Instead of freeing things by hand when you think it's right, let the compiler do it automatically when it knows it's right. In this case, wrapping xmlEntityPtr with a newtype and adding an impl Drop is all that is needed for the rest of the code to look like it's handling a normal, automatically-managed Rust object.