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

Known issues and concerns w.r.t soundness and best practices #796

Closed
bluss opened this issue Apr 13, 2020 · 2 comments · Fixed by #903
Closed

Known issues and concerns w.r.t soundness and best practices #796

bluss opened this issue Apr 13, 2020 · 2 comments · Fixed by #903
Labels
bug information needed More information is needed to make progress

Comments

@bluss
Copy link
Member

bluss commented Apr 13, 2020

I'll try to write down the concerns about ndarray's unsafe code that I know about. In many cases this is about aligning better with the developing unsafe code guidelines for Rust, which have been in progress for a long time. It is possible to adopt best practices and fixes, before these guidelines are finished.

ndarray is written carefully and has had many incremental improvements to its internals, but like much unsafe Rust code in the wild, it's unavoidable that our authors's model of unsafe Rust might not correspond to the consensus and best practices of Rust right now. So we try to list concerns and improve piece by piece.

If you know a clear-cut soundness bug with a scope-limited fix, it can be reported directly as a separate bug and does not need to be listed here. Specific concerns in this issue should be researched more deeply before they are fleshed out and fixed in pull requests.

Uninitialized data 1

Possibly fixed by #798. Examples fixed by #805

Concern description

Some examples and some internal code suppose we can use &mut T to overwrite in unitialized data, but it is unclear if this is correct.

Possible remedy

Use raw views and traversal with raw pointers - now possible with Zip to overwrite unitialized elements

Uninitialized data 2

See issue #685
Partly mitigated by #802

Concern description

Is the uninitialized data constructor for Arrays the right API? Does it make sense to expose it, and how do we do it in a safe way?

As food for thought, consider that Vec::with_capacity(128) is a well-known and safe abstraction for dealing with unitialized data - in this case 128 elements of that - can we use this thinking for our arrays.

Ownership and aliasing through Raw Pointers

Fixed by #799

Concern description

The unique and shared ownership arrays have two access ways to the data - through self.ptr and through the owned data's pointer accessor. The Vec assumes unique ownership of its data (like the Box), so having an aliasing extra pointer is possibly problematic.

Possible remedy

When creating an Array from a Vec, "take apart" the Vec into data pointer, len, capacity and don't store it as a Vec but in a representation that shows the compiler that we no longer assert unique ownership through the Vec's data pointer.

Details and possible remedy for Arc<Vec<_>> are not known.

Further info

In principle, this problem is described in issue Kimundi/owning-ref-rs#49

@bluss
Copy link
Member Author

bluss commented Jan 25, 2021

Our swap needs to use a non-alias violating implementation - inspired by rust-lang/rust#81160 (? unclear if the same semantics apply to us - we use an aliasable owner for the allocation and a raw pointer for the head of the array).

@bluss
Copy link
Member Author

bluss commented Jan 30, 2021

This is fixed by #903 and by #902 - and when uninitialized is removed (it's now deprecated) #804

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug information needed More information is needed to make progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant