scipopt / russcip

Rust interface for SCIP
https://crates.io/crates/russcip
Apache License 2.0
37 stars 11 forks source link

segfault (use after free) when accessing solution after model is dropped #138

Closed Nudelmeister closed 3 weeks ago

Nudelmeister commented 6 months ago
use russcip::prelude::*;

fn main() {
    // Create model
    let mut model = Model::new()
        .hide_output()
        .include_default_plugins()
        .create_prob("test")
        .set_obj_sense(ObjSense::Maximize);

    // Add variables
    let x1 = model.add_var(0., f64::INFINITY, 3., "x1", VarType::Integer);
    let x2 = model.add_var(0., f64::INFINITY, 4., "x2", VarType::Integer);

    // Add constraints
    model.add_cons(
        vec![x1.clone(), x2.clone()],
        &[2., 1.],
        -f64::INFINITY,
        100.,
        "c1",
    );
    model.add_cons(
        vec![x1.clone(), x2.clone()],
        &[1., 2.],
        -f64::INFINITY,
        80.,
        "c2",
    );

    let sol = model.solve().best_sol().unwrap(); // Temporary value returned from `model.solve()` is dropped.

    dbg!(sol); // <-- Segfault here
}

From looking over the russcip code the cause seems obvious. ScipPtr's drop impl is responsible for cleaning up and freeing, but Solution also holds the same raw pointer to Scip, causing a use after free.

My first thought for a fix would be to remove the pointer aliasing, only ScipPtr should hold the pointer to Scip. ScipPtr could then be reference counted. Alternatively, change the API so that it more closely mimics that of scip and require passing the ScipPtr separately.

mmghannam commented 6 months ago

Thanks a lot for the report and the suggested fix! I think I like the solution of making ScipPtr count the given references as it gives more freedom for russcip and would allow for more ergonomic use. I will work on it as soon as I can :)

Andful commented 1 month ago

A (zero-cost) alternative is to add a lifetime parameter. That would be API breaking and less ergonomic, but variable lifetimes seems to be the root cause of the problem.

So the signature of Solution would become:

Solution<'a>

But I might be missing some intricacies of the libreary

mmghannam commented 3 weeks ago

Fixed in #149