danielpclark / rutie

“The Tie Between Ruby and Rust.”
MIT License
939 stars 62 forks source link

Raising a Ruby error from Rust silently leaks resources #159

Open georgeclaghorn opened 2 years ago

georgeclaghorn commented 2 years ago

rb_raise uses libc longjmp under the hood to enter the Ruby VM's exception handling code. This is how raising a Ruby exception short-circuits execution of the current function in C and Rust. Pretty cool!

Importantly for Rust, though, this bypasses the drops that the compiler inserts for owned values at the end of each wrapping scope. This means anything in scope when Rutie VM::raise is called will not be dropped. The result is leaked memory, file descriptors, sockets, mutexes, you name it—anything practicing RAII and relying on automatic Drop::drop calls at scope boundaries to clean up.

Consider the following minimal Ruby native extension written with Rutie:

use rutie::*;

struct Wrapper {
    #[allow(dead_code)]
    string: String
}

impl Drop for Wrapper {
    fn drop(&mut self) {
        println!("Dropping Wrapper");
    }
}

module!(Sandbox);

methods!(
    Sandbox,
    _itself,

    fn example(string: RString, raise: Boolean) -> NilClass {
        println!("Allocating Wrapper");

        let _wrapper = Wrapper { string: string.unwrap().to_string() };

        if raise.map(|raise| raise.to_bool()).unwrap_or(false) {
            VM::raise(Class::from_existing("StandardError"), "kaboom");
        }

        NilClass::new()
    }
);

#[no_mangle]
pub extern "C" fn Init_sandbox() {
    Module::new("Sandbox").define(|module| {
        module.def_self("example", example);
    });
}

If I run Sandbox.example "test", false in IRB, the following is printed:

irb(main):002:0> Sandbox.example "test", false
Allocating Wrapper
Dropping Wrapper                                         
=> nil                                                   
irb(main):003:0>

No exception is raised, so Sandbox.example allocates and properly drops a Wrapper struct.

If I run Sandbox.example "test", true, however, the result is this:

irb(main):003:0> Sandbox.example "test", true
Allocating Wrapper
(irb):3:in `example': kaboom (StandardError)             
        from (irb):3:in `<main>'                         
        from /Users/george/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/irb-1.4.1/exe/irb:11:in `<top (required)>'
        from /Users/george/.rbenv/versions/3.1.0/bin/irb:25:in `load'
        from /Users/george/.rbenv/versions/3.1.0/bin/irb:25:in `<main>'
irb(main):004:0> 

Yikes. Note that, unlike in the previous example, Dropping Wrapper is not printed. Sandbox.example allocated a Wrapper struct, but because it subsequently raised an exception via VM::raise, the Wrapper was not dropped. 🚨

To further demonstrate, if I run loop { Sandbox.example "a" * 1_000_000, true rescue nil }, the IRB process’s memory usage grows unbounded. This does not happen if I pass false as the second argument to Sandbox.example.

image

One possible solution: the PyO3 library, which provides Rust bindings for Python, deals with this by requiring Rust implementations of Python functions to return PyResult, similar to Rust standard library Result. Ok values are treated as returns. Err values are treated as exceptions and safely raised by the library. This seems idiomatic to me, but I’m not sure how tractable or palatable it is for Rutie.

danielpclark commented 2 years ago

Thank you @georgeclaghorn . I'll have to add this to the test suite and think about a solution. First thing that comes to mind is when a Ruby application closes there is some finally code that can be run... so I believe something like this might be implemented per thread (which is how raising works in Ruby by interrupting the current thread).

As far as a result return type that may not be easy to implement with Ruby if you're talking about catching exceptions because raised exceptions interrupt the current thread process one would have to write code to look for that, but then if you have multiple layers of Ruby code trying to catch then you run into the issue of multiple handlers for that scenario.

But this is definitely worth investigating and resolving.