scipopt / russcip

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

add memory safety tests #15

Closed lovasoa closed 1 year ago

lovasoa commented 1 year ago

These tests show a few examples of how the interface currently exposed is unsafe. This illustrates #8

After spending some time looking at the code, it looks like you tried to model the C interface very closely, which is not a good thing.

An advice would be to spend some time reading very carefully the C interface documentation, taking special care about preconditions and postconditions for every method, when memory should be allocated and released, and trying to come up with a rust interface that makes illegal states irrepresentable.

lovasoa commented 1 year ago

Ideally, there would be a large number of this kind of "unusual but safe" tests that ensure the library can never expose memory-unsafe functionality under its safe interface.

lovasoa commented 1 year ago

One thing you can do to make your life easier is to forbid some method calls at the type level. See https://willcrichton.net/rust-api-type-patterns/state_machines.html

It looks like scip maintains a state machine internally. These states should probably be reflected at the type level in rust. Instead of a single Model type, there should probably be at least an UnsolvedModel and a SolvedModel, and probably more.

This should also help you have less Result in your return types. If you can guarantee at the type level that a particular C function call will not fail, then you can turn a faillible_call()? into a faillible_call().expect("We are in state X, which meets all the preconditions of function faillible_call")

mmghannam commented 1 year ago

Thanks a lot for the tests and for the suggestions. I have added issue #19 to keep track of this and fix it in a later pull request.