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 21 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion cli/src/config.rs
Original file line number Diff line number Diff line change
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 @@ -256,9 +257,26 @@ pub struct Config {
pub test: Option<Test>,
}

#[derive(Default, Clone, Debug, Serialize, Deserialize)]
#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct FeaturesConfig {
#[serde(default)]
pub seeds: bool,
#[serde(default = "default_safety_checks")]
pub safety_checks: bool,
}

impl Default for FeaturesConfig {
fn default() -> Self {
Self {
seeds: false,
// Anchor safety checks on by default
safety_checks: true,
}
}
}

fn default_safety_checks() -> bool {
true
}

#[derive(Clone, Debug, Serialize, Deserialize)]
Expand Down
7 changes: 6 additions & 1 deletion cli/src/lib.rs
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
64 changes: 63 additions & 1 deletion lang/syn/src/parser/context.rs
Original file line number Diff line number Diff line change
@@ -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,53 @@ 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
}
proc_macro2::TokenTree::Group(g) => {
g.stream().into_iter().any(|token| match token {
proc_macro2::TokenTree::Ident(s) => {
// Allow PDAs and accounts which are being initialized without
// doc comments
s == "seeds" || s == "init"
}
_ => false,
})
}
_ => 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 explaining why no checks through types are necessary.
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 +228,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
2 changes: 1 addition & 1 deletion tests/auction-house
Submodule auction-house updated 1 files
+1 −0 Anchor.toml
3 changes: 3 additions & 0 deletions tests/cashiers-check/Anchor.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,6 @@ cashiers_check = "Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"

[scripts]
test = "yarn run mocha -t 1000000 tests/"

[features]
safety_checks = false
3 changes: 3 additions & 0 deletions tests/cfo/Anchor.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,6 @@ program = "./deps/stake/target/deploy/registry.so"
[[test.genesis]]
address = "6ebQNeTPZ1j7k3TtkCCtEPRvG7GQsucQrZ7sSEDQi9Ks"
program = "./deps/stake/target/deploy/lockup.so"

[features]
safety_checks = false
3 changes: 3 additions & 0 deletions tests/chat/Anchor.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,6 @@ chat = "Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"

[scripts]
test = "yarn run mocha -t 1000000 tests/"

[features]
safety_checks = false
3 changes: 3 additions & 0 deletions tests/custom-coder/Anchor.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,6 @@ wallet = "~/.config/solana/id.json"

[scripts]
test = "yarn run ts-mocha -p ./tsconfig.json -t 1000000 tests/**/*.ts"

[features]
safety_checks = false
3 changes: 3 additions & 0 deletions tests/errors/Anchor.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,6 @@ errors = "Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"

[scripts]
test = "yarn run mocha -t 1000000 tests/"

[features]
safety_checks = false
3 changes: 3 additions & 0 deletions tests/escrow/Anchor.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,6 @@ escrow = "Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"

[scripts]
test = "yarn run ts-mocha -t 1000000 tests/*.ts"

[features]
safety_checks = false
3 changes: 3 additions & 0 deletions tests/ido-pool/Anchor.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,6 @@ ido_pool = "Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"

[scripts]
test = "yarn run mocha -t 1000000 tests/"

[features]
safety_checks = false
3 changes: 3 additions & 0 deletions tests/interface/Anchor.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,6 @@ counter_auth = "Aws2XRVHjNqCUbMmaU245ojT2DBJFYX58KVo2YySEeeP"

[scripts]
test = "yarn run mocha -t 1000000 tests/"

[features]
safety_checks = false
3 changes: 3 additions & 0 deletions tests/lockup/Anchor.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,6 @@ registry = "HmbTLCmaGvZhKnn1Zfa1JVnp7vkMV4DYVxPLWBVoN65L"

[scripts]
test = "yarn run mocha -t 1000000 tests/"

[features]
safety_checks = false
40 changes: 40 additions & 0 deletions tests/misc/programs/misc/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ pub struct TestTokenSeedsInit<'info> {
)]
pub my_pda: Account<'info, TokenAccount>,
#[account(mut)]
/// SAFETY:
pub authority: AccountInfo<'info>,
pub system_program: Program<'info, System>,
pub rent: Sysvar<'info, Rent>,
Expand Down Expand Up @@ -60,6 +61,7 @@ pub struct TestValidateAssociatedToken<'info> {
)]
pub token: Account<'info, TokenAccount>,
pub mint: Account<'info, Mint>,
/// SAFETY:
pub wallet: AccountInfo<'info>,
}

Expand All @@ -71,6 +73,7 @@ pub struct TestInstructionConstraint<'info> {
bump = nonce,
)]
pub my_pda: AccountInfo<'info>,
/// SAFETY:
pub my_account: AccountInfo<'info>,
}

Expand All @@ -86,6 +89,7 @@ pub struct TestPdaInit<'info> {
pub my_pda: Account<'info, DataU16>,
#[account(mut)]
pub my_payer: Signer<'info>,
/// SAFETY:
pub foo: AccountInfo<'info>,
pub system_program: Program<'info, System>,
}
Expand All @@ -112,6 +116,7 @@ pub struct TestPdaMutZeroCopy<'info> {
bump = my_pda.load()?.bump,
)]
pub my_pda: Loader<'info, DataZeroCopy>,
/// SAFETY:
pub my_payer: AccountInfo<'info>,
}

Expand All @@ -135,36 +140,43 @@ pub struct InitializeSkipRentExempt<'info> {

#[derive(Accounts)]
pub struct InitializeNoRentExempt<'info> {
/// SAFETY:
pub data: AccountInfo<'info>,
}

#[derive(Accounts)]
pub struct TestOwner<'info> {
#[account(owner = *misc.key)]
/// SAFETY:
pub data: AccountInfo<'info>,
/// SAFETY:
pub misc: AccountInfo<'info>,
}

#[derive(Accounts)]
pub struct TestExecutable<'info> {
#[account(executable)]
/// SAFETY:
pub program: AccountInfo<'info>,
}

#[derive(Accounts)]
pub struct TestStateCpi<'info> {
#[account(signer)]
/// SAFETY:
pub authority: AccountInfo<'info>,
#[account(mut, state = misc2_program)]
pub cpi_state: CpiState<'info, Misc2State>,
#[account(executable)]
/// SAFETY:
pub misc2_program: AccountInfo<'info>,
}

#[derive(Accounts)]
pub struct TestClose<'info> {
#[account(mut, close = sol_dest)]
pub data: Account<'info, Data>,
/// SAFETY:
sol_dest: AccountInfo<'info>,
}

Expand Down Expand Up @@ -259,6 +271,7 @@ pub struct TestInitWithEmptySeeds<'info> {
#[derive(Accounts)]
pub struct TestEmptySeedsConstraint<'info> {
#[account(seeds = [], bump)]
/// SAFETY:
pub pda: AccountInfo<'info>,
}

Expand Down Expand Up @@ -287,6 +300,7 @@ pub struct TestInitIfNeededChecksOwner<'info> {
#[account(mut)]
pub payer: Signer<'info>,
pub system_program: Program<'info, System>,
/// SAFETY:
pub owner: AccountInfo<'info>,
}

Expand All @@ -310,7 +324,9 @@ pub struct TestInitMintIfNeeded<'info> {
pub rent: Sysvar<'info, Rent>,
pub system_program: Program<'info, System>,
pub token_program: Program<'info, Token>,
/// SAFETY:
pub mint_authority: AccountInfo<'info>,
/// SAFETY:
pub freeze_authority: AccountInfo<'info>,
}

Expand All @@ -324,6 +340,7 @@ pub struct TestInitTokenIfNeeded<'info> {
pub rent: Sysvar<'info, Rent>,
pub system_program: Program<'info, System>,
pub token_program: Program<'info, Token>,
/// SAFETY:
pub authority: AccountInfo<'info>,
}

Expand All @@ -343,6 +360,7 @@ pub struct TestInitAssociatedTokenIfNeeded<'info> {
pub system_program: Program<'info, System>,
pub token_program: Program<'info, Token>,
pub associated_token_program: Program<'info, AssociatedToken>,
/// SAFETY:
pub authority: AccountInfo<'info>,
}

Expand All @@ -360,12 +378,14 @@ pub struct TestConstArraySize<'info> {

#[derive(Accounts)]
pub struct NoRentExempt<'info> {
/// SAFETY:
pub data: AccountInfo<'info>,
}

#[derive(Accounts)]
pub struct EnforceRentExempt<'info> {
#[account(rent_exempt = enforce)]
/// SAFETY:
pub data: AccountInfo<'info>,
}

Expand All @@ -381,6 +401,7 @@ pub struct InitDecreaseLamports<'info> {
#[derive(Accounts)]
pub struct InitIfNeededChecksRentExemption<'info> {
#[account(init_if_needed, payer = user, space = 1000)]
/// SAFETY:
pub data: AccountInfo<'info>,
#[account(mut)]
pub user: Signer<'info>,
Expand Down Expand Up @@ -409,3 +430,22 @@ pub struct TestProgramIdConstraintUsingFindPda<'info> {
#[account(seeds = [b"seed"], bump, seeds::program = crate::ID)]
second: AccountInfo<'info>,
}

#[derive(Accounts)]
pub struct TestUnsafeFieldSafetyErrors<'info> {
#[doc = "test"]
/// SAFETY:
pub data: UncheckedAccount<'info>,
#[account(mut)]
/// SAFETY:
pub data_two: UncheckedAccount<'info>,
#[account(
seeds = [b"my-seed", signer.key.as_ref()],
bump
)]
pub data_three: UncheckedAccount<'info>,
/// SAFETY:
pub data_four: UncheckedAccount<'info>,
pub signer: Signer<'info>,
pub system_program: Program<'info, System>,
}
Loading