Merge pull request #1352 from bjorng/bjorn/asn1/better-error-messages/OTP-13961

Better error descriptions for ASN.1 encoding/decoding failures

10 files changed, 121 insertions, 78 deletions

diff --git a/lib/asn1/doc/src/asn1_getting_started.xml b/lib/asn1/doc/src/asn1_getting_started.xml
index d2b73d63c3..c036d289fc 100644
--- a/lib/asn1/doc/src/asn1_getting_started.xml
+++ b/lib/asn1/doc/src/asn1_getting_started.xml
@@ -266,6 +266,10 @@ asn1ct:compile("H323-MESSAGES.asn1",[per]).      </pre>
         <c>{error, {asn1, Description}}</c> where
         <c>Description</c> is
         an Erlang term describing the error.</p>
+	<p>Currently, <c>Description</c> looks like this:
+	<c>{ErrorDescription, StackTrace}</c>. Applications should
+	not depend on the exact contents of <c>Description</c> as it
+	could change in the future.</p>
     </section>
   </section>
 
diff --git a/lib/asn1/src/asn1ct_constructed_per.erl b/lib/asn1/src/asn1ct_constructed_per.erl
index b7579c8065..9cd9864b80 100644
--- a/lib/asn1/src/asn1ct_constructed_per.erl
+++ b/lib/asn1/src/asn1ct_constructed_per.erl
@@ -979,6 +979,10 @@ mark_optional(Other) ->
 gen_enc_components_call1(Gen, TopType, [C|Rest], DynamicEnc, Ext) ->
     #'ComponentType'{name=Cname,typespec=Type,
                      prop=Prop,textual_order=Num} = C,
+    InnerType = asn1ct_gen:get_inner(Type#type.def),
+    CommentString = attribute_comment(InnerType, Num, Cname),
+    ImmComment = asn1ct_imm:enc_comment(CommentString),
+
     {Imm0,Element} = enc_fetch_field(Gen, Num, Prop),
     Imm1 = gen_enc_line_imm(Gen, TopType, Cname, Type,
                             Element, DynamicEnc, Ext),
@@ -993,7 +997,7 @@ gen_enc_components_call1(Gen, TopType, [C|Rest], DynamicEnc, Ext) ->
 	   end,
     Imm = case Imm2 of
 	      [] -> [];
-	      _ -> Imm0 ++ Imm2
+	      _ -> [ImmComment|Imm0 ++ Imm2]
 	  end,
     [Imm|gen_enc_components_call1(Gen, TopType, Rest, DynamicEnc, Ext)];
 gen_enc_components_call1(_Gen, _TopType, [], _, _) ->
@@ -1328,27 +1332,17 @@ gen_dec_comp_calls([], _, _, _, _, _, _, Tpos, Acc) ->
 
 gen_dec_comp_call(Comp, Gen, TopType, Tpos, OptTable, DecInfObj,
 		  Ext, NumberOfOptionals) ->
-    #'ComponentType'{typespec=Type,prop=Prop,textual_order=TextPos} = Comp,
+    #'ComponentType'{name=Cname,typespec=Type,
+                     prop=Prop,textual_order=TextPos} = Comp,
     Pos = case Ext of
 	      noext -> Tpos;
 	      {ext,Epos,_Enum} -> Tpos - Epos + 1
 	  end,
-    InnerType = 
-	case Type#type.def of
-	    #'ObjectClassFieldType'{type=InType} ->
-		InType;
-	    Def ->
-		asn1ct_gen:get_inner(Def)
-	end,
+    InnerType = asn1ct_gen:get_inner(Type#type.def),
 
-    DispType = case InnerType of
-		   #'Externaltypereference'{type=T} -> T;
-		   IT when is_tuple(IT) -> element(2,IT);
-		   _ -> InnerType
-	       end,
+    CommentString = attribute_comment(InnerType, TextPos, Cname),
     Comment = fun(St) ->
-		      emit([nl,"%% attribute number ",TextPos,
-			    " with type ",DispType,nl]),
+		      emit([nl,"%% ",CommentString,nl]),
 		      St
 	      end,
 
@@ -1987,3 +1981,12 @@ enc_dig_out_value(#gen{pack=map}=Gen, [{_,Name}|T], Value) ->
 
 make_var(Base) ->
     {var,atom_to_list(asn1ct_gen:mk_var(asn1ct_name:curr(Base)))}.
+
+attribute_comment(InnerType, TextPos, Cname) ->
+    DispType = case InnerType of
+		   #'Externaltypereference'{type=T} -> T;
+		   IT when is_tuple(IT) -> element(2,IT);
+		   _ -> InnerType
+	       end,
+    Comment = ["attribute ",Cname,"(",TextPos,") with type ",DispType],
+    lists:concat(Comment).
diff --git a/lib/asn1/src/asn1ct_gen.erl b/lib/asn1/src/asn1ct_gen.erl
index 9943bd056a..9f628c7b04 100644
--- a/lib/asn1/src/asn1ct_gen.erl
+++ b/lib/asn1/src/asn1ct_gen.erl
@@ -802,11 +802,12 @@ result_line_1(Items) ->
 try_catch() ->
     ["  catch",nl,
      "    Class:Exception when Class =:= error; Class =:= exit ->",nl,
+     "      Stk = erlang:get_stacktrace(),",nl,
      "      case Exception of",nl,
-     "        {error,Reason}=Error ->",nl,
-     "          Error;",nl,
+     "        {error,{asn1,Reason}} ->",nl,
+     "          {error,{asn1,{Reason,Stk}}};",nl,
      "        Reason ->",nl,
-     "         {error,{asn1,Reason}}",nl,
+     "         {error,{asn1,{Reason,Stk}}}",nl,
      "      end",nl,
      "end."].
 
diff --git a/lib/asn1/src/asn1ct_imm.erl b/lib/asn1/src/asn1ct_imm.erl
index 2ab848652e..130f68c21d 100644
--- a/lib/asn1/src/asn1ct_imm.erl
+++ b/lib/asn1/src/asn1ct_imm.erl
@@ -41,7 +41,8 @@
          per_enc_extensions_map/4,
          per_enc_optional/2]).
 -export([per_enc_sof/5]).
--export([enc_absent/3,enc_append/1,enc_element/2,enc_maps_get/2]).
+-export([enc_absent/3,enc_append/1,enc_element/2,enc_maps_get/2,
+         enc_comment/1]).
 -export([enc_cg/2]).
 -export([optimize_alignment/1,optimize_alignment/2,
 	 dec_slim_cg/2,dec_code_gen/2]).
@@ -216,7 +217,8 @@ per_enc_legacy_bit_string(Val0, NNL0, Constraint0, Aligned) ->
 per_enc_boolean(Val0, _Aligned) ->
     {B,[Val]} = mk_vars(Val0, []),
     B++build_cond([[{eq,Val,false},{put_bits,0,1,[1]}],
-		   [{eq,Val,true},{put_bits,1,1,[1]}]]).
+		   [{eq,Val,true},{put_bits,1,1,[1]}],
+                   ['_',{error,{illegal_boolean,Val}}]]).
 
 per_enc_choice(Val0, Cs0, _Aligned) ->
     {B,[Val]} = mk_vars(Val0, []),
@@ -237,7 +239,7 @@ per_enc_enumerated(Val0, Root, Aligned) ->
     B++[{'cond',Cs++enumerated_error(Val)}].
 
 enumerated_error(Val) ->
-    [['_',{error,Val}]].
+    [['_',{error,{illegal_enumerated,Val}}]].
 
 per_enc_integer(Val0, Constraint0, Aligned) ->
     {B,[Val]} = mk_vars(Val0, []),
@@ -437,6 +439,9 @@ enc_maps_get(N, Val0) ->
     {var,SrcVar} = Val,
     {[{assign,DstExpr,SrcVar}],Dst0}.
 
+enc_comment(Comment) ->
+    {comment,Comment}.
+
 enc_cg(Imm0, false) ->
     Imm1 = enc_cse(Imm0),
     Imm2 = enc_pre_cg(Imm1),
@@ -874,10 +879,8 @@ flatten_map_cs_1([integer_default], {Int,_}) ->
     [{'_',Int}];
 flatten_map_cs_1([enum_default], {Int,_}) ->
     [{'_',["{asn1_enum,",Int,"}"]}];
-flatten_map_cs_1([enum_error], {Var,Cs}) ->
-    Vs = [V || {_,V} <- Cs],
-    [{'_',["exit({error,{asn1,{decode_enumerated,{",Var,",",
-	   {asis,Vs},"}}}})"]}];
+flatten_map_cs_1([enum_error], {Var,_}) ->
+    [{'_',["exit({error,{asn1,{decode_enumerated,",Var,"}}})"]}];
 flatten_map_cs_1([], _) -> [].
 
 flatten_hoist_align([[{align_bits,_,_}=Ab|T]|Cs]) ->
@@ -1051,6 +1054,7 @@ split_off_nonbuilding(Imm) ->
 
 is_nonbuilding({assign,_,_}) -> true;
 is_nonbuilding({call,_,_,_,_}) -> true;
+is_nonbuilding({comment,_}) -> true;
 is_nonbuilding({lc,_,_,_,_}) -> true;
 is_nonbuilding({set,_,_}) -> true;
 is_nonbuilding({list,_,_}) -> true;
@@ -1107,7 +1111,7 @@ per_enc_integer_1(Val0, [{{_,_}=Constr,[]}], Aligned) ->
 per_enc_integer_1(Val0, [Constr], Aligned) ->
     {Prefix,Check,Action} = per_enc_integer_2(Val0, Constr, Aligned),
     Prefix++build_cond([[Check|Action],
-			['_',{error,Val0}]]).
+			['_',{error,{illegal_integer,Val0}}]]).
 
 per_enc_integer_2(Val, {'SingleValue',Sv}, Aligned) when is_integer(Sv) ->
     per_enc_constrained(Val, Sv, Sv, Aligned);
@@ -1931,6 +1935,8 @@ enc_opt({'cond',Cs0}, St0) ->
 	    {Cs,Type} = enc_opt_cond_1(Cs1, Type0, [{Cond,Imm}]),
 	    {{'cond',Cs},St0#ost{t=Type}}
     end;
+enc_opt({comment,_}=Imm, St) ->
+    {Imm,St#ost{t=undefined}};
 enc_opt({cons,H0,T0}, St0) ->
     {H,#ost{t=TypeH}=St1} = enc_opt(H0, St0),
     {T,#ost{t=TypeT}=St} = enc_opt(T0, St1),
@@ -2320,6 +2326,9 @@ enc_cg({block,Imm}) ->
     enc_cg(Imm),
     emit([nl,
 	  "end"]);
+enc_cg({seq,{comment,Comment},Then}) ->
+    emit(["%% ",Comment,nl]),
+    enc_cg(Then);
 enc_cg({seq,First,Then}) ->
     enc_cg(First),
     emit([com,nl]),
@@ -2353,9 +2362,9 @@ enc_cg({'cond',Cs}) ->
     enc_cg_cond(Cs);
 enc_cg({error,Error}) when is_function(Error, 0) ->
     Error();
-enc_cg({error,Var0}) ->
+enc_cg({error,{Tag,Var0}}) ->
     Var = mk_val(Var0),
-    emit(["exit({error,{asn1,{illegal_value,",Var,"}}})"]);
+    emit(["exit({error,{asn1,{",Tag,",",Var,"}}})"]);
 enc_cg({integer,Int}) ->
     emit(mk_val(Int));
 enc_cg({lc,Body,Var,List}) ->
@@ -2618,6 +2627,8 @@ enc_opt_al({call,per_common,encode_unconstrained_number,[_]}=Call, _) ->
     {[Call],0};
 enc_opt_al({call,_,_,_,_}=Call, Al) ->
     {[Call],Al};
+enc_opt_al({comment,_}=Imm, Al) ->
+    {[Imm],Al};
 enc_opt_al({'cond',Cs0}, Al0) ->
     {Cs,Al} = enc_opt_al_cond(Cs0, Al0),
     {[{'cond',Cs}],Al};
@@ -2714,6 +2725,8 @@ per_fixup([{block,Block}|T]) ->
     [{block,per_fixup(Block)}|per_fixup(T)];
 per_fixup([{'assign',_,_}=H|T]) ->
     [H|per_fixup(T)];
+per_fixup([{comment,_}=H|T]) ->
+    [H|per_fixup(T)];
 per_fixup([{'cond',Cs0}|T]) ->
     Cs = [[C|per_fixup(Act)] || [C|Act] <- Cs0],
     [{'cond',Cs}|per_fixup(T)];
diff --git a/lib/asn1/src/asn1rtt_per_common.erl b/lib/asn1/src/asn1rtt_per_common.erl
index 3896cb7fa5..e7edfb1ee0 100644
--- a/lib/asn1/src/asn1rtt_per_common.erl
+++ b/lib/asn1/src/asn1rtt_per_common.erl
@@ -140,6 +140,8 @@ encode_relative_oid(Val) when is_tuple(Val) ->
 encode_relative_oid(Val) when is_list(Val) ->
     list_to_binary([e_object_element(X)||X <- Val]).
 
+encode_unconstrained_number(Val) when not is_integer(Val) ->
+    exit({error,{asn1,{illegal_integer,Val}}});
 encode_unconstrained_number(Val) when Val >= 0 ->
     if
 	Val < 16#80 ->
diff --git a/lib/asn1/test/asn1_SUITE_data/Prim.asn1 b/lib/asn1/test/asn1_SUITE_data/Prim.asn1
index 4fe0901683..91c8696e61 100644
--- a/lib/asn1/test/asn1_SUITE_data/Prim.asn1
+++ b/lib/asn1/test/asn1_SUITE_data/Prim.asn1
@@ -18,6 +18,8 @@ BEGIN
   IntExpPri ::=  [PRIVATE 51] EXPLICIT INTEGER
   IntExpApp ::=  [APPLICATION 52] EXPLICIT INTEGER
 
+  IntConstrained ::= INTEGER (0..255)
+
   IntEnum ::=  INTEGER {first(1),last(31)}
 
   Enum ::=  ENUMERATED {monday(1),tuesday(2),wednesday(3),thursday(4),
diff --git a/lib/asn1/test/ber_decode_error.erl b/lib/asn1/test/ber_decode_error.erl
index c0840e02d7..c45d130ff4 100644
--- a/lib/asn1/test/ber_decode_error.erl
+++ b/lib/asn1/test/ber_decode_error.erl
@@ -26,48 +26,41 @@ run([]) ->
     {ok,B}  = 'Constructed':encode('S3', {'S3',17}),
     [T,L|V] = binary_to_list(B),
     Bytes = list_to_binary([T,L+3|V] ++ [2,1,3]),
-    case 'Constructed':decode('S3', Bytes) of
-	{error,{asn1,{unexpected,_}}} -> ok
-    end,
+    {unexpected,_} = dec_error('S3', Bytes),
 
     %% Unexpected bytes must be accepted if there is an extensionmark
     {ok,{'S3ext',17}} = 'Constructed':decode('S3ext', Bytes),
 
     %% Truncated tag.
-    {error,{asn1,{invalid_tag,_}}} =
-	(catch 'Constructed':decode('I', <<31,255,255>>)),
+    {invalid_tag,_} = dec_error('I', <<31,255,255>>),
 
     %% Overlong tag.
-    {error,{asn1,{invalid_tag,_}}} =
-	(catch 'Constructed':decode('I', <<31,255,255,255,127>>)),
+    {invalid_tag,_} = dec_error('I', <<31,255,255,255,127>>),
 
     %% Invalid length.
-    {error,{asn1,{invalid_length,_}}} =
-	(catch 'Constructed':decode('I', <<8,255>>)),
+    {invalid_length,_} = dec_error('I', <<8,255>>),
 
     %% Other errors.
-    {error,{asn1,{invalid_value,_}}} =
-	(catch 'Constructed':decode('I', <<>>)),
+    {invalid_value,_} = dec_error('I', <<>>),
 
-    {error,{asn1,{invalid_value,_}}} =
-	(catch 'Constructed':decode('I', <<8,7>>)),
+    {invalid_value,_} = dec_error('I', <<8,7>>),
 
     %% Short indefinite length. Make sure that the decoder doesn't look
     %% beyond the end of binary when looking for a 0,0 terminator.
-    {error,{asn1,{invalid_length,_}}} =
-	(catch 'Constructed':decode('S', sub(<<8,16#80,0,0>>, 3))),
-    {error,{asn1,{invalid_length,_}}} =
-	(catch 'Constructed':decode('S', sub(<<8,16#80,0,0>>, 2))),
-    {error,{asn1,{invalid_length,_}}} =
-	(catch 'Constructed':decode('S', sub(<<40,16#80,1,1,255,0,0>>, 6))),
-    {error,{asn1,{invalid_length,_}}} =
-	(catch 'Constructed':decode('S', sub(<<40,16#80,1,1,255,0,0>>, 5))),
+    {invalid_length,_} = dec_error('S', sub(<<8,16#80,0,0>>, 3)),
+    {invalid_length,_} = dec_error('S', sub(<<8,16#80,0,0>>, 2)),
+    {invalid_length,_} = dec_error('S', sub(<<40,16#80,1,1,255,0,0>>, 6)),
+    {invalid_length,_} = dec_error('S', sub(<<40,16#80,1,1,255,0,0>>, 5)),
 
     %% A primitive must not be encoded with an indefinite length.
-    {error,{asn1,{invalid_length,_}}} =
-	(catch 'Constructed':decode('OS', <<4,128,4,3,97,98,99,0,0>>)),
+    {invalid_length,_} = dec_error('OS', <<4,128,4,3,97,98,99,0,0>>),
     ok.
 
+dec_error(T, Bin) ->
+    {error,{asn1,{Reason,Stk}}} = 'Constructed':decode(T, Bin),
+    [{_,_,_,_}|_] = Stk,
+    Reason.
+
 sub(Bin, Bytes) ->
     <<B:Bytes/binary,_/binary>> = Bin,
     B.
diff --git a/lib/asn1/test/testChoPrim.erl b/lib/asn1/test/testChoPrim.erl
index 573c482f2b..61b6ab2d05 100644
--- a/lib/asn1/test/testChoPrim.erl
+++ b/lib/asn1/test/testChoPrim.erl
@@ -31,10 +31,10 @@ bool(Rules) ->
     roundtrip('ChoCon', {int2,233}),
     case Rules of
 	ber ->
-	    {error,{asn1,{invalid_choice_type,wrong}}} =
-		(catch 'ChoPrim':encode('ChoCon', {wrong,233})),
-	    {error,{asn1,{invalid_choice_tag,_WrongTag}}} =
-		(catch 'ChoPrim':decode('ChoCon', <<131,2,0,233>>));
+	    {error,{asn1,{{invalid_choice_type,wrong},[_|_]}}} =
+                 (catch 'ChoPrim':encode('ChoCon', {wrong,233})),
+	    {error,{asn1,{{invalid_choice_tag,_WrongTag},[_|_]}}} =
+                 (catch 'ChoPrim':decode('ChoCon', <<131,2,0,233>>));
 	per ->
 	    ok;
 	uper ->
diff --git a/lib/asn1/test/testInfObjectClass.erl b/lib/asn1/test/testInfObjectClass.erl
index 560986fac9..540407fa51 100644
--- a/lib/asn1/test/testInfObjectClass.erl
+++ b/lib/asn1/test/testInfObjectClass.erl
@@ -33,19 +33,29 @@ main(Rule) ->
     roundtrip('Seq', Val),
     
     %% OTP-5783
-    {error,{asn1,{'Type not compatible with table constraint',
-		  {component,'ArgumentType'},
-		  {value,_},_}}} = 'InfClass':encode('Seq', {'Seq',12,13,1}),
+    {'Type not compatible with table constraint',
+     {component,'ArgumentType'},
+     {value,_},_} = enc_error('Seq', {'Seq',12,13,1}),
     Bytes2 = case Rule of
 		 ber ->
 		     <<48,9,2,1,12,2,1,11,2,1,1>>;
 		 _ ->
 		     <<1,12,1,11,1,1>>
 	     end,
-    {error,{asn1,{'Type not compatible with table constraint',
-		  {{component,_},
-		   {value,_B},_}}}} = 'InfClass':decode('Seq', Bytes2),
+    {'Type not compatible with table constraint',
+     {{component,_},
+      {value,_B},_}} = dec_error('Seq', Bytes2),
     ok.
 
 roundtrip(T, V) ->
     asn1_test_lib:roundtrip('InfClass', T, V).
+
+enc_error(T, V) ->
+    {error,{asn1,{Reason,Stk}}} = 'InfClass':encode(T, V),
+    [{_,_,_,_}|_] = Stk,
+    Reason.
+
+dec_error(T, Bin) ->
+    {error,{asn1,{Reason,Stk}}} = 'InfClass':decode(T, Bin),
+    [{_,_,_,_}|_] = Stk,
+    Reason.
diff --git a/lib/asn1/test/testPrim.erl b/lib/asn1/test/testPrim.erl
index 96a2dd6c79..b2933dfabc 100644
--- a/lib/asn1/test/testPrim.erl
+++ b/lib/asn1/test/testPrim.erl
@@ -34,15 +34,12 @@ bool(Rules) ->
     Types = ['Bool','BoolCon','BoolPri','BoolApp',
 	     'BoolExpCon','BoolExpPri','BoolExpApp'],
     [roundtrip(T, V) || T <- Types, V <- [true,false]],
-    case Rules of
-	ber ->
-	    [begin
-		 {error,{asn1,{encode_boolean,517}}} = enc_error(T, 517)
-	     end || T <- Types],
-	    ok;
-	_ ->
-	    ok
-    end.
+    Tag = case Rules of
+              ber -> encode_boolean;
+              _ -> illegal_boolean
+          end,
+    [{Tag,517} = enc_error(T, 517) || T <- Types],
+    ok.
 
 
 int(Rules) ->
@@ -60,10 +57,22 @@ int(Rules) ->
 	      123456789,12345678901234567890,
 	      -1,-2,-3,-4,-100,-127,-255,-256,-257,
 	      -1234567890,-2147483648],
-    [roundtrip(T, V) ||
-	T <- ['Int','IntCon','IntPri','IntApp',
-	      'IntExpCon','IntExpPri','IntExpApp'],
-	V <- [1|Values]],
+    Types = ['Int','IntCon','IntPri','IntApp',
+             'IntExpCon','IntExpPri','IntExpApp'],
+    _ = [roundtrip(T, V) || T <- Types, V <- [1|Values]],
+    Tag = case Rules of
+              ber -> encode_integer;
+              _ -> illegal_integer
+          end,
+    _ = [{Tag,V} = enc_error(T, V) ||
+            T <- Types, V <- [atom,42.0,{a,b,c}]],
+    case Rules of
+        ber ->
+            ok;
+        _ ->
+            _ = [{Tag,V} = enc_error('IntConstrained', V) ||
+                    V <- [atom,-1,256,42.0]]
+    end,
 
     %%==========================================================
     %% IntEnum ::=  INTEGER {first(1),last(31)} 
@@ -119,7 +128,11 @@ enum(Rules) ->
 
     roundtrip('Enum', monday),
     roundtrip('Enum', thursday),
-    {error,{asn1,{_,4}}} = enc_error('Enum', 4),
+    Tag = case Rules of
+              ber -> enumerated_not_in_range;
+              _ -> illegal_enumerated
+          end,
+    {Tag,4} = enc_error('Enum', 4),
 
     case Rules of
 	Per when Per =:= per; Per =:= uper ->
@@ -182,13 +195,15 @@ roundtrip(Type, Value, ExpectedValue) ->
 enc_error(T, V) ->
     case get(no_ok_wrapper) of
 	false ->
-	    'Prim':encode(T, V);
+	    {error,{asn1,{Reason,Stk}}} = 'Prim':encode(T, V),
+            [{_,_,_,_}|_] = Stk,
+            Reason;
 	true ->
 	    try 'Prim':encode(T, V) of
 		_ ->
 		    ?t:fail()
 	    catch
-		_:Reason ->
+		_:{error,{asn1,Reason}} ->
 		    Reason
 	    end
     end.