Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v0.12 SIGSEGV: invalid memory reference #224

Closed
jayvdb opened this issue Dec 2, 2023 · 1 comment · Fixed by #225
Closed

v0.12 SIGSEGV: invalid memory reference #224

jayvdb opened this issue Dec 2, 2023 · 1 comment · Fixed by #225

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Dec 2, 2023

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 f9b1876 . 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
Copy link
Contributor Author

jayvdb commented Dec 2, 2023

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);
             }
-        }
+        });
     }
 }
 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant