File 6483-Fix-unsafe-re-ordering-of-binary-clauses.patch of Package erlang
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