SPropagator: Fix bad propagation for vvars defined in Assignment.src. (#6462)

angr/analyses/s_propagator.py

@@ -258,10 +258,16 @@ class SPropagatorAnalysis(Analysis):
 
                 block = blocks[(defloc.block_addr, defloc.block_idx)]
                 stmt = block.statements[defloc.stmt_idx]
+
+                if not (
+                    isinstance(stmt, Assignment) and isinstance(stmt.dst, VirtualVariable) and stmt.dst.varid == vvar_id
+                ):
+                    # come back later, this is not the def you're looking for
+                    continue
+
                 if (
                     (vvar.was_reg or vvar.was_parameter)
                     and sum(vvar_useloc_to_count.values()) <= 2
-                    and isinstance(stmt, Assignment)
                     and isinstance(stmt.src, Load)
                 ):
                     # do we want to propagate this Load expression if it's used for less than twice?
@@ -281,12 +287,7 @@ class SPropagatorAnalysis(Analysis):
                             self.replace(replacements, vvar_useloc, vvar_used, stmt.src)
                         continue
 
-                if (
-                    (vvar.was_reg or vvar.was_stack)
-                    and len(vvar_uselocs_set) == 2
-                    and isinstance(stmt, Assignment)
-                    and not is_phi_assignment(stmt)
-                ):
+                if (vvar.was_reg or vvar.was_stack) and len(vvar_uselocs_set) == 2 and not is_phi_assignment(stmt):
                     # a special case: in a typical switch-case construct, a variable may be used once for comparison
                     # for the default case and then used again for constructing the jump target. we can propagate this
                     # variable for such cases.
@@ -367,7 +368,7 @@ class SPropagatorAnalysis(Analysis):
 
                 # special logic for global variables: if it's used once or multiple times, and the variable is never
                 # updated before it's used, we will propagate the load
-                if (vvar.was_reg or vvar.was_parameter) and isinstance(stmt, Assignment) and not has_tmp_expr(stmt.src):
+                if (vvar.was_reg or vvar.was_parameter) and not has_tmp_expr(stmt.src):
                     stmt_src = stmt.src
                     # unpack conversions
                     while isinstance(stmt_src, Convert):

angr/utils/ssa/__init__.py

@@ -426,10 +426,19 @@ def has_load_expr_in_between_stmts(
     )
 
 
-def is_vvar_propagatable(vvar: VirtualVariable, def_stmt: Statement | None, stack_arg_offsets: set[int] | None) -> bool:
+def is_vvar_propagatable(vvar: VirtualVariable, def_stmt: Statement, stack_arg_offsets: set[int] | None) -> bool:
     if isinstance(def_stmt, Assignment) and isinstance(def_stmt.src, Insert):
         # do not create huge insert chains
         return False
+    if (
+        isinstance(def_stmt, Assignment)
+        and isinstance(def_stmt.dst, VirtualVariable)
+        and def_stmt.dst.varid != vvar.varid
+    ):
+        # the definition statement is not directly assigning to the vvar; this is probably because the vvar happens to
+        # be defined together in def_stmt.src, e.g., `vvar_781 = Reference(vvar_780)` where vvar_780 is first seen at
+        # this statement. we cannot propagate vvar_780.
+        return False
     if vvar.was_tmp or vvar.was_reg or vvar.was_parameter:
         return True
     if vvar.was_stack and isinstance(def_stmt, Assignment):

tests/analyses/decompiler/test_propagator_rules.py

@@ -57,6 +57,23 @@ class TestPropagatorRules(unittest.TestCase):
             else:
                 assert op_count <= 7
 
+    def test_spropagator_do_not_propagate_vvars_defined_in_assignment_src(self):
+        proj = angr.Project(
+            os.path.join(
+                test_location, "i386", "windows", "0c694dfa7ad465bded90c4faf63100c7008b5efc4bc49b38644a9770b42669b0"
+            )
+        )
+        _ = proj.analyses.CFG(force_smart_scan=False, normalize=True)
+        dec = proj.analyses.Decompiler(0x4847D4, fail_fast=True)
+        # it should not raise any exceptions; it was triggering an assertion error before this fix at
+        # ailment/expression.py:
+        #
+        # assert not isinstance(offset, Const) or offset.value * 8 + value.bits <= base.bits
+        #
+        # this is because we were trying to propagate Reference(vvar_780) to vvar_780 (16-byte) in a statement of
+        # `vvar_781 = Reference(vvar_780)`, where both vvar_780 and vvar_781 are defined at the same statement.
+        assert dec.codegen is not None and dec.codegen.text is not None
+
 
 if __name__ == "__main__":
     unittest.main()