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

Add wrapper type for safety #32

Closed
wants to merge 1 commit into from
Closed

Add wrapper type for safety #32

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
42 changes: 29 additions & 13 deletions runtime/src/builtins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use {
solana_sdk::{
feature_set, instruction::InstructionError, pubkey::Pubkey, stake, system_program,
},
std::fmt,
std::{fmt, ops::Deref},
};

fn process_instruction_with_program_logging(
Expand Down Expand Up @@ -108,7 +108,7 @@ pub enum BuiltinAction {
/// State transition enum used for adding and removing builtin programs through
/// feature activations.
#[derive(Debug, Clone, AbiExample)]
pub enum BuiltinFeatureTransition {
pub enum InnerBuiltinFeatureTransition {
/// Add a builtin program if a feature is activated.
Add {
builtin: Builtin,
Expand All @@ -123,6 +123,15 @@ pub enum BuiltinFeatureTransition {
},
}

#[derive(Clone, Debug)]
pub struct BuiltinFeatureTransition(InnerBuiltinFeatureTransition);
impl Deref for BuiltinFeatureTransition {
type Target = InnerBuiltinFeatureTransition;
fn deref(&self) -> &Self::Target {
&self.0
}
}

// https://github.com/solana-labs/solana/pull/23233 added `BuiltinFeatureTransition`
// to `Bank` which triggers https://github.com/rust-lang/rust/issues/92987 while
// attempting to resolve `Sync` on `BankRc` in `AccountsBackgroundService::new` ala,
Expand All @@ -141,7 +150,14 @@ pub enum BuiltinFeatureTransition {
unsafe impl Send for BuiltinFeatureTransition {}
unsafe impl Sync for BuiltinFeatureTransition {}

impl BuiltinFeatureTransition {
// The following trait bounds ensure that the above unsafe trait implementations
// won't allow undefined behavior to be introduced.
#[cfg(debug_assertions)]
trait AutoSendSync: Send + Sync {}
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry how does this work? My understanding of how marker traits are handled is that they just tell the auto-trait resolver, "stop digging, you've struck gold." There's no compiler side verification of Send-iness of Sync-iness, which is why they're unsafe.

Copy link
Author

Choose a reason for hiding this comment

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

A type can't implement AutoSendSync unless it satisfies the Send + Sync trait requirements as well

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I understand that, but it doesn't prevent UB. I can still implement AutoSendSync on a type that I've screwed up Send and/or Sync implementations on. So I guess the comment confused me at best. I'm not sure I understand the value add here other than maybe to mark any other types we may hit this bug on before Rust 1.60.0?

To be clear, the issue the original PR is fixing is not UB, it's bypassing a bug in the auto-trait resolver

Copy link
Author

Choose a reason for hiding this comment

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

You can't implement AutoSendSync unless a type already implements Send + Sync. So if you break the type, the AutoSendSync will cause a compiler error. Without it, your unsafe impl could allow new UB to be undetected

Copy link
Owner

Choose a reason for hiding this comment

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

Ok I get it after some offline brain bashing. Didn't realize that the trait resolver would check field types on explicitly declared marker traits.

I'm cool with this. Going to move it to sdk and mark it deprecated under the assumption we'll need it again

#[cfg(debug_assertions)]
impl AutoSendSync for InnerBuiltinFeatureTransition {}

impl InnerBuiltinFeatureTransition {
pub fn to_action(
&self,
should_apply_action_for_feature: &impl Fn(&Pubkey) -> bool,
Expand Down Expand Up @@ -213,48 +229,48 @@ fn dummy_process_instruction(
/// Dynamic feature transitions for builtin programs
fn builtin_feature_transitions() -> Vec<BuiltinFeatureTransition> {
vec![
BuiltinFeatureTransition::Add {
BuiltinFeatureTransition(InnerBuiltinFeatureTransition::Add {
builtin: Builtin::new(
"compute_budget_program",
solana_sdk::compute_budget::id(),
solana_compute_budget_program::process_instruction,
),
feature_id: feature_set::add_compute_budget_program::id(),
},
BuiltinFeatureTransition::RemoveOrRetain {
}),
BuiltinFeatureTransition(InnerBuiltinFeatureTransition::RemoveOrRetain {
previously_added_builtin: Builtin::new(
"secp256k1_program",
solana_sdk::secp256k1_program::id(),
dummy_process_instruction,
),
addition_feature_id: feature_set::secp256k1_program_enabled::id(),
removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(),
},
BuiltinFeatureTransition::RemoveOrRetain {
}),
BuiltinFeatureTransition(InnerBuiltinFeatureTransition::RemoveOrRetain {
previously_added_builtin: Builtin::new(
"ed25519_program",
solana_sdk::ed25519_program::id(),
dummy_process_instruction,
),
addition_feature_id: feature_set::ed25519_program_enabled::id(),
removal_feature_id: feature_set::prevent_calling_precompiles_as_programs::id(),
},
BuiltinFeatureTransition::Add {
}),
BuiltinFeatureTransition(InnerBuiltinFeatureTransition::Add {
builtin: Builtin::new(
"address_lookup_table_program",
solana_address_lookup_table_program::id(),
solana_address_lookup_table_program::processor::process_instruction,
),
feature_id: feature_set::versioned_tx_message_enabled::id(),
},
BuiltinFeatureTransition::Add {
}),
BuiltinFeatureTransition(InnerBuiltinFeatureTransition::Add {
builtin: Builtin::new(
"zk_token_proof_program",
solana_zk_token_sdk::zk_token_proof_program::id(),
with_program_logging!(solana_zk_token_proof_program::process_instruction),
),
feature_id: feature_set::zk_token_sdk_enabled::id(),
},
}),
]
}

Expand Down