eclipse-zenoh / zenoh

zenoh unifies data in motion, data in-use, data at rest and computations. It carefully blends traditional pub/sub with geo-distributed storages, queries and computations, while retaining a level of time and space efficiency that is well beyond any of the mainstream stacks.
https://zenoh.io
Other
1.43k stars 151 forks source link

fix: replace some unsafe code with safe version #1184

Closed wyfo closed 3 months ago

wyfo commented 3 months ago

Compiler is able to optimize bound checks based on previous checks See https://godbolt.org/z/oGesnb6a4 or https://godbolt.org/z/c6c41bvE5

Writer::with_slot has been made unsafe, because its implementations rely on a precondition on the write callback

wyfo commented 3 months ago

@Mallets @kydos

wyfo commented 3 months ago

@fuzzypixelz

wyfo commented 3 months ago

By the way, since 1.78, Rust has enabled debug of the standard library, so <[T]>::get_unchecked now trigger an error in tests if they are not built with --release.

That's why we may want to remove zenoh_buffers::unsafe_slice and other macros of the same kind, as they are no more "necessary". There are two caveats:

Do we care having the check on release build for tests? If we do, I will put back unsafe_slice! slice in the places where I used get_unchecked instead.

wyfo commented 3 months ago

Also, I've one (big) issue with unsafe_slice macro, which is that it doesn't require unsafe block. It make it harder to track unsafe code. I know that it is more convenient to write, but I don't think it's a good argument when it comes to unsafe code, which requires more cautious care than convenience.

So, if we keep unsafe_slice, I would like to remove the unsafe block from it to for us writing unsafe { crate::unsafe_slice!(s, ..) }.

Mallets commented 3 months ago

By the way, since 1.78, Rust has enabled debug of the standard library, so <[T]>::get_unchecked now trigger an error in tests if they are not built with --release.

That's why we may want to remove zenoh_buffers::unsafe_slice and other macros of the same kind, as they are no more "necessary". There are two caveats:

  • as written in the 1.78 release, these debug assertion are not stable (but I think we can still rely on <[T]>::get_unchecked one)
  • they are removed from --release build, contrary to the "test" feature

Do we care having the check on release build for tests? If we do, I will put back unsafe_slice! slice in the places where I used get_unchecked instead.

At the moment Zenoh uses Rust 1.75. I agree with your comment once we bump Rust to 1.78.

wyfo commented 3 months ago

At the moment Zenoh uses Rust 1.75. I agree with your comment once we bump Rust to 1.78.

In my head, the CI was using stable, but I realized that it was only for checks and not for tests 🤦 So I agree

fuzzypixelz commented 3 months ago

@wyfo @Mallets I would like to review this before it gets merged :)

wyfo commented 3 months ago

Even though this pull request doesn't affect it, zenoh_buffers::vec::uninit is unsound.

We know that, it's a huge problem, but this is not the subject of this PR. The goal is to split the work into small PRs.