Merge pull request #8622 from bjorng/bjorn/compiler/fix-bs-match/GH-8617

Fix unsafe code generation for binary matching
erlang · Jun 28, 2024 · e74dd94 · e74dd94
2 parents f335c66 + 60f8f9e
commit e74dd94
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 13 deletions.
diff --git a/lib/compiler/src/beam_ssa_opt.erl b/lib/compiler/src/beam_ssa_opt.erl
@@ -3623,7 +3623,7 @@ is_bs_match_blk(L, Blocks) ->
     Blk = map_get(L, Blocks),
     case Blk of
         #b_blk{is=Is,last=#b_br{bool=#b_var{}}=Last} ->
-            case is_bs_match_is(Is) of
+            case is_bs_match_is(Is, true) of
                 no ->
                     no;
                 {yes,CtxSizeUnit} ->
@@ -3634,20 +3634,33 @@ is_bs_match_blk(L, Blocks) ->
     end.
 
 is_bs_match_is([#b_set{op=bs_match,dst=Dst}=I,
-                #b_set{op={succeeded,guard},args=[Dst]}]) ->
-    case is_viable_match(I) of
-        no ->
+                #b_set{op={succeeded,guard},args=[Dst]}], Safe) ->
+    case Safe of
+        false ->
+            %% This `bs_match` (SSA) instruction was preceded by other
+            %% instructions (such as guard BIF calls) that would
+            %% prevent this match operation to be incorporated into
+            %% the commands list of a `bs_match` (BEAM) instruction.
             no;
-        {yes,{Ctx,Size,Unit}} when Size bsr 24 =:= 0 ->
-            %% Only include matches of reasonable size.
-            {yes,{{Ctx,Dst},Size,Unit}};
-        {yes,_} ->
-            %% Too large size.
-            no
+        true ->
+            case is_viable_match(I) of
+                no ->
+                    no;
+                {yes,{Ctx,Size,Unit}} when Size bsr 24 =:= 0 ->
+                    %% Only include matches of reasonable size.
+                    {yes,{{Ctx,Dst},Size,Unit}};
+                {yes,_} ->
+                    %% Too large size.
+                    no
+            end
     end;
-is_bs_match_is([_|Is]) ->
-    is_bs_match_is(Is);
-is_bs_match_is([]) -> no.
+is_bs_match_is([#b_set{op=bs_extract}|Is], Safe) ->
+    is_bs_match_is(Is, Safe);
+is_bs_match_is([#b_set{op=bs_start_match}|Is], _Safe) ->
+    is_bs_match_is(Is, true);
+is_bs_match_is([_|Is], _Safe) ->
+    is_bs_match_is(Is, false);
+is_bs_match_is([], _Safe) -> no.
 
 is_viable_match(#b_set{op=bs_match,args=Args}) ->
     case Args of

diff --git a/lib/compiler/test/bs_bincomp_SUITE.erl b/lib/compiler/test/bs_bincomp_SUITE.erl
@@ -663,12 +663,23 @@ grab_bag(_Config) ->
     %% Cover a line v3_kernel:get_line/1.
     _ = catch << ok || <<>> <= ok, ok >>,
 
+    [] = grab_bag_gh_8617(<<>>),
+    [0] = grab_bag_gh_8617(<<1:1>>),
+    [0,0,0] = grab_bag_gh_8617(<<0:3>>),
+
     ok.
 
 grab_bag_gh_6553(<<X>>) ->
     %% Would crash in beam_ssa_pre_codegen.
     <<X, ((<<_:0>> = <<_>>) = <<>>)>>.
 
+grab_bag_gh_8617(Bin) ->
+    %% GH-8617: CSE would cause a call self/0 to be inserted in
+    %% the middle of a sequence of `bs_match` instructions, causing
+    %% unsafe matching code to be emitted.
+    [0 || <<_:0, _:(tuple_size({self()}))>> <= Bin,
+          is_pid(id(self()))].
+
 cs_init() ->
     erts_debug:set_internal_state(available_internal_state, true),
     ok.