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

Weird panic inside the xi-unicode crate, perhaps relating to UTF-8 encoding #124

Closed
tuzz opened this issue Dec 26, 2020 · 4 comments
Closed

Comments

@tuzz
Copy link

tuzz commented Dec 26, 2020

Hello, I'm using glyph-brush to render text to a framebuffer for a game that I'm making.

Occasionally, the game is crashing due to a panic deep inside the xi-unicode crate which glyph-brush is using.

Backtrace:

thread 'main' panicked at 'attempt to subtract with overflow', /Users/tuzz/.cargo/registry/src/github.com-1ecc6299db9ec823/xi-unicode-0.3.0/src/lib.rs:55:18
stack backtrace:
   0: rust_begin_unwind
             at /rustc/1f5bc176b0e54a8e464704adcd7e571700207fe9/library/std/src/panicking.rs:493:5
   1: core::panicking::panic_fmt
             at /rustc/1f5bc176b0e54a8e464704adcd7e571700207fe9/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/1f5bc176b0e54a8e464704adcd7e571700207fe9/library/core/src/panicking.rs:50:5
   3: xi_unicode::linebreak_property_str
             at /Users/tuzz/.cargo/registry/src/github.com-1ecc6299db9ec823/xi-unicode-0.3.0/src/lib.rs:55:18
   4: <xi_unicode::LineBreakIterator as core::iter::traits::iterator::Iterator>::next
             at /Users/tuzz/.cargo/registry/src/github.com-1ecc6299db9ec823/xi-unicode-0.3.0/src/lib.rs:105:37
   5: <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::next
             at /rustc/1f5bc176b0e54a8e464704adcd7e571700207fe9/library/core/src/iter/adapters/map.rs:99:9
   6: <alloc::boxed::Box<I,A> as core::iter::traits::iterator::Iterator>::next
             at /rustc/1f5bc176b0e54a8e464704adcd7e571700207fe9/library/alloc/src/boxed.rs:1262:9
   7: <glyph_brush_layout::characters::Characters<L,F,S> as core::iter::traits::iterator::Iterator>::next
             at /Users/tuzz/.cargo/registry/src/github.com-1ecc6299db9ec823/glyph_brush_layout-0.2.1/src/characters.rs:118:36
   8: <&mut I as core::iter::traits::iterator::Iterator>::next
             at /rustc/1f5bc176b0e54a8e464704adcd7e571700207fe9/library/core/src/iter/traits/iterator.rs:3292:9
   9: <glyph_brush_layout::words::Words<L,F,S> as core::iter::traits::iterator::Iterator>::next
             at /Users/tuzz/.cargo/registry/src/github.com-1ecc6299db9ec823/glyph_brush_layout-0.2.1/src/words.rs:109:14
  10: core::iter::adapters::peekable::Peekable<I>::peek::{{closure}}
             at /rustc/1f5bc176b0e54a8e464704adcd7e571700207fe9/library/core/src/iter/adapters/peekable.rs:216:43
  11: core::option::Option<T>::get_or_insert_with
             at /rustc/1f5bc176b0e54a8e464704adcd7e571700207fe9/library/core/src/option.rs:869:26
  12: core::iter::adapters::peekable::Peekable<I>::peek
             at /rustc/1f5bc176b0e54a8e464704adcd7e571700207fe9/library/core/src/iter/adapters/peekable.rs:216:9
  13: <glyph_brush_layout::lines::Lines<L,F,S> as core::iter::traits::iterator::Iterator>::next
             at /Users/tuzz/.cargo/registry/src/github.com-1ecc6299db9ec823/glyph_brush_layout-0.2.1/src/lines.rs:99:32
  14: <glyph_brush_layout::builtin::Layout<L> as glyph_brush_layout::GlyphPositioner>::calculate_glyphs
             at /Users/tuzz/.cargo/registry/src/github.com-1ecc6299db9ec823/glyph_brush_layout-0.2.1/src/builtin.rs:186:29
  15: glyph_brush::glyph_brush::GlyphBrush<V,X,F,H>::cache_glyphs::{{closure}}
             at /Users/tuzz/.cargo/registry/src/github.com-1ecc6299db9ec823/glyph_brush-0.7.1/src/glyph_brush.rs:288:29
  16: core::option::Option<T>::unwrap_or_else
             at /rustc/1f5bc176b0e54a8e464704adcd7e571700207fe9/library/core/src/option.rs:427:21
  17: glyph_brush::glyph_brush::GlyphBrush<V,X,F,H>::cache_glyphs
             at /Users/tuzz/.cargo/registry/src/github.com-1ecc6299db9ec823/glyph_brush-0.7.1/src/glyph_brush.rs:287:33
  18: glyph_brush::glyph_brush::GlyphBrush<V,X,F,H>::queue_custom_layout
             at /Users/tuzz/.cargo/registry/src/github.com-1ecc6299db9ec823/glyph_brush-0.7.1/src/glyph_brush.rs:193:28
  19: glyph_brush::glyph_brush::GlyphBrush<V,X,F,H>::queue
             at /Users/tuzz/.cargo/registry/src/github.com-1ecc6299db9ec823/glyph_brush-0.7.1/src/glyph_brush.rs:217:9
  20: engine::systems::text_render::TextRender::run
             at ./engine/src/systems/text_render.rs:87:17
  21: engine::run_engine::{{closure}}
             at ./engine/src/lib.rs:248:9
... (more lines omitted)

I have tried to investigate by adding some extra logging around this line in xi-unicode:

Before:

let cp = ((b as usize) << 6) + (s.as_bytes()[ix + 1] as usize) - 0x3080;

After:

let foo = ((b as usize) << 6) + (s.as_bytes()[ix + 1] as usize);
let bar = 0x3080;

if foo < bar {
    println!("xi-unicode is about to panic");

    let bytes = s.bytes().collect::<Vec<_>>();
    println!("bytes: {:?}, index: {:?}", bytes, ix);
}

let cp = foo - bar;

I then ran the game again until another panic occurred and it logged the following:

xi-unicode is about to panic
bytes: [34, 194, 173, 69, 153, 185, 178, 69, 111, 196, 12, 45, 255], index: 4

I then tried to build a str from these bytes in Rust and received the following error:

let bytes = vec![34, 194, 173, 69, 153, 185, 178, 69, 111, 196, 12, 45, 255];
let s = str::from_utf8(&bytes).unwrap();

// thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Utf8Error { valid_up_to: 4, error_len: Some(1) }',

It seems like these bytes are an invalid UTF-8 sequence. I tried putting them into this converter tool and it says "Error: Invalid UTF-8 detected". I tried removing all bytes except the first four and it successfully decodes as "E. I'm not sure what this string is supposed to say.

As far as I'm aware, my game isn't using any weird characters so I'm not really sure what's going on. I haven't yet been able to make it crash predictably every time. I don't think the panic is specific to any one text string but it's hard to say. It seems like glyph-brush might passing invalid UTF-8 to the xi-unicode crate.

I thought I'd ask here if you have any idea what's going and whether there's a bug in my code, glyph-brush or the xi-unicode crate? I can open an issue there as well but thought I'd ask here first.

Many thanks!

@alexheretic
Copy link
Owner

alexheretic commented Dec 26, 2020 via email

@tuzz
Copy link
Author

tuzz commented Dec 26, 2020

Thanks for encouraging me to dig further. I managed to replicate the problem and found an issue with my code that I have now fixed. Here's a quick explanation if you're curious.

Basically, it was a use-after-free bug and glyph-brush was pointing to invalid memory - hence the garbled UTF-8 and crashes. The lifetimes in my game are a bit tricky. I store strings against game entities but also store an index of glyph-brush Sections I've already queued so I can reuse the same section so that it isn't rendered more than once (unless the texture is resized, etc). The index has a lifetime that is strictly longer than a game entity's (which can be short-lived) but this is a problem because the glyph-brush Section holds an index to its string.

To workaround this, I intentionally leak the strings so that they become static and can be happily referenced by Section. I then manually clean up these leaked strings when I determine they are no longer used (i.e. when the game entity has been destroyed). This works fine most of the time, but in a couple of cases I was accidentally cleaning up strings when I shouldn't have been. There were two strings on my 'kill screen' which live for the duration of the entire game and should never be cleaned up.

Serves me right for using unsafe code! I'll close this now as it's resolved. Thanks!

@tuzz tuzz closed this as completed Dec 26, 2020
@alexheretic
Copy link
Owner

Wow that's wild, thanks for getting to the bottom of this & explaining!

There are OwnedSection, OwnedText variants sans-lifetimes that may help, though of course I don't 100% understand your use case. Maybe the extra allocation isn't acceptable etc.

@tuzz
Copy link
Author

tuzz commented Dec 27, 2020

Thanks, I'll check those out! They might be just what I need. 👍

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

No branches or pull requests

2 participants