memory safety issue: free(): invalid pointer

[lovasoa]

Hello,
Recently a CI run in good_lp returned the following, I think this is a memory safety bug in the coin_cbc interface. I cannot reproduce the issue on my machine, but here is the CI run:

https://github.com/rust-or/good_lp/pull/2/checks

---- src/variables_macro.rs - variables (line 14) stdout ----
Test executable failed (terminated by signal).

stdout:
Dual infeasible - objective value 0
DualInfeasible objective 0 - 0 iterations time 0.002

stderr:
free(): invalid pointer

github actions runner logs.zip

good_lp uses only safe functions in coin_cbc.

[lovasoa]

The memory corruption was caused by solving the following (nonsensical, I admit) problem :

 variables!{problem:
     0 <= x;  // Create a variable x that varies between 0 and +∞
     0 <= y <= 10;  // y varies between 0 and 10
     z; // z varies between -∞ and +∞
 } // x, y, and z are now three rust variables that are in scope
 problem.minimise(x + y + z).using(default_solver).solve();

The problem is unbounded, but this still shouldn't cause a memory error.

[TeXitoi]

It looks more like a bug in coin than anything the rust binding can fix.

[lovasoa]

Were you able to reproduce the issue in coin ? I cannot reproduce it.

[TeXitoi]

I didn't try anything, but the binding doesn't allocate anything that coin must free.

[lovasoa]

Given that the issue is not deterministic, maybe the root cause is : coin-or/Cbc#223

[TeXitoi]

If you have this in tests, and you have several tests in parallel, there is chance this is the problem yes.

[lovasoa]

You may want to add a big red warning in the README and the doc of this crate, then. For a rust crate, this is a kind of serious problem.

[lovasoa]

Reading coin-or/Cbc#332 (comment) , it looks like there is a build parameter that is disabled by default called -DCBC_THRED_SAFE. Maybe this crate should statically link to a cbc compiled with this option ?

[lovasoa]

Otherwise, this crate could add a Mutex around the cbc pointer.

[dlaehnemann]

@lovasoa Here are suggestions how to work around this (basically get a version compiled with the flag you mention), until this is fixed in a proper release -- which will probably happen with the next Cbc release:
#7 (comment)

[lovasoa]

Thanks ! But until cbc itself is thread safe, this crate should probably prevent solving multiple problems in parallel.

[TeXitoi]

I can't without a global variable, and it would block using cbc in parallel if you have a thread safe cbc.

A note about this issue can be added in the README.md and in the doc, PR welcome.

[dlaehnemann]

Hmm, I prefer being able to run multiple instances of Cbc in parallel and will use the workaround. But this obviously requires explicitly using one of the workarounds.

And I agree that running into this repeatedly is annoying. So maybe putting this into the README somewhere prominently is a good idea, until this is fixed in Cbc.

[lovasoa]

I don't think this requires a global variable. It just requires wrapping the cbc model pointer in a mutex, doesn't it ? Maybe the crate could have a feature flag called unsafe-parallel to remove the mutex and allow parallel solving if the user has a non-standard cbc compiled with the thread safe feature flag ?

[TeXitoi]

It need a global variable, the problem is when you use several cbc model pointer.

[lovasoa]

I just tried rerunning good_lp's tests, and unfortunately this doesn't fix the problem 😢

Here is a minimal reproduction :

    #[test]
    fn unbounded() {
        // Formulate an unbounded problem and try to solve it
        let mut m = Model::default();
        let z = m.add_col();
        m.set_obj_coeff(z, 1.);
        m.set_col_lower(z, f64::NEG_INFINITY); // This causes the invalid free()
        m.set_obj_sense(Sense::Minimize);
        m.solve();
    }

I was so sure it indeed came from the concurrency bug that I didn't even try that before.

[TeXitoi]

With no parallelism? and setting it to -1e100 fixes the problem?

[lovasoa]

Yes, with a single thread. -1e100 still fails, but -1e27 works 🙁

[TeXitoi]

I can't really reproduce, but by insisting, I have segfaults. So I thing that's not enough.

[lovasoa]

To be clear, you never observed #9 with libcbc 2.9 ? If so, this means coin-or/Cbc#367 is a regression

[TeXitoi]

Never observed.