search

scipopt / russcip

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

undefined behavior #8

Closed lovasoa closed 1 year ago

lovasoa commented 1 year ago

Isn't this undefined behavior ?

https://github.com/mmghannam/russcip/blob/cdda8e7142d7bf62769eafc5cd58fd27eb305045/src/model.rs#L21

zero-initializing a variable of reference type causes instantaneous undefined behavior https://doc.rust-lang.org/std/mem/union.MaybeUninit.html

In general, the usage of mem::zeroed everywhere should probably be replaced with MaybeUninint.

Maybe you could add a CI check to test this library with miri ?

mmghannam commented 1 year ago

Thanks! I fixed this in #12. About miri, it currently doesn't support FFI, so I don't think it would be particularly helpful in this repo.

lovasoa commented 1 year ago

Oh, I see. Then maybe just run the tests with an address sanitizer ? https://doc.rust-lang.org/beta/unstable-book/compiler-flags/sanitizer.html (And add test cases with more diverse resource usage patterns, solving multiple problems, dropping problems but not solutions or variables...)

And you can add a fuzzing target; that's what I did for the HiGHS bindings: https://github.com/rust-or/highs/blob/main/fuzz/fuzz_targets/fuzz_target_1.rs

Looking at the code now, I'm still not convinced it does not expose undefined behavior to safe rust.

mmghannam commented 1 year ago

@lovasoa Now with the use of ScipPtr, the type state design and the memory leak test is in the CI, do you think we can safely close this now?

lovasoa commented 1 year ago

Yes, the code looks much better now, @mmghannam !

I don't have the time to make a real audit, but at least the code structure is clean, and I don't see any obvious safety problems.

I think we can close this, and we'll reopen specific issues if we find problems later.

I think we can resume the work on integrating with good-lp :smiley:

mmghannam commented 1 year ago

Great, thanks a lot for your help so far!