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

Make uninit_vector function unsafe #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kitcatier
Copy link

@kitcatier kitcatier commented Mar 17, 2023

Hello, I found a soundness issue in this crate.

distaff/src/utils/mod.rs

Lines 10 to 14 in fad92ce

pub fn uninit_vector<T>(length: usize) -> Vec<T> {
let mut vector = Vec::with_capacity(length);
unsafe { vector.set_len(length); }
return vector;
}

The unsafe function called needs to ensure that the parameter must be:

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

https://doc.rust-lang.org/std/vec/struct.Vec.html#method.set_len
and the developer who calls the uninit_vector function may not notice this safety requirement.
Marking them unsafe also means that callers must make sure they know what they're doing.
A similar situation to the above is as follows:
https://github.com/facebook/winterfell/pull/30

@kitcatier
Copy link
Author

Here's a safer implementation that uses std::mem::MaybeUninit to create an uninitialized Vec:

pub fn uninit_vector<T>(length: usize) -> Vec<T> { 
     let mut vector = Vec::with_capacity(length); 
     unsafe { 
         let data_ptr = vector.as_mut_ptr(); 
         std::ptr::write_bytes(data_ptr, 0, length); 
         vector.set_len(length); 
     } 
     return vector; 
 }

This implementation uses std::ptr::write_bytes to initialize each element in the vector. This ensures that all elements in the vector are properly initialized, avoiding potential memory safety issues.

@bobbinth
Copy link
Contributor

@kitcatier - thank you for noticing this! This codebase isn't actively maintained, but there is a similar function here: https://github.com/facebook/winterfell/blob/main/utils/core/src/lib.rs#L78. You may want to create an issue about it in the Winterfell repo (though, I think there the function is marked as unsafe).

@kitcatier
Copy link
Author

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants