servo / font-kit

A cross-platform font loading library written in Rust
Apache License 2.0
660 stars 98 forks source link

v0.12 SIGSEGV: invalid memory reference #224

Closed jayvdb closed 7 months ago

jayvdb commented 7 months ago

When I update the version for https://crates.io/crates/plotters and run their tests, I dont get compile issues - I get:

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

Caused by:
  process didn't exit successfully: `/home/jayvdb/rust/plotters/target/debug/deps/plotters-14ec6df465028332` (signal: 11, SIGSEGV: invalid memory reference)

It occurs after the tests have finished - i.e. it is on exit.

The SIGSEGV comes from https://github.com/servo/font-kit/commit/f9b1876e44d32eec545810bdf9f57b11da658863 . reverting that resolves the issue.

Also when keeping that commit, and instead commenting out the freetype.rs impl Drop for Font {...} also makes the problem go away.

So I assume that it is interplay between the impl Drop for Font {...} and impl Drop for FtLibrary {...} that is causing the problem. https://freetype.org/freetype2/docs/reference/ft2-module_management.html#ft_done_library says it cleans up all resources.

https://github.com/plotters-rs/plotters/blob/d84fdb5210a8d2a5eef0fa9ce014321c1e56260e/plotters/src/style/font/ttf.rs includes its own cache of Font (inner member of FontExt)

thread_local! {
    static FONT_SOURCE: SystemSource = SystemSource::new();
    static FONT_OBJECT_CACHE: RefCell<HashMap<String, FontExt>> = RefCell::new(HashMap::new());
}

As plotters does not directly use font_kit::loaders::freetype , it seems that font-kit should be responsible for ensuring that the drops are happening in the right order.

jayvdb commented 7 months ago

The following both appear to resolve the problem, but I am not very familiar with TLS so this may not be optimal yet. Will keep reading & testing

diff --git a/src/loaders/freetype.rs b/src/loaders/freetype.rs
index 1ef0698..f09e08c 100644
--- a/src/loaders/freetype.rs
+++ b/src/loaders/freetype.rs
@@ -987,11 +987,11 @@ impl Clone for Font {

 impl Drop for Font {
     fn drop(&mut self) {
-        unsafe {
-            if !self.freetype_face.is_null() {
+        let _ = FREETYPE_LIBRARY.try_with(|freetype_library| unsafe {
+            if !freetype_library.0.is_null() && !self.freetype_face.is_null() {
                 assert_eq!(FT_Done_Face(self.freetype_face), 0);
             }
-        }
+        });
     }
 }

and

diff --git a/src/loaders/freetype.rs b/src/loaders/freetype.rs
index 1ef0698..d6290f9 100644
--- a/src/loaders/freetype.rs
+++ b/src/loaders/freetype.rs
@@ -987,11 +987,11 @@ impl Clone for Font {

 impl Drop for Font {
     fn drop(&mut self) {
-        unsafe {
+        let _ = FREETYPE_LIBRARY.try_with(|_| unsafe {
             if !self.freetype_face.is_null() {
                 assert_eq!(FT_Done_Face(self.freetype_face), 0);
             }
-        }
+        });
     }
 }