Skip to content

Commit

Permalink
[move-compiler] More lint filtering (#19364)
Browse files Browse the repository at this point in the history
## Description 

- Updated filtering for absint lints 

## Test plan 

- ran tests

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:
  • Loading branch information
tnowacki authored Sep 17, 2024
1 parent bf03567 commit c092832
Show file tree
Hide file tree
Showing 8 changed files with 180 additions and 96 deletions.
2 changes: 1 addition & 1 deletion external-crates/move/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

116 changes: 82 additions & 34 deletions external-crates/move/crates/move-compiler/src/cfgir/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -773,37 +773,95 @@ impl<V: SimpleAbsIntConstructor> AbstractInterpreterVisitor for V {
// utils
//**************************************************************************************************

pub fn cfg_satisfies<FCommand, FExp>(
cfg: &ImmForwardCFG,
mut p_command: FCommand,
mut p_exp: FExp,
) -> bool
where
FCommand: FnMut(&Command) -> bool,
FExp: FnMut(&Exp) -> bool,
{
cfg_satisfies_(cfg, &mut p_command, &mut p_exp)
}

pub fn command_satisfies<FCommand, FExp>(
cmd: &Command,
mut p_command: FCommand,
mut p_exp: FExp,
) -> bool
where
FCommand: FnMut(&Command) -> bool,
FExp: FnMut(&Exp) -> bool,
{
command_satisfies_(cmd, &mut p_command, &mut p_exp)
}

pub fn exp_satisfies<F>(e: &Exp, mut p: F) -> bool
where
F: FnMut(&Exp) -> bool,
{
exp_satisfies_(e, &mut p)
}

pub fn calls_special_function(special: &[(&str, &str, &str)], cfg: &ImmForwardCFG) -> bool {
cfg_satisfies(cfg, |_| true, |e| is_special_function(special, e))
}

pub fn calls_special_function_command(special: &[(&str, &str, &str)], cmd: &Command) -> bool {
command_satisfies(cmd, |_| true, |e| is_special_function(special, e))
}

pub fn calls_special_function_exp(special: &[(&str, &str, &str)], e: &Exp) -> bool {
exp_satisfies(e, |e| is_special_function(special, e))
}

fn is_special_function(special: &[(&str, &str, &str)], e: &Exp) -> bool {
use H::UnannotatedExp_ as E;
matches!(
&e.exp.value,
E::ModuleCall(call) if special.iter().any(|(a, m, f)| call.is(a, m, f)),
)
}

fn cfg_satisfies_(
cfg: &ImmForwardCFG,
p_command: &mut impl FnMut(&Command) -> bool,
p_exp: &mut impl FnMut(&Exp) -> bool,
) -> bool {
cfg.blocks().values().any(|block| {
block
.iter()
.any(|cmd| calls_special_function_command(special, cmd))
.any(|cmd| command_satisfies_(cmd, p_command, p_exp))
})
}

pub fn calls_special_function_command(
special: &[(&str, &str, &str)],
sp!(_, cmd_): &Command,
fn command_satisfies_(
cmd @ sp!(_, cmd_): &Command,
p_command: &mut impl FnMut(&Command) -> bool,
p_exp: &mut impl FnMut(&Exp) -> bool,
) -> bool {
use H::Command_ as C;
match cmd_ {
C::Assign(_, _, e)
| C::Abort(_, e)
| C::Return { exp: e, .. }
| C::IgnoreAndPop { exp: e, .. }
| C::JumpIf { cond: e, .. }
| C::VariantSwitch { subject: e, .. } => calls_special_function_exp(special, e),
C::Mutate(el, er) => {
calls_special_function_exp(special, el) || calls_special_function_exp(special, er)
p_command(cmd)
|| match cmd_ {
C::Assign(_, _, e)
| C::Abort(_, e)
| C::Return { exp: e, .. }
| C::IgnoreAndPop { exp: e, .. }
| C::JumpIf { cond: e, .. }
| C::VariantSwitch { subject: e, .. } => exp_satisfies_(e, p_exp),
C::Mutate(el, er) => exp_satisfies_(el, p_exp) || exp_satisfies_(er, p_exp),
C::Jump { .. } => false,
C::Break(_) | C::Continue(_) => panic!("ICE break/continue not translated to jumps"),
}
C::Jump { .. } => false,
C::Break(_) | C::Continue(_) => panic!("ICE break/continue not translated to jumps"),
}
}

#[growing_stack]
pub fn calls_special_function_exp(special: &[(&str, &str, &str)], e: &Exp) -> bool {
fn exp_satisfies_(e: &Exp, p: &mut impl FnMut(&Exp) -> bool) -> bool {
use H::UnannotatedExp_ as E;
if p(e) {
return true;
}
match &e.exp.value {
E::Unit { .. }
| E::Move { .. }
Expand All @@ -819,25 +877,15 @@ pub fn calls_special_function_exp(special: &[(&str, &str, &str)], e: &Exp) -> bo
| E::Dereference(e)
| E::UnaryExp(_, e)
| E::Borrow(_, e, _, _)
| E::Cast(e, _) => calls_special_function_exp(special, e),
| E::Cast(e, _) => exp_satisfies_(e, p),

E::BinopExp(el, _, er) => {
calls_special_function_exp(special, el) || calls_special_function_exp(special, er)
}
E::BinopExp(el, _, er) => exp_satisfies_(el, p) || exp_satisfies_(er, p),

E::ModuleCall(call) => {
special.iter().any(|(a, m, f)| call.is(*a, *m, *f))
|| call
.arguments
.iter()
.any(|arg| calls_special_function_exp(special, arg))
}
E::Vector(_, _, _, es) | E::Multiple(es) => {
es.iter().any(|e| calls_special_function_exp(special, e))
}
E::ModuleCall(call) => call.arguments.iter().any(|arg| exp_satisfies_(arg, p)),
E::Vector(_, _, _, es) | E::Multiple(es) => es.iter().any(move |e| exp_satisfies_(e, p)),

E::Pack(_, _, es) | E::PackVariant(_, _, _, es) => es
.iter()
.any(|(_, _, e)| calls_special_function_exp(special, e)),
E::Pack(_, _, es) | E::PackVariant(_, _, _, es) => {
es.iter().any(|(_, _, e)| exp_satisfies_(e, p))
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::{
shared::CompilationEnv,
typing::{
ast::{self as T, UnannotatedExp_},
visitor::{has_special_exp, TypingVisitorConstructor, TypingVisitorContext},
visitor::{exp_satisfies, TypingVisitorConstructor, TypingVisitorContext},
},
};

Expand Down Expand Up @@ -63,5 +63,5 @@ impl TypingVisitorContext for Context<'_> {
}

fn has_return(e: &T::Exp) -> bool {
has_special_exp(e, |e| matches!(e.exp.value, UnannotatedExp_::Return(_)))
exp_satisfies(e, |e| matches!(e.exp.value, UnannotatedExp_::Return(_)))
}
22 changes: 18 additions & 4 deletions external-crates/move/crates/move-compiler/src/sui_mode/id_leak.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,16 @@ use crate::{
absint::JoinResult,
cfg::ImmForwardCFG,
visitor::{
LocalState, SimpleAbsInt, SimpleAbsIntConstructor, SimpleDomain, SimpleExecutionContext,
cfg_satisfies, LocalState, SimpleAbsInt, SimpleAbsIntConstructor, SimpleDomain,
SimpleExecutionContext,
},
CFGContext, MemberName,
},
diag,
diagnostics::{Diagnostic, Diagnostics},
editions::Flavor,
expansion::ast::{ModuleIdent, TargetKind},
hlir::ast::{Exp, Label, ModuleCall, SingleType, Type, Type_, Var},
hlir::ast::{self as H, Exp, Label, ModuleCall, SingleType, Type, Type_, Var},
parser::ast::Ability_,
shared::{program_info::TypingProgramInfo, CompilationEnv, Identifier},
sui_mode::{OBJECT_NEW, TEST_SCENARIO_MODULE_NAME, TS_NEW_OBJECT},
Expand Down Expand Up @@ -95,7 +96,7 @@ impl SimpleAbsIntConstructor for IDLeakVerifier {
fn new<'a>(
env: &CompilationEnv,
context: &'a CFGContext<'a>,
_cfg: &ImmForwardCFG,
cfg: &ImmForwardCFG,
_init_state: &mut <Self::AI<'a> as SimpleAbsInt>::State,
) -> Option<Self::AI<'a>> {
let module = &context.module;
Expand Down Expand Up @@ -125,6 +126,19 @@ impl SimpleAbsIntConstructor for IDLeakVerifier {
}
}

// skip any function that doesn't create an object
cfg_satisfies(
cfg,
|_| true,
|e| {
use H::UnannotatedExp_ as E;
matches!(
&e.exp.value,
E::Pack(s, _, _) if minfo.structs.get(s).is_some_and(|s| s.abilities.has_ability_(Ability_::Key)),
)
},
);

Some(IDLeakVerifierAI {
module,
info: context.info,
Expand Down Expand Up @@ -157,7 +171,7 @@ impl<'a> SimpleAbsInt for IDLeakVerifierAI<'a> {
state: &mut State,
e: &Exp,
) -> Option<Vec<Value>> {
use crate::hlir::ast::UnannotatedExp_ as E;
use H::UnannotatedExp_ as E;

let e__ = &e.exp.value;
let E::Pack(s, _tys, fields) = e__ else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ use crate::{
absint::JoinResult,
cfg::ImmForwardCFG,
visitor::{
LocalState, SimpleAbsInt, SimpleAbsIntConstructor, SimpleDomain, SimpleExecutionContext,
calls_special_function, LocalState, SimpleAbsInt, SimpleAbsIntConstructor,
SimpleDomain, SimpleExecutionContext,
},
CFGContext, MemberName,
},
Expand Down Expand Up @@ -88,13 +89,28 @@ impl SimpleAbsIntConstructor for CustomStateChangeVerifier {
fn new<'a>(
_env: &CompilationEnv,
context: &'a CFGContext<'a>,
_cfg: &ImmForwardCFG,
_init_state: &mut State,
cfg: &ImmForwardCFG,
init_state: &mut State,
) -> Option<Self::AI<'a>> {
let MemberName::Function(fn_name) = context.member else {
return None;
};

if !init_state
.locals
.values()
.any(|state| matches!(state, LocalState::Available(_, Value::LocalObjWithStore(_))))
{
// if there is no object parameter with store, we can skip the function
// since this is the only case which will trigger the warning
return None;
}

if !calls_special_function(PRIVATE_OBJ_FUNCTIONS, cfg) {
// if the function does not call any of the private transfer functions, we can skip it
return None;
}

Some(CustomStateChangeVerifierAI {
fn_name_loc: fn_name.loc,
})
Expand Down Expand Up @@ -134,34 +150,34 @@ impl SimpleAbsInt for CustomStateChangeVerifierAI {
.find(|(addr, module, fun)| f.is(addr, module, fun))
{
if let Value::LocalObjWithStore(obj_addr_loc) = args[0] {
let msg = format!(
"Potential unintended implementation of a custom {} function.",
fname
);
let (op, action) = if *fname == TRANSFER_FUN {
("transfer", "transferred")
} else if *fname == SHARE_FUN {
("share", "shared")
} else if *fname == FREEZE_FUN {
("freeze", "frozen")
} else {
("receive", "received")
let (op, action) = match *fname {
TRANSFER_FUN => ("transfer", "transferred"),
SHARE_FUN => ("share", "shared"),
FREEZE_FUN => ("freeze", "frozen"),
RECEIVE_FUN => ("receive", "received"),
s => unimplemented!("Unexpected private obj function {s}"),
};
let msg = format!("Potential unintended implementation of a custom {op} function.");
let uid_msg = format!(
"Instances of a type with a store ability can be {action} using \
the public_{fname} function which often negates the intent \
of enforcing a custom {op} policy"
"Instances of a type with a 'store' ability can be {action} using \
the 'public_{fname}' function which often negates the intent \
of enforcing a custom {op} policy"
);
let note_msg = format!(
"A custom {op} policy for a given type is implemented through \
calling the private '{fname}' function variant in the module defining this type"
);
let note_msg = format!("A custom {op} policy for a given type is implemented through calling \
the private {fname} function variant in the module defining this type");
let mut d = diag!(
CUSTOM_STATE_CHANGE_DIAG,
(self.fn_name_loc, msg),
(f.name.loc(), uid_msg)
);
d.add_note(note_msg);
if obj_addr_loc != INVALID_LOC {
let loc_msg = format!("An instance of a module-private type with a store ability to be {} coming from here", action);
let loc_msg = format!(
"An instance of a module-private type with a \
'store' ability to be {action} coming from here"
);
d.add_secondary_label((obj_addr_loc, loc_msg));
}
context.add_diag(d)
Expand Down Expand Up @@ -203,6 +219,7 @@ impl SimpleDomain for State {
for (_mut, v, st) in &context.signature.parameters {
if is_local_obj_with_store(st, context) {
let local_state = locals.get_mut(v).unwrap();
debug_assert!(matches!(local_state, LocalState::Available(_, _)));
if let LocalState::Available(loc, _) = local_state {
*local_state = LocalState::Available(*loc, Value::LocalObjWithStore(*loc));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ use crate::{
absint::JoinResult,
cfg::ImmForwardCFG,
visitor::{
LocalState, SimpleAbsInt, SimpleAbsIntConstructor, SimpleDomain, SimpleExecutionContext,
calls_special_function, LocalState, SimpleAbsInt, SimpleAbsIntConstructor,
SimpleDomain, SimpleExecutionContext,
},
CFGContext, MemberName,
},
Expand Down Expand Up @@ -81,7 +82,7 @@ impl SimpleAbsIntConstructor for SelfTransferVerifier {
fn new<'a>(
_env: &CompilationEnv,
context: &'a CFGContext<'a>,
_cfg: &ImmForwardCFG,
cfg: &ImmForwardCFG,
_init_state: &mut <Self::AI<'a> as SimpleAbsInt>::State,
) -> Option<Self::AI<'a>> {
let MemberName::Function(name) = context.member else {
Expand All @@ -107,6 +108,10 @@ impl SimpleAbsIntConstructor for SelfTransferVerifier {
// values instead of using transfer
return None;
}
if !calls_special_function(TRANSFER_FUNCTIONS, cfg) {
// skip if it does not use transfer functions
return None;
}
Some(SelfTransferVerifierAI {
fn_ret_loc: context.signature.return_type.loc,
})
Expand Down
Loading

0 comments on commit c092832

Please sign in to comment.