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.

Sign up for GitHub

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

Removes remove_on_drop field from AppendVec #32741

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Removes remove_on_drop field from AppendVec #32741

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 83 additions & 87 deletions runtime/src/append_vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,6 @@ pub struct AppendVec {

/// The number of bytes available for storing items.
file_size: u64,

/// True if the file should automatically be deleted when this AppendVec is dropped.
remove_on_drop: bool,
}

lazy_static! {
Expand All @@ -228,15 +225,13 @@ lazy_static! {

impl Drop for AppendVec {
fn drop(&mut self) {
if self.remove_on_drop {
APPEND_VEC_MMAPPED_FILES_OPEN.fetch_sub(1, Ordering::Relaxed);
if let Err(_e) = remove_file(&self.path) {
// promote this to panic soon.
// disabled due to many false positive warnings while running tests.
// blocked by rpc's upgrade to jsonrpc v17
//error!("AppendVec failed to remove {:?}: {:?}", &self.path, e);
inc_new_counter_info!("append_vec_drop_fail", 1);
}
APPEND_VEC_MMAPPED_FILES_OPEN.fetch_sub(1, Ordering::Relaxed);
if let Err(_err) = remove_file(&self.path) {
// promote this to panic soon.
// disabled due to many false positive warnings while running tests.
// blocked by rpc's upgrade to jsonrpc v17
//error!("AppendVec failed to remove {}: {err}", &self.path.display());
inc_new_counter_info!("append_vec_drop_fail", 1);
}
}
}
Expand Down Expand Up @@ -294,12 +289,11 @@ impl AppendVec {
append_lock: Mutex::new(()),
current_len: AtomicUsize::new(initial_len),
file_size: size as u64,
remove_on_drop: true,
}
}

pub fn set_no_remove_on_drop(&mut self) {
self.remove_on_drop = false;
// noop: will be removed next
}

fn sanitize_len_and_size(current_len: usize, file_size: usize) -> Result<()> {
Expand Down Expand Up @@ -398,7 +392,6 @@ impl AppendVec {
append_lock: Mutex::new(()),
current_len: AtomicUsize::new(current_len),
file_size,
remove_on_drop: true,
})
}

Expand Down Expand Up @@ -667,7 +660,7 @@ pub mod tests {
account::{accounts_equal, Account, AccountSharedData, WritableAccount},
timing::duration_as_ms,
},
std::time::Instant,
std::{mem::ManuallyDrop, time::Instant},
};

impl AppendVec {
Expand Down Expand Up @@ -1176,26 +1169,27 @@ pub mod tests {
fn test_new_from_file_crafted_data_len() {
let file = get_append_vec_path("test_new_from_file_crafted_data_len");
let path = &file.path;
let mut av = AppendVec::new(path, true, 1024 * 1024);
av.set_no_remove_on_drop();
let accounts_len = {
// wrap AppendVec in ManuallyDrop to ensure we do not remove the backing file when dropped
let av = ManuallyDrop::new(AppendVec::new(path, true, 1024 * 1024));

let crafted_data_len = 1;
let crafted_data_len = 1;

av.append_account_test(&create_test_account(10)).unwrap();
av.append_account_test(&create_test_account(10)).unwrap();

let accounts = av.accounts(0);
let StoredAccountMeta::AppendVec(account) = accounts.first().unwrap();
account.set_data_len_unsafe(crafted_data_len);
assert_eq!(account.data_len(), crafted_data_len);
let accounts = av.accounts(0);
let StoredAccountMeta::AppendVec(account) = accounts.first().unwrap();
account.set_data_len_unsafe(crafted_data_len);
assert_eq!(account.data_len(), crafted_data_len);

// Reload accounts and observe crafted_data_len
let accounts = av.accounts(0);
let account = accounts.first().unwrap();
assert_eq!(account.data_len(), crafted_data_len);
// Reload accounts and observe crafted_data_len
let accounts = av.accounts(0);
let account = accounts.first().unwrap();
assert_eq!(account.data_len(), crafted_data_len);

av.flush().unwrap();
let accounts_len = av.len();
drop(av);
Copy link
Contributor Author

@brooksprumo brooksprumo Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling drop on a ManuallyDrop is a clippy lint. So I wrapped the block of code in its own scope to drop av without explicitly calling drop.

av.flush().unwrap();
av.len()
};
let result = AppendVec::new_from_file(path, accounts_len);
assert_matches!(result, Err(ref message) if message.to_string().contains("incorrect layout/length/data"));
}
Expand All @@ -1204,24 +1198,25 @@ pub mod tests {
fn test_new_from_file_too_large_data_len() {
let file = get_append_vec_path("test_new_from_file_too_large_data_len");
let path = &file.path;
let mut av = AppendVec::new(path, true, 1024 * 1024);
av.set_no_remove_on_drop();
let accounts_len = {
// wrap AppendVec in ManuallyDrop to ensure we do not remove the backing file when dropped
let av = ManuallyDrop::new(AppendVec::new(path, true, 1024 * 1024));

let too_large_data_len = u64::max_value();
av.append_account_test(&create_test_account(10)).unwrap();
let too_large_data_len = u64::max_value();
av.append_account_test(&create_test_account(10)).unwrap();

let accounts = av.accounts(0);
let StoredAccountMeta::AppendVec(account) = accounts.first().unwrap();
account.set_data_len_unsafe(too_large_data_len);
assert_eq!(account.data_len(), too_large_data_len);
let accounts = av.accounts(0);
let StoredAccountMeta::AppendVec(account) = accounts.first().unwrap();
account.set_data_len_unsafe(too_large_data_len);
assert_eq!(account.data_len(), too_large_data_len);

// Reload accounts and observe no account with bad offset
let accounts = av.accounts(0);
assert_matches!(accounts.first(), None);
// Reload accounts and observe no account with bad offset
let accounts = av.accounts(0);
assert_matches!(accounts.first(), None);

av.flush().unwrap();
let accounts_len = av.len();
drop(av);
av.flush().unwrap();
av.len()
};
let result = AppendVec::new_from_file(path, accounts_len);
assert_matches!(result, Err(ref message) if message.to_string().contains("incorrect layout/length/data"));
}
Expand All @@ -1230,60 +1225,61 @@ pub mod tests {
fn test_new_from_file_crafted_executable() {
let file = get_append_vec_path("test_new_from_crafted_executable");
let path = &file.path;
let mut av = AppendVec::new(path, true, 1024 * 1024);
av.set_no_remove_on_drop();
av.append_account_test(&create_test_account(10)).unwrap();
{
let mut executable_account = create_test_account(10);
executable_account.1.set_executable(true);
av.append_account_test(&executable_account).unwrap();
}
let accounts_len = {
// wrap AppendVec in ManuallyDrop to ensure we do not remove the backing file when dropped
let av = ManuallyDrop::new(AppendVec::new(path, true, 1024 * 1024));
av.append_account_test(&create_test_account(10)).unwrap();
{
let mut executable_account = create_test_account(10);
executable_account.1.set_executable(true);
av.append_account_test(&executable_account).unwrap();
}

// reload accounts
let accounts = av.accounts(0);
// reload accounts
let accounts = av.accounts(0);

// ensure false is 0u8 and true is 1u8 actually
assert_eq!(*accounts[0].ref_executable_byte(), 0);
assert_eq!(*accounts[1].ref_executable_byte(), 1);
// ensure false is 0u8 and true is 1u8 actually
assert_eq!(*accounts[0].ref_executable_byte(), 0);
assert_eq!(*accounts[1].ref_executable_byte(), 1);

let StoredAccountMeta::AppendVec(account) = &accounts[0];
let crafted_executable = u8::max_value() - 1;
let StoredAccountMeta::AppendVec(account) = &accounts[0];
let crafted_executable = u8::max_value() - 1;

account.set_executable_as_byte(crafted_executable);
account.set_executable_as_byte(crafted_executable);

// reload crafted accounts
let accounts = av.accounts(0);
let StoredAccountMeta::AppendVec(account) = accounts.first().unwrap();
// reload crafted accounts
let accounts = av.accounts(0);
let StoredAccountMeta::AppendVec(account) = accounts.first().unwrap();

// upper 7-bits are not 0, so sanitization should fail
assert!(!account.sanitize_executable());
// upper 7-bits are not 0, so sanitization should fail
assert!(!account.sanitize_executable());

// we can observe crafted value by ref
{
let executable_bool: &bool = &account.account_meta.executable;
// Depending on use, *executable_bool can be truthy or falsy due to direct memory manipulation
// assert_eq! thinks *executable_bool is equal to false but the if condition thinks it's not, contradictorily.
assert!(!*executable_bool);
#[cfg(not(target_arch = "aarch64"))]
// we can observe crafted value by ref
{
const FALSE: bool = false; // keep clippy happy
if *executable_bool == FALSE {
panic!("This didn't occur if this test passed.");
let executable_bool: &bool = &account.account_meta.executable;
// Depending on use, *executable_bool can be truthy or falsy due to direct memory manipulation
// assert_eq! thinks *executable_bool is equal to false but the if condition thinks it's not, contradictorily.
assert!(!*executable_bool);
#[cfg(not(target_arch = "aarch64"))]
{
const FALSE: bool = false; // keep clippy happy
if *executable_bool == FALSE {
panic!("This didn't occur if this test passed.");
}
}
assert_eq!(*account.ref_executable_byte(), crafted_executable);
}
assert_eq!(*account.ref_executable_byte(), crafted_executable);
}

// we can NOT observe crafted value by value
{
let executable_bool: bool = account.executable();
assert!(!executable_bool);
assert_eq!(account.get_executable_byte(), 0); // Wow, not crafted_executable!
}
// we can NOT observe crafted value by value
{
let executable_bool: bool = account.executable();
assert!(!executable_bool);
assert_eq!(account.get_executable_byte(), 0); // Wow, not crafted_executable!
}

av.flush().unwrap();
let accounts_len = av.len();
drop(av);
av.flush().unwrap();
av.len()
};
yhchiang-sol marked this conversation as resolved.
Show resolved Hide resolved
let result = AppendVec::new_from_file(path, accounts_len);
assert_matches!(result, Err(ref message) if message.to_string().contains("incorrect layout/length/data"));
}
Expand Down