sys_core_fold: Correct optimization of 'case'

The optimization of a 'case' statement could lead to incorrect
code that would cause an exception at run-time.

Here is an example to show how the optimization went wrong. Start
with the following code:

  f({r,#{key:=Val},X}=S) ->
    case S of
      {r,_,_} ->
        setelement(3, Val, S)
    end.

(The record operations have already been translated to the
corresponding tuple operations.) The first step in case_opt/3 is
to substitute S to obtain:

  f({r,#{key:=Val},X}=S) ->
    case {r,#{key:=Val},X} of
      {r,_,_} ->
        setelement(3, Val, S)
    end.

After that substitution the 'case' can be simplified to:

  f({r,#{key:=Val},_}=S) ->
    case #{key:=Val} of
      NewVar ->
       setelement(3, Val, S)
    end.

That is the result from case_opt/3. Now eval_case/2 notices
that since there is only one clause left in the 'case', the
'case' can eliminated:

  f({r,#{key:=Val},_}=S) ->
    NewVar = #{key:=Val},
    setelement(3, Val, S).

Since the map construction may have a side effect, it was not
eliminated, but assigned to a variable that is never used.

The problem is that '#{key:=Val}' is fine as a pattern, but in a
construction of a new map, the '=>' operator must be used. So the
map construction will fail, generating an exception.

As a conservative correction for a maintenance release, we will
abort the 'case' optimization if the substitution into the 'case'
expression is anything but data items (tuples, conses, or
literals) or variables.

Reported-by: Dmitry Aleksandrov

1 files changed, 36 insertions, 6 deletions

diff --git a/lib/compiler/src/sys_core_fold.erl b/lib/compiler/src/sys_core_fold.erl
index 82817a987a..09716d0866 100644
--- a/lib/compiler/src/sys_core_fold.erl
+++ b/lib/compiler/src/sys_core_fold.erl
@@ -2072,17 +2072,47 @@ maybe_replace_var_1(E, #sub{t=Tdb}) ->
 		false ->
 		    E;
 		true ->
-		    cerl_trees:map(fun(C) ->
-					   case cerl:is_c_alias(C) of
-					       false -> C;
-					       true -> cerl:alias_pat(C)
-					   end
-				   end, T0)
+		    %% The pattern was a tuple. Now we must make sure
+		    %% that the elements of the tuple are suitable. In
+		    %% particular, we don't want binary or map
+		    %% construction here, since that means that the
+		    %% binary or map will be constructed in the 'case'
+		    %% argument. That is wasteful for binaries. Even
+		    %% worse is that any map pattern that use the ':='
+		    %% operator will fail when used in map
+		    %% construction (only the '=>' operator is allowed
+		    %% when constructing a map from scratch).
+		    ToData = fun coerce_to_data/1,
+		    try
+			cerl_trees:map(ToData, T0)
+		    catch
+			throw:impossible ->
+			    %% Something unsuitable was found (map or
+			    %% or binary). Keep the variable.
+			    E
+		    end
 	    end;
 	error ->
 	    E
     end.
 
+%% coerce_to_data(Core) -> Core'
+%%  Coerce an element originally from a pattern to an data item or or
+%%  variable. Throw an 'impossible' exception if non-data Core Erlang
+%%  terms such as binary construction or map construction are
+%%  encountered.
+
+coerce_to_data(C) ->
+    case cerl:is_c_alias(C) of
+	false ->
+	    case cerl:is_data(C) orelse cerl:is_c_var(C) of
+		true -> C;
+		false -> throw(impossible)
+	    end;
+	true ->
+	    coerce_to_data(cerl:alias_pat(C))
+    end.
+
 %% case_opt_lit(Literal, Clauses0, LitExpr) ->
 %%           {ok,[],Clauses} | error
 %%  The current part of the case expression is a literal. That