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

Raising a Ruby error from Rust silently leaks resources #159

Open
georgeclaghorn opened this issue May 15, 2022 · 1 comment
Open

Raising a Ruby error from Rust silently leaks resources #159

georgeclaghorn opened this issue May 15, 2022 · 1 comment

Comments

@georgeclaghorn
Copy link

georgeclaghorn commented May 15, 2022

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
Copy link
Owner

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.

pawandubey added a commit to pawandubey/troml that referenced this issue Jun 22, 2022
Right now, we just raise an exception on parse failures. Rutie apparently leaks memory on VM::raise calls,
so this might need to be rethought at some point.

ref. danielpclark/rutie#159
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