Rust: Document all unsafety used in libcore and libstd

[oli-obk]

The repository still contains many instances of

// ignore-tidy-undocumented-unsafe

We should eliminate all of them by documenting the unsafe used in those files.

Documenting unsafe blocks works by adding a // SAFETY: comment infront of them explaining why the unsafe block is ok. If there is an explanation about the unsafety elsewhere in the file, you can also leave a // SAFETY: comment explaining that the overarching logic is explained elsewhere (and mention where, too!)

cc @Centril

not sure what labels to add to this issue.

See https://github.com/rust-lang/rust/pull/63793/files for examples and the introduction of the unsafety tidy check

[foeb]

Hi! I'm interested in working on this.

[foeb]

@rustbot claim

[LeSeulArtichaut]

@foeb I took the liberty of opening #71063. I hope you weren't working on those 3 files 😅

[hbina]

Hello,

Is anyone working on https://github.com/rust-lang/rust/blob/2dc5b602eee35d70e8e6e506a7ea07b6c7e0197d/src/libcore/slice/sort.rs#L35-L55

This can be shown to be safe because len > 2 => len - 2 > 0, right?

[foeb]

I haven't had the time to work on this for a while, so I don't think it's right for me to hold on to this assignment.

@rustbot release-assignment

[LeSeulArtichaut]

I think I'll assign myself then?
@rustbot claim

@hbina Please take src/libcore/slice/sort.rs if you want, I wasn't working on anything, and I think @foeb wasn't either 🙂. However I see a lot of unsafe functions, I think you want to go to their documentation and prove that their safety contract is upheld. For most of them, there is a "Safety" category listing everything to prove the call to the function safe.
In that example you gave, you'd need to prove that all calls to get_unchecked are within the slice, that copy_nonoverlapping is not called on overlapping memory, etc...

[LeSeulArtichaut]

Here's a list of all the files we have left to document. We can use it to synchronize and track our progress.

• [x] src/libcore/panicking.rs (@LeSeulArtichaut, #71492)
• [x] src/libcore/option.rs (@LeSeulArtichaut, #71063)
• [x] src/libcore/hint.rs (@LeSeulArtichaut, #71492)
• [x] src/libcore/alloc/layout.rs (@LeSeulArtichaut, #71492)
• [x] src/libcore/hash/mod.rs (@LeSeulArtichaut, #71063)
• [x] src/libcore/hash/sip.rs (@LeSeulArtichaut, #71063)
• [ ] src/libcore/slice/mod.rs (@LeSeulArtichaut, #73555)
• [ ] src/libcore/slice/sort.rs (@hbina, #71568)
• [ ] src/libcore/slice/memchr.rs
• [x] src/libcore/ptr/mut_ptr.rs (@LeSeulArtichaut, #71760)
• [x] src/libcore/ptr/const_ptr.rs (@LeSeulArtichaut, #71760)
• [x] src/libcore/ptr/unique.rs (@CohenArthur, #71507)
• [x] src/libcore/ptr/non_null.rs (@CohenArthur, #71507)
• [x] src/libcore/ptr/mod.rs (@CohenArthur, #71507)
• [x] src/libcore/mem/maybe_uninit.rs
• [x] src/libcore/iter/adapters/zip.rs (@LeSeulArtichaut, #71492)

[CohenArthur]

@LeSeulArtichaut I'd like to help you with this. I've started on src/libcore/ptr/mod.rs so far.

[LeSeulArtichaut]

Thanks! I added you in the list so we don't do duplicate work (😅)

[CohenArthur]

@LeSeulArtichaut I've completed the following files.

• src/libcore/ptr/mod.rs
• src/libcore/ptr/unique.rs
• src/libcore/ptr/non_null.rs

I'm not quite sure yet as to how to proceed for the remaining files in src/libcore/ptr

I've opened #71507 to get some feedback

[hbina]

I will take a look at src/libcore/slice/sort.rs. I will need some help tho :)

[LeSeulArtichaut]

@hbina I can probably help! But I'm not an expert, if you need more help you can probably reach people out on Discord or on Zulip 🙂

[hbina]

Is there a reason why a simple swap is not used here? Seems overly complicated for what its trying to do...

    unsafe {
        // If the first two elements are out-of-order...
        if len >= 2 && is_less(v.get_unchecked(1), v.get_unchecked(0)) {
            // Read the first element into a stack-allocated variable. If a following comparison
            // operation panics, `hole` will get dropped and automatically write the element back
            // into the slice.
            let mut tmp = mem::ManuallyDrop::new(ptr::read(v.get_unchecked(0)));
            let mut hole = CopyOnDrop { src: &mut *tmp, dest: v.get_unchecked_mut(1) };
            ptr::copy_nonoverlapping(v.get_unchecked(1), v.get_unchecked_mut(0), 1);

            for i in 2..len {
                if !is_less(v.get_unchecked(i), &*tmp) {
                    break;
                }

                // Move `i`-th element one place to the left, thus shifting the hole to the right.
                ptr::copy_nonoverlapping(v.get_unchecked(i), v.get_unchecked_mut(i - 1), 1);
                hole.dest = v.get_unchecked_mut(i);
            }
            // `hole` gets dropped and thus copies `tmp` into the remaining hole in `v`.
        }
    }

I guess its to prevent creating runtime stack?

This file also contain a specialized data structure

struct CopyOnDrop<T> {
    src: *mut T,
    dest: *mut T,
}

impl<T> Drop for CopyOnDrop<T> {
    fn drop(&mut self) {
        unsafe {
            ptr::copy_nonoverlapping(self.src, self.dest, 1);
        }
    }
}

Can anyone enlighten me on the safety here? In what ways can ptr::copy_nonoverlapping fails? afaik, it could be unsafe if:

1. src or dest are invalid pointers
2. src or dest overlaps
3. the count argument is too large for dest

But it internally does all this checking anyway...

[LeSeulArtichaut]

Can anyone enlighten me on the safety here? In what ways can ptr::copy_nonoverlapping fails? afaik, it could be unsafe if:

1. `src` or `dest` are invalid pointers

2. `src` or `dest` overlaps

3. the `count` argument is too large for `dest`

But it internally does all this checking anyway...

ptr::copy_nonoverlapping doesn't really check anything. The check that you see are debug_assert! macros, that are only run for debug builds. No, as the caller of any unsafe function, you have to follow the "contract" of that fonction, here, the 3 things mentionned in the documentation.
In fact, if you don't follow those rules, the function doesn't just fail, it executes as it would for valid inputs, but they are invalid so you get UB (Undefined Behaviour).

In the code you gave, I think that for the calls to copy_nonoverlapping, you can say that the pointers are valid since they point to the slice that you're sorting (guaranteed to be valid by the reference), they are non-overlapping since they point to different elements of the slice, and that the count argument is 1.

As for why this implementation is so complicated, I have no answer. Maybe you can find something if you dive into the file history?

[hbina]

I don't think I will be able to properly document the safety of https://github.com/rust-lang/rust/blob/0862458dad90a0d80827e22e3f86e33add6d847c/src/libcore/slice/sort.rs#L195

I have no understanding of the algorithm. Trying to read the paper but I haven't read academical paper in a while :(

[RalfJung]

I did maybe_uninit.rs as part of https://github.com/rust-lang/rust/pull/76217, not realizing that this was still an ongoing effort.^^ I checked the box above.