Skip to content

Commit

Permalink
Prevent dangerous re-cloning inside loop
Browse files Browse the repository at this point in the history
  • Loading branch information
muglug committed Oct 1, 2024
1 parent 7509f8c commit 580c18e
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 0 deletions.
43 changes: 43 additions & 0 deletions src/analyzer/expr/binop/assignment_analyzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,41 @@ pub(crate) fn analyze(
if let (Some(var_id), Some(existing_var_type), Bop::Eq(None)) =
(&var_id, &existing_var_type, binop)
{
if context.inside_loop && !context.inside_assignment_op {
if let Some(assign_value) = assign_value {
if let aast::Expr_::Clone(cloned_expr) = &assign_value.2 {
if let aast::Expr_::Lvar(cloned_var) = &cloned_expr.2 {
if cloned_var.name() == var_id {
let mut origin_node_ids = vec![];

for parent_node in &existing_var_type.parent_nodes {
origin_node_ids.extend(
analysis_data.data_flow_graph.get_origin_node_ids(
&parent_node.id,
&[],
false,
),
);
}

if origin_node_ids.len() > 1 {
analysis_data.maybe_add_issue(
Issue::new(
IssueKind::CloneInsideLoop,
format!("Overwriting an object {} outside the loop with a clone likely not intended", var_id),
statements_analyzer.get_hpos(pos),
&context.function_context.calling_functionlike_id,
),
statements_analyzer.get_config(),
statements_analyzer.get_file_path_actual(),
)
}
}
}
}
}
}

if context.inside_loop
&& !context.inside_assignment_op
&& context.for_loop_init_bounds.0 > 0
Expand All @@ -197,6 +232,14 @@ pub(crate) fn analyze(
));
}

if let Some(assign_value) = assign_value {
if let aast::Expr_::Clone(cloned_expr) = &assign_value.2 {
if let aast::Expr_::Lvar(cloned_var) = &cloned_expr.2 {
if cloned_var.name() == var_id {}
}
}
}

origin_node_ids.retain(|id| {
if let Some(node) = analysis_data.data_flow_graph.get_node(id) {
match (&id, &node.kind) {
Expand Down
1 change: 1 addition & 0 deletions src/code_info/issue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub enum IssueKind {
BannedFunction,
ExtendFinalClass,
CannotInferGenericParam,
CloneInsideLoop,
CustomIssue(Box<String>),
DuplicateEnumValue,
EmptyBlock,
Expand Down
11 changes: 11 additions & 0 deletions tests/inference/Loop/Foreach/cloneAssignmentInLoop/input.hack
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
final class A {
public function __construct(public int $num) {}
}

function foo(A $a, vec<int> $nums): void {
foreach ($nums as $num) {
$a = clone $a;
$a->num += $num;
echo $a->num;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ERROR: CloneInsideLoop - input.hack:7:6 - Overwriting an object $a outside the loop with a clone likely not intended

0 comments on commit 580c18e

Please sign in to comment.