KardinalAI / coin_cbc

Rust bindings to the CoinOR CBC MILP Solveur using the C API.
MIT License
16 stars 14 forks source link

soundness bug #4

Closed lovasoa closed 3 years ago

lovasoa commented 3 years ago

The following safe rust program :

#[test]
fn test() {
    use coin_cbc::{raw, Col, Model};
    let mut m = raw::Model::new();
    let numcols = 0;
    let numrows = i32::MAX as usize;
    let start = [1];
    let value = [0.];
    m.load_problem(
        numcols, numrows, &start, &start, &value, None, None, None, None, None,
    );
    m.set_initial_solution(&[]);
    assert_eq!(&value, &[0.])
}

when running cargo test --release results in

test test ... test test has been running for over 60 seconds
error: test failed, to rerun pass '--test coin_cbc_unsound'

Caused by:
  process didn't exit successfully: `[...]/target/release/deps/coin_cbc_unsound-50c23e196a10e5a1` (signal: 11, SIGSEGV: invalid memory reference)

I tried that while poking around coin_cbc::raw, which seems to to some arithmetic without checking overflows, does some type casting with as, and doesn't check for pointer aliasing. I didn't really have the time to investigate deeper, but I think there may be more memory unsafety exposed as safe rust functions in coin_cbc::raw.

lovasoa commented 3 years ago

It may be interesting to fuzz this library with cargo-fuzz, for instance.

TeXitoi commented 3 years ago

There is 2 aspects on this:

Looking at your example, I suspect the numrows is very high, and that coin might do some crazy allocations without checking the result, breaking everything. But I don't know what I can do about that, there is no assert that can do to check that.

For as usage, I can change them to try_into().unwrap() everywhere. For arithmetic, there is only 2 +1. Can do some .checked_add(1).unwrap() to be sure.

Using cargo-fuzz will not be very interesting: you might generate very difficult problems that takes ages to solve, blocking everything, and you will surely find bugs in coin, not in the binding.

TeXitoi commented 3 years ago

I've run your test, and it has been killed by the linux OOM killer.

TeXitoi commented 3 years ago

Arithmetic and conversion checks added. If you see any other possible soundness bugs that I can fix (i.e. in the bindings, and not in cbc itself), tell me. If you are OK with the result, please close the issue.

lovasoa commented 3 years ago

Thanks for reacting so fast !