What is an appropriate way of making this unsafe code safe?

1

The following code produces the desired results. However, I've heard that some types of unsafe code, in the future, may be optimized-away and no longer produce the desired results. As such, I'd like to convert the following code from unsafe to safe (with the exception of the set_len function):

    fn scramble_next_group(mut self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<Option<GroupSender>> {
        //println!("[G{}] {} / {}", self.groups_rendered, self.read_cursor, self.file_len);
        let ptr = &mut *self as *mut Self;
        if self.read_cursor != self.file_len {
            //log::info!("Polling to create group {}", self.groups_rendered);
            let pqc = AssertSendSyncSafe::wrap(self.pqc.clone());
            let pqc = pqc.for_ref();

            let fx = unsafe { &(*ptr).header_inscriber };
            let this = &mut *self;
            let remaining = this.file_len - this.read_cursor;
            let poll_amt = std::cmp::min(remaining, MAX_BYTES_PER_GROUP);
            unsafe { this.buffer.set_len(poll_amt) };
            let bytes = unsafe { &mut (*ptr).buffer[..poll_amt] };

            if let Ok(_) = this.reader.read_exact(bytes) {
                let group_id_input = this.group_id + (this.groups_rendered as u64);
                if let Ok(sender) = par_scramble_encrypt_group(bytes, this.security_level, &this.drill, pqc, this.header_size_bytes, this.target_cid, this.object_id, group_id_input, |a, b, c, d, e| {
                    (fx)(a, b, c, d, e)
                }) {
                    this.groups_rendered += 1;
                    this.read_cursor += poll_amt;
                    Poll::Ready(Some(sender))
                } else {
                    println!("Error parallel scrambling file");
                    Poll::Ready(None)
                }
            } else {
                println!("Error polling exact amt {}", poll_amt);
                Poll::Ready(None)
            }
        } else {
            println!("Done rendering all groups!");
            Poll::Ready(None)
        }
    }

I need to use a pointer because I mix mutable and immutable borrows in the same closure. I need to pass a mutable reference of buffer into the read_exact function, and thereafter, process its bytes in par_scramble_encrypt_group. Further, a function labelled fx needs to be immutably borrowed for the par_scramble_encrypt_group.

2

Hard to tell for sure since there’s a lot going on but can you use a destructuring borrow on self initially to borrow the various fields simultaneously?

Re: set_len, can you use `&mut buffer[..poll_amt]' on the call to read_exact instead?

It feels like unsafe shouldn’t be needed for anything with respect to borrowing in this code, but I’ve had that thought before and been 100% wrong😉

EDIT
Something like this right at the top of the function

        let Self {
            pqc,
            file_len,
            read_cursor,
            buffer,
            ..
        } = &mut *self;
3 Likes
3

I'm sure I'm a bit slow but I can't begin to fathom what that code is supposed to do.

Having been coding all kind of things in Rust for a year now and never having to use the likes of unsafe or Pin I have to wonder if it is really necessary here.

2 Likes
4

Yeah, my bad. The code is messy, have had bad sleep lately, thus a tangled mind.

This is what the code does:
It calculates poll_amt bytes that need to be read_exact from the source (in this case, a BufReader that reads from a File). Then, it takes those bytes and runs them through par_scramble_encrypt_group (you can think of a group as a "chunk" of the file). It then increments the read cursor AND number of groups rendered, and then returns the output of that function

5

I suppose this was a "duh", but it didn't even cross my mind that this was the most reasonable way to handle it. Thanks for this. I'll process your other recommendations in a bit

6

@drewkett, thanks to your help, the code is much cleaner:

   fn scramble_next_group(mut self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<Option<GroupSender>> {
        let Self {
            pqc,
            file_len,
            read_cursor,
            buffer,
            group_id,
            groups_rendered,
            header_size_bytes,
            target_cid,
            object_id,
            header_inscriber,
            reader,
            security_level,
            drill,
            ..
        } = &mut *self;
        if *read_cursor != *file_len {
            let remaining = *file_len - *read_cursor;
            let poll_amt = std::cmp::min(remaining, MAX_BYTES_PER_GROUP);
            let bytes = &mut buffer[..poll_amt];

            if let Ok(_) = reader.read_exact(bytes) {
                let group_id_input = *group_id + (*groups_rendered as u64);
                if let Ok(sender) = par_scramble_encrypt_group(bytes, *security_level, &*drill, pqc, *header_size_bytes, *target_cid, *object_id, group_id_input, |a, b, c, d, e| {
                    (header_inscriber)(a, b, c, d, e)
                }) {
                    *groups_rendered += 1;
                    *read_cursor += poll_amt;
                    Poll::Ready(Some(sender))
                } else {
                    println!("Error parallel scrambling file");
                    Poll::Ready(None)
                }
            } else {
                println!("Error polling exact amt {}", poll_amt);
                Poll::Ready(None)
            }
        } else {
            println!("Done rendering all groups!");
            Poll::Ready(None)
        }
    }

When initializing the buffer (in another closure entirely), I have to set it up with_capacity, and then unsafely set the length of the buffer. I could extend it with zeroes to get the required length, but it's much quicker to just set a single field.

BTW: To me, being able to extract individual fields from self in the way you displayed is game changing. Awesome!!

1 Like
7

Looks a little bit easier to read this way :wink:

And yes destructuring/split borrows is pretty essential to avoiding unsafe. Combining it with dereference and Deref/DerefMut (which is what’s happening with &mut *self to go from Pin<&mut Self> to &mut Self ) makes using wrapper types like Pin a lot easier.

Regarding the buffer, are you using it anywhere else? If not, you should be able to just initialize it with vec![0; MAX_BYTES_PER_GROUP] or even Box::new([0; MAX_BYTES_PER_GROUP]) if that value is const and not worry about the capacity. The way it’s used in the code that’s shown, you’re only ever using a slice which only cares that the length of the vec is greater than or equal to the slice length.

1 Like
8

Nope, just that one place.

The capacity of the buffer needs to be:

let mut buffer = vec![0u8; std::cmp::min(file_len, MAX_BYTES_PER_GROUP)];

In the docs of vec!, it says it clones the first element for len amount. Thus, it would be more efficient to just "unsafely" (but not really) set the length field of the vector, unless I'm missing a better method?

9

I'm not sure why you say set_len is "not really" unsafe (i.e. sound) here, since (as I see it) you're breaking one of the rules explicit in its documentation:

  • new_len must be less than or equal to capacity() .
  • The elements at old_len..new_len must be initialized.

You're not initializing the bytes, so it seems to me that this is unsound. The example in the documentation for set_len shows calling an FFI function that initializes the contents of the buffer, then calling set_len.

Since you're using read_exact, it's not obvious that there's a good way around just initializing the buffer. Do you actually know there is a performance difference between vec![0; n] and with_capacity(n), or are you just assuming? Zero-initialized memory is usually very cheap these days. I would not make any a priori assumptions about it being slower than uninitialized memory.

10

So long as the logic in the parts of the program read std::cmp::min(file_len, MAX_BYTES_PER_GROUP) or less, it's not unsafe. I never go over the capacity here, even here:

let remaining = *file_len - *read_cursor;
let poll_amt = std::cmp::min(remaining, MAX_BYTES_PER_GROUP);
let bytes = &mut buffer[..poll_amt];

So, not need to have initialized bytes, because they get overwritten on every poll. The allocator allocates the bytes required by the capacity. This code is safe.

For all practical purposes, the difference in time is probably on the order of nanoseconds, and as such probably doesn't matter. The a priori assumption is that calling the underlying allocator without asking it to zero the memory is faster than just telling the allocator to give me a slice of RAM. Unless I'm missing something here, like compiler optimizations, etc.

If the time required to allocate n bytes from the allocator is A(n), then the time required to allocate and zero is A(n) + O(n) (using big-o notation). Right?

Time required to allocate the RAM: A(n)
Time required to allocate and zero the RAM: A(n) + O(n)

For all n > 0, A(n) < A(n) + O(n)

But, as we discussed, O(n) is infinitessimal, so it's basically equivalent

11

The set_len function documents that it is UB to call it with a length that exposes uninitialized memory. If you call it first and then initialize it, that is definitely UB. The other order is necessary for doing it without UB.

12

When you call with_capacity, does that not guarantee that that memory, even though not zeroed, is still writable (i.e., without overlapping between other pointers in the program)?

13

Depends, but not necessarily, which is why I asked. IIRC Vec uses specialization under the hood to call alloc_zeroed instead of alloc, which can request already-zeroed pages from the OS instead of doing a memset. This doesn't have to be O(n) because of virtual memory tricks... not really my area of expertise, so I can't go into detail. The only thing I know is that I know nothing, until I have measured it.

14

Yes, but it is uninitialized. You have to have to write to it first, and then call set_len.

15

Hmm, looks like I don't understand the concept of initialization then. When you say "write to it", does that imply that each index in the allocated slice must be written-to with some T (in this case, a u8)?

Where might not initializing the memory go awry?

16

Socrates, that you?

1 Like
17

Just remember that you are coding for the Rust abstract machine, nor for any underlying hardware that might be used to implement that abstract machine. If you want to code for the underlying hardware, use architecture-specific assembly rather than a hyper-optimizing compiler.

See @RalfJung's post "What The Hardware Does" is not What Your Program Does: Uninitialized Memory.

5 Likes
18

Interesting! I didn't think it went that low to the metal. The more I learn, the more I realize that unsafe really shouldn't be used

With this said, here's one other place I have unsafe code (independent to the code insofar revealed):

let plaintext: &[u8] = plaintext.as_slice();
let plaintext_insert_index: Range<usize> = Self::get_plaintext_buffer_insertion_range_by_wave_id(wave_id, ciphertext_bytes_for_this_wave.len(), self.max_plaintext_wave_length);
let dest_bytes: &mut [u8] = &mut self.unified_plaintext_slab[plaintext_insert_index.clone()];
debug_assert_eq!(plaintext_insert_index.end - plaintext_insert_index.start, dest_bytes.len());
unsafe { std::ptr::copy(plaintext as *const [u8] as *const u8, dest_bytes as *mut [u8] as *mut u8, plaintext.len()) };

I want to copy plaintext, &[u8], into dest_bytes, &mut [u8]. The code above does the job, and have had no unexpected results. The safe way that I know about to replace this single-lined unsafe ptr copy is to just run an iter_mut through the dest_bytes, zip it to an iter of the plaintext, and dereference each individual &mut u8 to equal each individual &u8. I use unsafe here because one line of code seems cleaner than 3-4 lines of code. What is the wisest thing to do in this case?

19

As for your copy, just use copy_from_slice. It compiles down to the same memcpy call as your unsafe.

1 Like
20

For some more reading, I came across this RFC which proposes a solution to your use case, which looks close to being accepted.

RFC: Reading into uninitialized buffers

2 Likes