Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions src/checker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use crate::problem::ProblemList;
use crate::problem::UnusedAllowApi;
use crate::proxy::cargo::profile_name;
use crate::proxy::rpc;
use crate::proxy::rpc::ExternUsage;
use crate::proxy::rpc::UnsafeUsage;
use crate::proxy::subprocess::SubprocessConfig;
use crate::symbol_graph::NameSource;
Expand Down Expand Up @@ -238,6 +239,7 @@ impl Checker {
};
match request {
rpc::Request::CrateUsesUnsafe(usage) => Ok(self.crate_uses_unsafe(usage)),
rpc::Request::CrateUsesExtern(usage) => Ok(self.crate_uses_extern(usage)),
rpc::Request::LinkerInvoked(link_info) => {
self.outstanding_linker_invocations.push(link_info.clone());
Ok(ProblemList::default())
Expand Down Expand Up @@ -343,6 +345,10 @@ impl Checker {
Problem::DisallowedUnsafe(usage.clone()).into()
}

pub(crate) fn crate_uses_extern(&self, usage: &ExternUsage) -> ProblemList {
Problem::DisallowedExtern(usage.clone()).into()
}

pub(crate) fn verify_build_script_permitted(&mut self, pkg_id: &PackageId) -> ProblemList {
if !self.config.raw.common.explicit_build_scripts {
return ProblemList::default();
Expand Down
3 changes: 3 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ pub(crate) struct PackageConfig {
#[serde(default)]
pub(crate) allow_unsafe: bool,

#[serde(default)]
pub(crate) allow_extern: bool,

#[serde(default)]
pub(crate) allow_build_instructions: Vec<String>,

Expand Down
19 changes: 19 additions & 0 deletions src/config/permissions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use super::PackageConfig;
use super::PackageName;
use super::RawConfig;
use super::SandboxConfig;
use crate::config::CommonConfig;
use crate::crate_index::CrateIndex;
use crate::crate_index::CrateKind;
use crate::crate_index::CrateSel;
Expand Down Expand Up @@ -121,6 +122,23 @@ impl Permissions {
.is_some_and(|crate_config| crate_config.allow_unsafe)
}

pub(crate) fn extern_permitted_for_crate(
&self,
crate_sel: &CrateSel,
common_config: &CommonConfig,
) -> bool {
if common_config.version <= 2 {
// For compatibility treat extern permission as granted when unsafe is granted
self.packages
.get(&PermSel::for_non_build_output(crate_sel))
.is_some_and(|crate_config| crate_config.allow_unsafe)
} else {
self.packages
.get(&PermSel::for_non_build_output(crate_sel))
.is_some_and(|crate_config| crate_config.allow_extern)
}
}

pub(crate) fn get(&self, perm_sel: &PermSel) -> Option<&PackageConfig> {
self.packages.get(perm_sel)
}
Expand Down Expand Up @@ -184,6 +202,7 @@ impl PackageConfig {
);
self.allow_proc_macro |= other.allow_proc_macro;
self.allow_unsafe |= other.allow_unsafe;
self.allow_extern |= other.allow_extern;
self.sandbox.inherit(&other.sandbox);
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/config/versions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ pub(crate) const VERSIONS: &[Version] = &[
Ok(())
},
},
Version {
number: 3,
change_notes: "Added field allow_extern",
apply_fn: |_| {},
update_fn: |_| Ok(()),
},
];

impl Version {
Expand Down
28 changes: 28 additions & 0 deletions src/config_editor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,9 @@ pub(crate) fn fixes_for_problem(problem: &Problem, config: &Config) -> Vec<Box<d
Problem::DisallowedUnsafe(failure) => edits.push(Box::new(AllowUnsafe {
perm_sel: PermSel::for_non_build_output(&failure.crate_sel),
})),
Problem::DisallowedExtern(failure) => edits.push(Box::new(AllowExtern {
perm_sel: PermSel::for_non_build_output(&failure.crate_sel),
})),
Problem::UnusedAllowApi(failure) => edits.push(Box::new(RemoveUnusedAllowApis {
unused: failure.clone(),
})),
Expand Down Expand Up @@ -1086,6 +1089,10 @@ struct AllowUnsafe {
perm_sel: PermSel,
}

struct AllowExtern {
perm_sel: PermSel,
}

impl Edit for AllowUnsafe {
fn title(&self) -> String {
format!("Allow package `{}` to use unsafe code", self.perm_sel)
Expand All @@ -1107,6 +1114,27 @@ impl Edit for AllowUnsafe {
}
}

impl Edit for AllowExtern {
fn title(&self) -> String {
format!("Allow package `{}` to use extern code", self.perm_sel)
}

fn help(&self) -> Cow<'static, str> {
"Allow this crate to use extern code. With extern code, this crate could do just about \
anything, so this is like a bit like a wildcard permission. Crates that use extern \
sometimes export APIs that you might want to restrict - e.g. network or filesystem APIs. \
so you should have a think about if this crate falls into that category and if it does, \
add some API definitions for it."
.into()
}

fn apply(&self, editor: &mut ConfigEditor, opts: &EditOpts) -> Result<()> {
let table = editor.pkg_table(&self.perm_sel)?;
set_table_value(table, "allow_extern", toml_edit::value(true), opts);
Ok(())
}
}

struct SandboxAllowNetwork {
perm_sel: PermSel,
}
Expand Down
136 changes: 136 additions & 0 deletions src/extern_checker.rs

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are we now reading and tokenising every file twice now? Once for checking for unsafe and once for checking for extern?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is indeed correct and not ideal. I split it to keep it as separate and easy to understand as possible for now. But it could also be merged, once it has been confirmed that this code in general is correct and useful.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh, right, sorry. I meant completely revert the changes to cackle.toml, so the new fields wouldn't be set

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have reverted both now, but that does not help either as then the tests are failing. I don't really understand where you want to go with this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

From the failure, it looks like the tests are failing because they're saying that the "extern" permission is needed. When I was suggesting reverting the changes to cackle.toml, I meant just the cackle.toml for cackle itself, not the one for the test.

I suspect we also need to make it so that if a config has version=2, then we copy the value from allow_unsafe to allow_extern. That way a version 2 config will still work with a newer version of cackle, which I think is important.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I changed it to only revert the main cackle.toml.

I will try to add this fallback for version 2 tomorrow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is the change I made now what you had in mind?

Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
//! This module tokenises Rust code and looks for the extern blocks. This is done as an additional
//! layer of defence in addition to use of the -Fmissing-unsafe-on-extern flag when compiling crates,
//! since that flag only works on code before Rust edition 2024.

use crate::location::SourceLocation;
use anyhow::Context;
use anyhow::Result;
use ra_ap_rustc_lexer::Token;
use ra_ap_rustc_lexer::TokenKind;
use std::path::Path;

/// Returns the locations of all extern block usages found in `path`
pub(crate) fn scan_path(path: &Path) -> Result<Vec<SourceLocation>> {
let bytes =
std::fs::read(path).with_context(|| format!("Failed to read `{}`", path.display()))?;
let Ok(source) = std::str::from_utf8(&bytes) else {
// If the file isn't valid UTF-8 then we don't need to check it for the extern blocks,
// since it can't be a source file that the rust compiler would accept.
return Ok(Vec::new());
};
Ok(scan_string(source, path))
}

fn scan_string(source: &str, path: &Path) -> Vec<SourceLocation> {
let mut token_start_offset = 0;
let mut locations = Vec::new();

let skip_condition = |x| {
matches!(
x,
TokenKind::BlockComment {
doc_style: _,
terminated: _
} | TokenKind::LineComment { doc_style: _ }
| TokenKind::Whitespace
)
};

let mut iter = ra_ap_rustc_lexer::tokenize(source, ra_ap_rustc_lexer::FrontmatterAllowed::No);
let mut previous_token_text = "";
let mut previous_token_end_offset = 0;
let mut previous_token_length = 0;

while let (Some(token), skipped_offset) = next_token(&mut iter, skip_condition) {
let token_length = usize::try_from(token.len).unwrap();

token_start_offset += skipped_offset;

let token_end_offset = token_start_offset + token_length;
let token_text = &source[token_start_offset..token_end_offset];
if !previous_token_text.is_empty() {
// check against previous token
if check_tokens(previous_token_text, token_text) {
add_location(
source,
path,
&mut locations,
previous_token_end_offset,
previous_token_length,
);
}
}
previous_token_text = token_text;
previous_token_end_offset = token_end_offset;
previous_token_length = token_length;
// always check against potential future token
if let (Some(next_token), skipped_offset) = next_token(&mut iter, skip_condition) {
// the next token starts after the end of the current token
token_start_offset += token_length;
token_start_offset += skipped_offset;

let next_token_length = usize::try_from(next_token.len).unwrap();

let next_token_end_offset = token_start_offset + next_token_length;
let next_token_text = &source[token_start_offset..next_token_end_offset];
if check_tokens(token_text, next_token_text) {
add_location(source, path, &mut locations, token_end_offset, token_length);
}
token_start_offset = next_token_end_offset;
// as we consumed the next token already, this will be our previous token in the next loop iteration
previous_token_text = next_token_text;
previous_token_end_offset = next_token_end_offset;
previous_token_length = next_token_length;
} else {
// current token is last token in file
// this should never be valid code, but we flag it nevertheless to be safe
if token_text == "extern" {
add_location(source, path, &mut locations, token_end_offset, token_length);
}
// as there should not be another loop iteration this should be useless
// we do it just for completeness
token_start_offset = token_end_offset;
}
}
locations
}

fn check_tokens(first_token_text: &str, second_token_text: &str) -> bool {
first_token_text == "extern" && !matches!(second_token_text, "crate" | "]" | ")")
}

/// Returns the next relevant token according to the condition given and the offset to it
fn next_token<F>(
iter: &mut impl Iterator<Item = Token>,
skip_condition: F,
) -> (Option<Token>, usize)
where
F: Fn(TokenKind) -> bool,
{
let mut skipped_offset = 0;

for next in iter.by_ref() {
if skip_condition(next.kind) {
skipped_offset += usize::try_from(next.len).unwrap();
} else {
return (Some(next), skipped_offset);
}
}
(None, skipped_offset)
}

fn add_location(
source: &str,
path: &Path,
locations: &mut Vec<SourceLocation>,
token_end_offset: usize,
token_length: usize,
) {
let column = source[..token_end_offset]
.lines()
.last()
.map(|line| (line.len() - token_length + 1) as u32)
.unwrap_or(1);
let line = 1.max(source[..token_end_offset].lines().count() as u32);
locations.push(SourceLocation::new(path, line, Some(column)));
}
1 change: 1 addition & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ mod crate_index;
mod demangle;
mod deps;
pub(crate) mod events;
mod extern_checker;
pub(crate) mod fs;
pub(crate) mod link_info;
pub(crate) mod location;
Expand Down
16 changes: 15 additions & 1 deletion src/problem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::crate_index::CrateSel;
use crate::crate_index::PackageId;
use crate::names::SymbolOrDebugName;
use crate::proxy::rpc::BinExecutionOutput;
use crate::proxy::rpc::ExternUsage;
use crate::proxy::rpc::UnsafeUsage;
use crate::symbol::Symbol;
use std::collections::BTreeMap;
Expand All @@ -32,6 +33,7 @@ pub(crate) enum Problem {
MissingConfiguration(PathBuf),
UsesBuildScript(PackageId),
DisallowedUnsafe(UnsafeUsage),
DisallowedExtern(ExternUsage),
IsProcMacro(PackageId),
DisallowedApiUsage(ApiUsages),
OffTreeApiUsage(OffTreeApiUsage),
Expand Down Expand Up @@ -175,7 +177,9 @@ impl Problem {
fn should_send_retry_to_subprocess(&self) -> bool {
matches!(
self,
&Problem::ExecutionFailed(..) | &Problem::DisallowedUnsafe(..)
&Problem::ExecutionFailed(..)
| &Problem::DisallowedUnsafe(..)
| &Problem::DisallowedExtern(..)
)
}

Expand Down Expand Up @@ -213,6 +217,7 @@ impl Problem {
Problem::MissingConfiguration(_) => None,
Problem::UsesBuildScript(pkg_id) => Some(pkg_id),
Problem::DisallowedUnsafe(d) => Some(d.crate_sel.pkg_id()),
Problem::DisallowedExtern(d) => Some(d.crate_sel.pkg_id()),
Problem::IsProcMacro(pkg_id) => Some(pkg_id),
Problem::DisallowedApiUsage(d) => Some(&d.pkg_id),
Problem::OffTreeApiUsage(d) => Some(&d.usages.pkg_id),
Expand Down Expand Up @@ -252,6 +257,15 @@ impl Display for Problem {
}
}
}
Problem::DisallowedExtern(usage) => {
write!(f, "`{}` uses extern", usage.crate_sel)?;
if f.alternate() {
writeln!(f)?;
for location in &usage.locations {
writeln!(f, "{location}")?;
}
}
}
Problem::UsesBuildScript(pkg_id) => {
write!(
f,
Expand Down
Loading