File 4873-Fix-unsafe-re-ordering-of-binary-clauses.p... of Package erlang

Overview Repositories Revisions Requests Users Attributes Meta

File 4873-Fix-unsafe-re-ordering-of-binary-clauses.patch of Package erlang (Revision 58ddfd9fcf4c281c82656d8c2d2a71df)

Currently displaying revision 58ddfd9fcf4c281c82656d8c2d2a71df , Show latest

From 9d76f481a0f829e1750c3fe4f56edcb236edc0e6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Gustavsson?= <bjorn@erlang.org>
Date: Mon, 22 May 2023 07:29:59 +0200
Subject: [PATCH] Fix unsafe re-ordering of binary clauses

Closes #7259
---
 lib/compiler/src/v3_kernel.erl       | 33 +++++++++++++++++++++++-----
 lib/compiler/test/bs_match_SUITE.erl | 23 ++++++++++++++++---
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/lib/compiler/src/v3_kernel.erl b/lib/compiler/src/v3_kernel.erl
index 809924c7f3..46063bccdf 100644
--- a/lib/compiler/src/v3_kernel.erl
+++ b/lib/compiler/src/v3_kernel.erl
@@ -1437,9 +1437,15 @@ match_fun(Val) ->
 reorder_bin_ints([_]=Cs) ->
     Cs;
 reorder_bin_ints(Cs0) ->
-    %% It is safe to reorder clauses that matches binaries if the
-    %% first segments for all of them match the same number of bits
-    %% and if the patterns that follow are also safe to re-order.
+    %% It is safe to reorder clauses that match binaries if all
+    %% of the followings conditions are true:
+    %%
+    %% * The first segments for all of them match the same number of
+    %%   bits (guaranteed by caller).
+    %%
+    %% * All segments have fixed sizes.
+    %%
+    %% * The patterns that follow are also safe to re-order.
     try
         Cs = sort([{reorder_bin_int_sort_key(C),C} || C <- Cs0]),
         [C || {_,C} <- Cs]
@@ -1448,7 +1454,7 @@ reorder_bin_ints(Cs0) ->
             Cs0
     end.
 
-reorder_bin_int_sort_key(#iclause{pats=[Pats|More],guard=#c_literal{val=true}}) ->
+reorder_bin_int_sort_key(#iclause{pats=[Pat|More],guard=#c_literal{val=true}}) ->
     case all(fun(#k_var{}) -> true;
                 (_) -> false
              end, More) of
@@ -1461,7 +1467,14 @@ reorder_bin_int_sort_key(#iclause{pats=[Pats|More],guard=#c_literal{val=true}})
             %%    f([<<"prefix">>, Variable]) -> ...
             throw(not_possible)
     end,
-    case Pats of
+
+    %% Ensure that the remaining segments have fixed sizes. For example, the following
+    %% clauses are not safe to re-order:
+    %%    f(<<"dd",_/binary>>) -> dd;
+    %%    f(<<"d",_/binary>>) -> d.
+    ensure_fixed_size(Pat#k_bin_int.next),
+
+    case Pat of
         #k_bin_int{val=Val,next=#k_bin_end{}} ->
             %% Sort before clauses with additional segments. This usually results in
             %% better code.
@@ -1472,6 +1485,16 @@ reorder_bin_int_sort_key(#iclause{pats=[Pats|More],guard=#c_literal{val=true}})
 reorder_bin_int_sort_key(#iclause{}) ->
     throw(not_possible).
 
+ensure_fixed_size(#k_bin_seg{size=Size,next=Next}) ->
+    case Size of
+        #k_literal{val=Sz} when is_integer(Sz) ->
+            ensure_fixed_size(Next);
+        _ ->
+            throw(not_possible)
+    end;
+ensure_fixed_size(#k_bin_end{}) ->
+    ok.
+
 %% match_value([Var], Con, [Clause], Default, State) -> {SelectExpr,State}.
 %%  At this point all the clauses have the same constructor, we must
 %%  now separate them according to value.
diff --git a/lib/compiler/test/bs_match_SUITE.erl b/lib/compiler/test/bs_match_SUITE.erl
index 1c379b70cf..dd47641c2d 100644
--- a/lib/compiler/test/bs_match_SUITE.erl
+++ b/lib/compiler/test/bs_match_SUITE.erl
@@ -19,7 +19,10 @@
 %%
 
 -module(bs_match_SUITE).
--compile(nowarn_shadow_vars).
+
+%% Limiting error locations to lines makes it more likely that unsafe
+%% reordering of clauses will be noticed.
+-compile([nowarn_shadow_vars, {error_location,line}]).
 
 -export([all/0, suite/0,groups/0,init_per_suite/1, end_per_suite/1, 
 	 init_per_group/2,end_per_group/2,
@@ -1327,7 +1330,7 @@ match_string(Config) when is_list(Config) ->
     %% To make sure that native endian really is handled correctly
     %% (i.e. that the compiler does not attempt to use bs_match_string/4
     %% instructions for native segments), running this test is not enough.
-    %% Either examine the generated for do_match_string_native/1 or
+    %% Either examine the generated code for do_match_string_native/1 or
     %% check the coverage for the v3_kernel module.
     case erlang:system_info(endian) of
 	little ->
@@ -1345,6 +1348,14 @@ match_string(Config) when is_list(Config) ->
     plain = no_match_string_opt(<<"abc">>),
     strange = no_match_string_opt(<<$a:9,$b:9,$c:9>>),
 
+    d = do_match_string_tail(id(<<"d">>)),
+    dd = do_match_string_tail(id(<<"dd">>)),
+
+    a = do_match_string_var_size(id(<<"a">>), id(0)),
+    a = do_match_string_var_size(id(<<"ab">>), id(8)),
+    ab = do_match_string_var_size(id(<<"ab">>), id(0)),
+    ab = do_match_string_var_size(id(<<"abc">>), id(8)),
+
     ok.
 
 do_match_string_native(<<$a:16/native,$b:16/native>>) -> ok.
@@ -1359,7 +1370,13 @@ do_match_string_little_signed(<<(-1):16/little-signed>>) -> ok.
 
 no_match_string_opt(<<"abc">>) -> plain;
 no_match_string_opt(<<$a:9,$b:9,$c:9>>) -> strange.
-    
+
+%% GH-7259: Unsafe reordering of clauses. (The clauses must be on the
+%% same line to trigger this bug.)
+do_match_string_tail(<<"dd", _T/binary>>) -> dd; do_match_string_tail(<<"d", _T/binary>>) -> d.
+
+do_match_string_var_size(Bin, Size) ->
+    case Bin of <<"ab",_T:Size>> -> ab; <<"a",_T:Size>> -> a end.
 
 %% OTP-7591: A zero-width segment in matching would crash the compiler.
 
-- 
2.35.3

Places

File 4873-Fix-unsafe-re-ordering-of-binary-clauses.patch of Package erlang (Revision 58ddfd9fcf4c281c82656d8c2d2a71df)

Places