Possible UB in get_api function
#53
Comments
|
Hi, Yes, if what the reference points to is indeed dynamically allocated and is freed when the mainloop object is dropped, then the example you give presents a use-after-free vulnerability. It's been quite a while since I put this together and I can't recall exactly why I set the lifetime up this way. I clearly chose to deliberately separate the lifetime of the returned api reference from the lifetime of the mainloop object reference, but why escapes my recollection. I think either:
I'm leaning towards the second possibility. With respect to it I recall that the rust borrow checker became smarter several releases ago; I believe it now can assess what attributes of an object are being borrowed, thus allowing more code to compile. So perhaps the issue of an api reference blocking use of mutable reference methods is no longer an issue? So:
|
|
So I looked into it a bit more, and it maybe seems like the thing that it points to is maybe not freed when the mainloop object is dropped. I tried this code let mut mainloop = Mainloop::new().expect("Failed to create Mainoop");
let api: &MainloopApi = mainloop.get_api();
drop(mainloop);
api.quit.unwrap()(api as *const MainloopApi, 0 as i32);And I got the following error message:
This is indeed an error, but not a use-after free. Looks like the API vtable object is still around, because I'm able to call |
|
Another concern is, even if the API object is still around, the associated data might not be. There is a field in I have no idea what that would look like from the C side though. |
|
Taking a peek into here, ASAN and Valgrind cried foul on the sample program vrnst made. use libpulse_binding::mainloop::{api::MainloopApi, standard::Mainloop};
fn main() {
let mut mainloop = Mainloop::new().expect("Failed to create Mainoop");
let api: &MainloopApi = mainloop.get_api();
drop(mainloop);
api.quit.unwrap()(api as *const MainloopApi, 0_i32);
}And the angry Valgrind gave: @jnqnfe Something is defo wrong here. |
|
@powpingdone - thanks for the report. Just wondering, does Valgrind still complain if you remove the line with |
|
|
I think I just ran into this bug myself. I’m never actually calling I have a pretty simple example: use libpulse_binding as pulse;
use libpulse_glib_binding as pulse_glib;
use std::cell::RefCell;
use std::rc::Rc;
fn do_stuff() {
let ml = pulse_glib::Mainloop::new(None).expect("mainloop new failed");
let ctx = pulse::context::Context::new(&ml, "foo").expect("context new failed");
let ctx = Rc::new(RefCell::new(ctx));
let ctx_clone = ctx.clone();
ctx.borrow_mut().set_state_callback(Some(Box::new(move || {
if let Ok(borrow) = ctx_clone.try_borrow() {
println!("State CB: state is {:?}", borrow.get_state());
} else {
println!("State CB: cannot check state, context is borrowed");
}
})));
let ctx_clone = ctx.clone();
glib::source::timeout_add_local(std::time::Duration::from_secs(1), move || {
if let Ok(borrow) = ctx_clone.try_borrow() {
println!("Timeout CB: state is {:?}", borrow.get_state());
} else {
println!("Timeout CB: cannot check state, context is borrowed");
}
glib::ControlFlow::Continue
});
println!("Call connect.");
ctx.borrow_mut().connect(None, pulse::context::FlagSet::NOFLAGS, None).expect("connect failed");
println!("Connect returned, state is {:?}", ctx.borrow().get_state());
//std::mem::forget(ml);
}
fn main() {
let ml = glib::MainLoop::new(None, false);
glib::source::idle_add_local_once(do_stuff);
ml.run();
}Run this as is, and the output is as follows: Uncomment the So, clearly the context is not hanging onto the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hi,
In
libpulse_binding::mainloop::api::standard::Mainloop(this line) andlibpulse_binding::mainloop::api::threaded::Mainloop(this line), there is the functionget_apiwhich returns a borrow to the API vtable. I think the lifetime annotations of this function are incorrect and could lead to unexpected behavior.The returned borrow can live even after the Mainloop is freed and the Api is destroyed. As the function's docstring says, "No need to free the API as it is owned by the loop and is destroyed when the loop is freed."
I believe that something like this shouldn't be allowed to compile:
Changing the function signature to this would solve the issue and prevent the above code from compiling.
The text was updated successfully, but these errors were encountered: