Arreglar una fuga de memoria de un xmlEntityPtr en librsvg

Translations: en - Tags: gnome, librsvg, refactoring, rust

Desde hace unas semanas, librsvg ya es parte de oss-fuzz — el servicio de Google que hace fuzz testing de manera contínua para proyectos de software libre — y ya empezaron a aparecer los primeros bugs. Pronto tendré mucho que decir sobre los crashes en Cairo, que es donde está la mayoría de ellos, pero por ahora quiero contarles de un pequeño bug que acabo de arreglar.

El fuzzer encontró una fuga de memoria que ocurre cuando librsvg intenta procesar un documento XML inválido que tiene definiciones de entidades de XML — las cosas que se referencian como &foo; en medio de un documento XML.

Por ejemplo, este documento inválido hace que librsvg tenga una fuga de memoria:

<!DOCTYPEY[<!ENTITY a ''

Esto es lo que reporta Valgrind:

$ 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

Vamos a ver qué ocurrió.

El código en cuestión

Aun después del port a Rust, librsvg sigue usando libxml2 para procesar XML. Entonces, librsvg tiene que lidiar con los punteros que le llegan de libxml2 y debe manejar su memoria de forma manual, porque el compilador de Rust no sabe qué hacer con ellos automáticamente.

Librsvg usa el parser SAX, que requiere definir varias funciones para procesar eventos como "comenzó un elemento de XML" o "se definió una entidad".

Si tienes un documento válido con definiciones de entidades como estas:

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

Entonces el parser SAX de libxml2 va a emitir dos eventos para pedirle a tu código que defina dos entidades, una para foo y otra para bar, con su contenido correspondiente. Librsvg guarda esas entidades en una tabla de hash, porque después tiene que poderlas acceder cuando el parser SAX se las pida. En detalle, libxml2 requiere que tu código construya un xmlEntityPtr llamando a xmlNewEntity() y que luego lo guarde por ahí.

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

Más tarde, debes liberar la memoria de cada una de tus entidades usando xmlFreeNode() (en realidad soporta diferentes tipos de datos, incluidas las entidades), o si estás usando libxml2 2.12.0 o superior, con xmlFreeEntity().

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

Librsvg crea un parser SAX de libxml2, lo llama para hacer el parseo, y luego libera las entidades a mano. En el código siguiente, XmlState es la estructura que librsvg utiliza para guardar el estado temporal mientras se hace el parseo: un árbol de XML a medio construir, contadores del número de elementos creados, el elemento actual, cosas así. Al final de todo, se llama al método build_document() cuando XmlState está por terminar su ciclo de vida; esa función consume el XmlState y devuelve un documento válido parseado por completo o un error.

struct XmlState {
    inner: RefCell<XmlStateInner>,  // la parte mutable

    // ... otros campos inmutables aquí
}

type XmlEntityPtr = *mut libc::c_void;

struct XmlStateInner {
    // ... algunos campos con el árbol XML a medio construir, etc.
    document_builder: DocumentBuilder,

    // Nótese que ni XmlStateInner ni Xmlstate implementan Drop.
    //
    // Un XmlState se consume al final de su vida en XmlState::build_document(), y esa
    // función es responsable de liberar todos los XmlEtntityPtr del campo siguiente.
    //
    // (Las estructuras no pueden tener impl Drop porque build_document()
    // las des-estructura y consume al mismo tiempo.)
    entities: HashMap<String, XmlEntityPtr>,
}

impl XmlState {
    fn build_document(
        self,
        stream: &gio::InputStream,
        cancellable: Option<&gio::Cancellable>,
    ) -> Result<Document, LoadingError> {
        // hace el parseo con un parser SAX de libxml2
        self.parse_from_stream(stream, cancellable)?;

        // consumir self, luego consumir inner, luego consumir document_builder usando .build()
        let XmlState { inner, .. } = self;
        let mut inner = inner.into_inner();

        // Liberar el hash de XmlEntityPtr.  No podemos hacerlo en Drop porque consumimos a inner
        // des-estructurándolo.
        for (_key, entity) in inner.entities.drain() {
            unsafe {
                xmlFreeNode(entity);
            }
        }

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

Hay muchos Rust-ismos en este código.

  • Después de hacer el parseo en parse_from_stream(), self se des-estructura para consumirlo y ahí se extrae el campo inner, que es la parte mutable del estado temporal mientras se parsea el XML.

  • El código libera cada xmlEntityPtr que estaba guardado en la tabla de entidades.

  • El valor de inner, que es un XmlStateInner, se des-estructura para extraer el campo document_builder, que se consume al hacerle .build() para construir el árbol de XML ya terminado.

¿Dónde está el bug?

El bug ocurre en esta línea al principio de la función build_document():

        self.parse_from_stream(stream, cancellable)?;

El ? después de la llamada a la función es para devolver errores. Sin embargo, si hay un error durante el parseo, el código se saldrá de la función justo ahí y no va poder liberar los valores en las parejas llave-valor que están guarados en entities. ¡Fuga de memoria!

Este código ya había pasado por algunas refactorizaciones. Al principio le había puesto un impl Drop for XmlState que hacia lo obvio para liberar las entidades a mano:

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

            for (_key, entity) in inner.entities.drain() {
                // las entidades se liberan con xmlFreeNode(); ¿raro, no?
                xmlFreeNode(entity);
            }
        }
    }
}

Pero en cierto punto, decidí limpiar la manera en que se maneja la estructura inner, y usar la des-estructuración para sacarle los campos útiles al final de su tiempo de vida, pues eso haría el código más sencillo. Sin embargo, des-estructurar un objeto quiere decir que no puede tener un impl Drop para el objeto completo, pues algunos campos se van a mover, y otros a liberar de forma individual. Entonces cambié el código para liberar las entidades directamente en build_document() como decía ahí arriba.

Y no me di cuenta del caso en el que el parser se sale de la función prematuramente cuando encuentra un error.

La solución rustesca

Veamos de nuevo cómo se declara la tabla de hash para los entities:

type XmlEntityPtr = *mut libc::c_void;

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

Es decir, en la tabla de hash guardamos punteros que vienen directo de libxml2 en la parte del valor de las parejas pares llave-valor. Rust no sabe cómo manejar esos recursos externos, entonces vamos a enseñarle cómo.

La magia de tener un impl Drop para una envoltura alrededor de un recurso externo, como xmlEntityPtr, es que Rust entonces puede llamar al destructor automáticamente cuando sea necesario — en este caso, cuando se libera la tabla de hash.

Así pues, hagamos una envoltura alrededor de XmlEntityPtr, y pongámosle un impl Drop a la envoltura:

struct XmlEntity(xmlEntityPtr);

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

Y por último, cambiemos la tabla de hash para usar la envoltura para los valores:

    entities: HashMap<String, XmlEntity>,

Así, cuando Rust tenga que liberar el HashMap, ya va a saber cómo liberar los valores. Podemos seguir usando el código para des-estructurar el resto de las cosas en build_document() y funcionará correctamente aunque la función termine prematuramente debido a un error durante el parseo.

La evidencia de Valgrind sin la fuga de memoria

# 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

Moraleja de la historia

Los recursos externos que se manejan desde Rust funcionan mejor si les ponemos una envoltura en nivel más bajo posible, para que los destructores puedan correr de forma automática. En vez de liberar cosas a mano cuando uno cree que es adecuado, conviene más dejar que el compilador lo haga cuando sabe que es necesario. En este caso, envolver xmlEntityPtr con un newtype y su impl Drop es todo lo necesario para que el resto del código se sienta como si estuviéramos usando objetos de Rust con manejo automático de memoria.