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

lang: Error on undocumented unsafe account types #1452

Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
0f86b58
cli: Error in build step on missing docstring for AccountInfo and Unc…
tomlinton Feb 13, 2022
11ca096
Refactor to avoid parse_meta, allow account attribute with seeds
tomlinton Feb 14, 2022
72e9088
Attempt at printing span locations
tomlinton Feb 15, 2022
ff5d584
Refactor loop to support filename
tomlinton Feb 15, 2022
440c9e3
Clarify arg
tomlinton Feb 15, 2022
471ba5a
Add test to misc
tomlinton Feb 15, 2022
7ef9d51
Add SAFETY doc comments to some tests
tomlinton Feb 15, 2022
369c333
Add safety_checks to features
tomlinton Feb 16, 2022
b2563e0
Safety fixes for misc
tomlinton Feb 16, 2022
67df10e
Update book link
tomlinton Feb 16, 2022
78f0cdf
Remove init allowance
tomlinton Feb 16, 2022
95c662b
No check on read_all_programs
tomlinton Feb 16, 2022
e18863f
Clippy
tomlinton Feb 16, 2022
e162567
Update error message
tomlinton Feb 16, 2022
e1490c2
Dont require check on seeds or init constraints
tomlinton Feb 16, 2022
48288a0
Fix defaults for features in cli config
tomlinton Feb 16, 2022
72ab7f7
Properly fix defaults
tomlinton Feb 16, 2022
2aafe68
Update auction-house submodule
tomlinton Feb 17, 2022
a17219d
Clippy
tomlinton Feb 17, 2022
c35818f
Disable where necessary via Anchor.toml
tomlinton Feb 17, 2022
1ddc90d
Disable for cashiers-check
tomlinton Feb 17, 2022
729b73a
Update link
tomlinton Feb 17, 2022
8aee0c9
Remove SAFETY and leave only CHECK
tomlinton Feb 17, 2022
2e3b1d1
Update CFO submodules
tomlinton Feb 17, 2022
dc5e0d9
Remove init and seeds cases
tomlinton Feb 17, 2022
ff7fc25
Disable for multisig
tomlinton Feb 17, 2022
ff0104e
Disable for spl/token-proxy
tomlinton Feb 17, 2022
582231c
Update changelog
tomlinton Feb 17, 2022
2a7e472
Merge branch 'master' into tomlinton/error-on-missing-unsafe-docstring
tomlinton Feb 17, 2022
0082e0f
add integratoin test
armaniferrante Feb 17, 2022
ed50c62
lint
armaniferrante Feb 17, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions cli/src/config.rs
Expand Up @@ -166,6 +166,7 @@ impl WithPath<Config> {
path.join("src/lib.rs"),
version,
self.features.seeds,
false,
)?;
r.push(Program {
lib_name,
Expand Down Expand Up @@ -259,6 +260,13 @@ pub struct Config {
#[derive(Default, Clone, Debug, Serialize, Deserialize)]
pub struct FeaturesConfig {
pub seeds: bool,
#[serde(default = "default_safety_checks")]
pub safety_checks: bool,
}

// Anchor safety checks on by default
fn default_safety_checks() -> bool {
true
}

#[derive(Clone, Debug, Serialize, Deserialize)]
Expand Down
7 changes: 6 additions & 1 deletion cli/src/lib.rs
Expand Up @@ -1388,7 +1388,12 @@ fn extract_idl(cfg: &WithPath<Config>, file: &str) -> Result<Option<Idl>> {
let manifest_from_path = std::env::current_dir()?.join(PathBuf::from(&*file).parent().unwrap());
let cargo = Manifest::discover_from_path(manifest_from_path)?
.ok_or_else(|| anyhow!("Cargo.toml not found"))?;
anchor_syn::idl::file::parse(&*file, cargo.version(), cfg.features.seeds)
anchor_syn::idl::file::parse(
&*file,
cargo.version(),
cfg.features.seeds,
cfg.features.safety_checks,
)
}

fn idl(cfg_override: &ConfigOverride, subcmd: IdlCommand) -> Result<()> {
Expand Down
2 changes: 1 addition & 1 deletion lang/syn/Cargo.toml
Expand Up @@ -16,7 +16,7 @@ anchor-debug = []
seeds = []

[dependencies]
proc-macro2 = "1.0"
proc-macro2 = { version = "1.0", features=["span-locations"]}
proc-macro2-diagnostics = "0.9"
quote = "1.0"
syn = { version = "1.0.60", features = ["full", "extra-traits", "parsing"] }
Expand Down
4 changes: 4 additions & 0 deletions lang/syn/src/idl/file.rs
Expand Up @@ -18,8 +18,12 @@ pub fn parse(
filename: impl AsRef<Path>,
version: String,
seeds_feature: bool,
safety_checks: bool,
) -> Result<Option<Idl>> {
let ctx = CrateContext::parse(filename)?;
if safety_checks {
ctx.safety_checks()?;
}

let program_mod = match parse_program_mod(&ctx) {
None => return Ok(None),
Expand Down
54 changes: 53 additions & 1 deletion lang/syn/src/parser/context.rs
@@ -1,6 +1,6 @@
use anyhow::anyhow;
use std::collections::BTreeMap;
use std::path::{Path, PathBuf};

use syn::parse::{Error as ParseError, Result as ParseResult};

/// Crate parse context
Expand Down Expand Up @@ -40,6 +40,43 @@ impl CrateContext {
modules: ParsedModule::parse_recursive(root.as_ref())?,
})
}

// Perform Anchor safety checks on the parsed create
pub fn safety_checks(&self) -> Result<(), anyhow::Error> {
armaniferrante marked this conversation as resolved.
Show resolved Hide resolved
// Check all structs for unsafe field types, i.e. AccountInfo and UncheckedAccount.
for (_, ctx) in self.modules.iter() {
for unsafe_field in ctx.unsafe_struct_fields() {
// Check if unsafe field type has been documented with a /// SAFETY: doc string.
let is_documented_or_safe = unsafe_field.attrs.iter().any(|attr| {
attr.tokens.clone().into_iter().any(|token| match token {
// Check for comments containing SAFETY: or CHECK:
proc_macro2::TokenTree::Literal(s) => {
s.to_string().contains("SAFETY:") || s.to_string().contains("CHECK:")
armaniferrante marked this conversation as resolved.
Show resolved Hide resolved
}
_ => false,
})
});
if !is_documented_or_safe {
let ident = unsafe_field.ident.as_ref().unwrap();
let span = ident.span();
// Error if undocumented.
return Err(anyhow!(
r#"
{}:{}:{}
Struct field "{}" is unsafe, but is not documented.
Please add a `/// SAFETY:` doc comment to the field enumerating potential security risks.
tomlinton marked this conversation as resolved.
Show resolved Hide resolved
See https://book.anchor-lang.com/chapter_3/errors.html#safety-checks for more information.
"#,
ctx.file.canonicalize().unwrap().display(),
span.start().line,
span.start().column,
ident.to_string()
));
};
}
}
Ok(())
}
}

/// Module parse context
Expand Down Expand Up @@ -181,6 +218,21 @@ impl ParsedModule {
})
}

fn unsafe_struct_fields(&self) -> impl Iterator<Item = &syn::Field> {
tomlinton marked this conversation as resolved.
Show resolved Hide resolved
self.structs()
armaniferrante marked this conversation as resolved.
Show resolved Hide resolved
.flat_map(|s| &s.fields)
.filter(|f| match &f.ty {
syn::Type::Path(syn::TypePath {
path: syn::Path { segments, .. },
..
}) => {
segments.len() == 1 && segments[0].ident == "UncheckedAccount"
|| segments[0].ident == "AccountInfo"
}
_ => false,
})
}

fn enums(&self) -> impl Iterator<Item = &syn::ItemEnum> {
self.items.iter().filter_map(|i| match i {
syn::Item::Enum(item) => Some(item),
Expand Down
25 changes: 23 additions & 2 deletions tests/cfo/programs/cfo/src/lib.rs
Expand Up @@ -396,7 +396,7 @@ pub struct AuthorizeMarket<'info> {
market_auth: Account<'info, MarketAuth>,
#[account(mut)]
payer: Signer<'info>,
// Not read or written to so not validated.
/// SAFETY: Not read or written to so not validated.
market: UncheckedAccount<'info>,
system_program: Program<'info, System>,
}
Expand Down Expand Up @@ -450,7 +450,7 @@ pub struct CreateOfficerOpenOrders<'info> {
dex_program: Program<'info, Dex>,
system_program: Program<'info, System>,
rent: Sysvar<'info, Rent>,
// Used for CPI. Not read or written so not validated.
/// SAFETY: Used for CPI. Not read or written so not validated.
market: UncheckedAccount<'info>,
}

Expand Down Expand Up @@ -483,10 +483,14 @@ pub struct SweepFees<'info> {
#[derive(Accounts)]
pub struct DexAccounts<'info> {
#[account(mut)]
/// SAFETY:
market: UncheckedAccount<'info>,
#[account(mut)]
/// SAFETY:
pc_vault: UncheckedAccount<'info>,
/// SAFETY:
sweep_authority: UncheckedAccount<'info>,
/// SAFETY:
vault_signer: UncheckedAccount<'info>,
dex_program: Program<'info, Dex>,
token_program: Program<'info, Token>,
Expand Down Expand Up @@ -523,6 +527,7 @@ pub struct SwapToUsdc<'info> {
dex_program: Program<'info, Dex>,
token_program: Program<'info, Token>,
#[account(address = tx_instructions::ID)]
/// SAFETY:
instructions: UncheckedAccount<'info>,
rent: Sysvar<'info, Rent>,
}
Expand Down Expand Up @@ -561,6 +566,7 @@ pub struct SwapToSrm<'info> {
dex_program: Program<'info, Dex>,
token_program: Program<'info, Token>,
#[account(address = tx_instructions::ID)]
/// SAFETY:
instructions: UncheckedAccount<'info>,
rent: Sysvar<'info, Rent>,
}
Expand All @@ -569,32 +575,42 @@ pub struct SwapToSrm<'info> {
// They are not read or written and so are not validated.
#[derive(Accounts)]
pub struct DexMarketAccounts<'info> {
/// SAFETY:
#[account(mut)]
market: UncheckedAccount<'info>,
/// SAFETY:
#[account(mut)]
open_orders: UncheckedAccount<'info>,
/// SAFETY:
#[account(mut)]
request_queue: UncheckedAccount<'info>,
/// SAFETY:
#[account(mut)]
event_queue: UncheckedAccount<'info>,
/// SAFETY:
#[account(mut)]
bids: UncheckedAccount<'info>,
/// SAFETY:
#[account(mut)]
asks: UncheckedAccount<'info>,
/// SAFETY:
// The `spl_token::Account` that funds will be taken from, i.e., transferred
// from the user into the market's vault.
//
// For bids, this is the base currency. For asks, the quote.
#[account(mut)]
order_payer_token_account: UncheckedAccount<'info>,
/// SAFETY:
// Also known as the "base" currency. For a given A/B market,
// this is the vault for the A mint.
#[account(mut)]
coin_vault: UncheckedAccount<'info>,
/// SAFETY:
// Also known as the "quote" currency. For a given A/B market,
// this is the vault for the B mint.
#[account(mut)]
pc_vault: UncheckedAccount<'info>,
/// SAFETY:
// PDA owner of the DEX's token accounts for base + quote currencies.
vault_signer: UncheckedAccount<'info>,
}
Expand Down Expand Up @@ -636,6 +652,7 @@ pub struct DropStakeReward<'info> {
not(feature = "test"),
account(address = mint::SRM),
)]
/// SAFETY:
mint: UncheckedAccount<'info>,
srm: DropStakeRewardPool<'info>,
msrm: DropStakeRewardPool<'info>,
Expand All @@ -652,10 +669,14 @@ pub struct DropStakeReward<'info> {
// program to handle it.
#[derive(Accounts)]
pub struct DropStakeRewardPool<'info> {
/// SAFETY:
registrar: UncheckedAccount<'info>,
/// SAFETY:
reward_event_q: UncheckedAccount<'info>,
pool_mint: Account<'info, Mint>,
/// SAFETY:
vendor: UncheckedAccount<'info>,
/// SAFETY:
vendor_vault: UncheckedAccount<'info>,
}

Expand Down