Crash related to lifetime problem when creating Font from RWops.

[alexandermorozov]

As far as I understand RWops must outlive Font, since C library uses them after font is created.

In previous version I've just kept RWops around and bug wasn't triggered. In 0.14 method Sdl2TtfContext::load_font_from_rwops() consumes RWops, so they are dropped before the function returns and program crashes without traceback.

It's possible to add a lifetime to Font so it won't outlive RWops.

Another option is to use SDL's internal lifetime management: if second arg in ffi::TTF_OpenFontRW() is 1, then C library takes ownership of RWops and releases them when it's done. In this case rust shouldn't drop RWops since ownership is transferred to C. It's possible to just std::mem::forget(rwops) (and it works), but I think it's not a good idea. It would be better to add RWops::into_raw(self) -> ll::SDL_RWops in rust-sdl2.

What do you think?

Edit: it looks like RWops have internal state, at least current position. It means that RWops shouldn't be shared between different fonts (docs are not clear on this). So rwops should be either mutably borrowed or ownership fully transferred.

[Cobrand]

Where did you see that RWops had an internal state ? I can't seem to find that part in the SDL docs ...

And when you mean "RWops shoudln't be shared between different fonts", do you mean different font sizes, different font faces or both ?

[alexandermorozov]

RWops has to keep current position in file, so it has state that is implicitly modified by reads and writes. About sharing RWops between fonts I was wrong: I've checked sdl2-ttf source now and every read() is preceeded by seek(), so state is reset before every access.

[Cobrand]

Well then the problem now is that rwops is consumed when the font is loaded, meaning that it calls "Drop" and then destroy the object, but if we can immutably borrow it the rwops with "load_font_from_rwops", the rwops object will never be dropped (since the lifetime is static). So I guess my PR still stands and kills two bugs in one stone ?

[alexandermorozov]

Sorry for late reply, do you mean PR #37? Passing the reference is better than moving, but it's still unsafe: RWOps can be dropped before Font does, resulting in segfault.

I'm not sure how to resolve the issue in a straightforward way... sdl2-ttf modifies current position in the file, so RWOps should be owned by Font or mutably borrowed with appropriate lifetime annotations. But that's suboptimal, since each size/bold/italics combination will hold its own opened file. Actually sdl2-ttf always sets file position before read(), so RWOps can be borrowed immutably, but only if they are used within sdl2-ttf and nowhere else. This can be enforced with a wrapper type but will uglify API.

[Cobrand]

Or we could add our own implementation or RWops for sdl2_ttf, which would make sure it is not used anywhere else. Instead of the field owned:bool in Font, we could replace it by a reference to a custom RWops : we would then have something 100% safe.

Ideally it would be more convenient to have a Trait RWops in rust-sdl2 so we could use the same trait and the same impl, but with different names so it cannot be used the same way by sdl2_ttf and something else, but I am not sure such a change would be accepted at rust-sdl2 only to have a sub-library more secure (and to make our lives easier).

[alexandermorozov]

It's likely that this problem is specific to rust-sdl2ttf, so patching rust-sdl2 won't bring any benefits. Though I haven't surveyed sibling crates (rust-sdl2*) to make sure.

Re local RWOps implementation, it could be something like this:

#[derive(clone)]
struct RWopsWrapper {
    rwops: Rc<RWops>,
}

impl Into<RWopsWrapper> for RWops {
    fn into(rwops: RWops) -> RWopsWrapper {
        RWopsWrapper { rwops: Rc::new(rwops) }
    }
}
...
fn load_font_from_rwops<T: Into<RWopsWrapper>>(..., rwops: T, ...) {
    ...
    # keep rwops inside font in Option<>
}

impl Drop for Font {
  ... # check that deinitialization is done before rwops are dropped
}

It's also possible to use C library's internal refcounting instead of bolting on rust's Rc, and use RWopsWrapper just to make sure that RWops instance is properly wrapped and so won't be used by functions from other crates later. Looks like it's a better solution.

BTW currently Font.owned is always set to true and I don't quite understand what its intention was and if it can be replaced by Rc<> like above. Or if it's about something else; need to dig docs...

[Cobrand]

Maybe @andelf can bring us some insight about what was "owned" for and what were its use cases ?

I like the idea of Into<RwopsWrapper>, and since Rc<_> impls Deref from the get go we wouldn't have anything else to do, it should be working as usual ! I did not think of that and it is a brilliant idea.

However I do not understand the # keep rwops inside font in Option<>. You want to keep a Option<RWopsWrapper> in every Font, is that right ? Did I understand well ? If that is the case, why an Option ? In which cases would the Option be None instead of Some ?

[Cobrand]

Update : when the same RWops is used by 2 fonts, a crash happens. It works fine when a RWops is used by one and only one font. I will submit a PR changing &RWops into &mut RWops.

Now what we could do instead is take care ourselves of the RWops object. Since a RWops can be used by only one font, why not include an Option in said font ?

For now I will make a very simple PR avoiding multiple font using the same RWops, but I'm sure we could improve it even better.

[monkey0506]

I landed here from Google while trying to understand this precise problem:

As far as I understand RWops must outlive Font, since C library uses them after font is created.

This behavior isn't apparently documented, so I filed an SDL bug report. I'm not using Rust, but wanted to share my thanks and findings for anyone else coming across this.