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 campoinner
, 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 unXmlStateInner
, se des-estructura para extraer el campodocument_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.