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

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

Closed
Nudelmeister opened this issue Apr 7, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@Nudelmeister
Copy link

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 mmghannam added the bug Something isn't working label Apr 7, 2024
@mmghannam
Copy link
Member

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

Andful commented Aug 23, 2024

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

Fixed in #149

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants