File 2352-Warn-for-potentially-unsafe-use-of-get_sta... of Package erlang

Overview Repositories Revisions Requests Users Attributes Meta

File 2352-Warn-for-potentially-unsafe-use-of-get_stacktrace-0.patch of Package erlang

From 7b169140b2d37f43996b9d1a94877926a471d97d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Gustavsson?= <bjorn@erlang.org>
Date: Tue, 2 May 2017 05:41:17 +0200
Subject: [PATCH] Warn for potentially unsafe use of get_stacktrace/0

erlang:get_stacktrace/0 returns the stacktrace for the latest
exception.  The problem is that the stacktrace is kept until the next
exception occurs. If the last exception was a 'function_clause' or a
'badarg', the arguments for the call are also kept forever. The
arguments can be terms of any size (potentially huge).

In a future release, we would like to only allow
erlang:get_stacktrace/0 from within a 'try' expression. That would
make it possible to clear the stacktrace when the 'try' expression is
exited.

The 'catch' expression has no natural end where the stacktrace could
be cleared. The stacktrace could be cleared at the end of the function
that the 'catch' occurs in, but that would cause problems in the
following scenario (from real life, but simplified):

try
  ...
catch _:_ ->
  io:format(...),
  io:format("~p\n", [erlang:get_stacktrace()])
end.

%% In io.erl.
format(Fmt, Args) ->
  Res = case ... of
     SomePattern ->
       catch...
       ...;
     SomeOtherPattern ->
       %% Output the formatted string here
       ...
  end,
  clear_stacktrace(),  %% Inserted by compiler.
  Res.

The call to io:format() would always clear the stacktrace before
it could be retrieved.

That problem could be solved by tightning the scope in which the
stacktrace is kept, but the rules for how long erlang:get_stacktrace/0
would work would become complicated.

Therefore, the solution we suggest for a future major release of
OTP is that erlang:get_stacktrace/0 will return [] if it is called
outside the 'catch' part of a 'try' expression.

To help users prepare, introduce a warning when it is likely that
erlang:get_stacktrace/0 will always return an empty list, for example
in this code:

catch error(foo),
  Stk = erlang:get_stacktrace()

or in this code:

try Expr
  catch _:_ -> ok end,
  Stk = erlang:get_stacktrace()
---
 erts/doc/src/erlang.xml            | 28 +++++++++++++--
 lib/compiler/doc/src/compile.xml   |  8 +++++
 lib/stdlib/src/erl_lint.erl        | 73 ++++++++++++++++++++++++++++++++------
 lib/stdlib/test/erl_lint_SUITE.erl | 63 ++++++++++++++++++++++++++++++--
 4 files changed, 158 insertions(+), 14 deletions(-)

diff --git a/erts/doc/src/erlang.xml b/erts/doc/src/erlang.xml
index 6f70ae4a5..6d165e9ef 100644
--- a/erts/doc/src/erlang.xml
+++ b/erts/doc/src/erlang.xml
@@ -1813,8 +1813,9 @@ true</pre>
 <fsummary>Get the call stack back-trace of the last exception.</fsummary>
 <type name="stack_item"/>
 <desc>
- Gets the call stack back-trace (stacktrace) of the
- last exception in the calling process as a list of
+ Gets the call stack back-trace (stacktrace) for an
+ exception that has just been caught
+	 in the calling process as a list of
 <c>{<anno>Module</anno>,<anno>Function</anno>,<anno>Arity</anno>,<anno>Location</anno>}</c>
 tuples. Field <c><anno>Arity</anno></c> in the first tuple can be the
 argument list of that function call instead of an arity integer,
@@ -1822,6 +1823,29 @@ true</pre>
 If there has not been any exceptions in a process, the
 stacktrace is <c>[]</c>. After a code change for the process,
 the stacktrace can also be reset to <c>[]</c>.
+	<c>erlang:get_stacktrace/0</c> is only guaranteed to return
+	a stacktrace if called (directly or indirectly) from within the
+	scope of a <c>try</c> expression. That is, the following call works:
+<pre>
+try Expr
+catch
+ C:R ->
+ {C,R,erlang:get_stacktrace()}
+end</pre>
+	As does this call:
+<pre>
+try Expr
+catch
+ C:R ->
+ {C,R,helper()}
+end
+
+helper() ->
+ erlang:get_stacktrace().</pre>
+
+ <warning>In a future release,
+ <c>erlang:get_stacktrace/0</c> will return <c>[]</c> if called
+ from outside a <c>try</c> expression.</warning>
 The stacktrace is the same data as operator <c>catch</c>
 returns, for example:
 <pre>
diff --git a/lib/compiler/doc/src/compile.xml b/lib/compiler/doc/src/compile.xml
index fed68037c..10164890f 100644
--- a/lib/compiler/doc/src/compile.xml
+++ b/lib/compiler/doc/src/compile.xml
@@ -629,6 +629,14 @@ module.beam: module.erl \
 Turns off warnings for unused record types. Default is to 
 	 emit warnings for unused locally defined record types.
 </item>
+
+	 <tag><c>nowarn_get_stacktrace</c></tag>
+ <item>
+ Turns off warnings for using <c>get_stacktrace/0</c> in a context
+	 where it will probably not work in a future release. For example,
+	 by default there will be a warning if <c>get_stacktrace/0</c> is
+	 used following a <c>catch</c> expression.
+ </item>
 </taglist>
 
 	Another class of warnings is generated by the compiler
diff --git a/lib/stdlib/src/erl_lint.erl b/lib/stdlib/src/erl_lint.erl
index 7c40058dd..d53a31db0 100644
--- a/lib/stdlib/src/erl_lint.erl
+++ b/lib/stdlib/src/erl_lint.erl
@@ -92,6 +92,14 @@ value_option(Flag, Default, On, OnVal, Off, OffVal, Opts) ->
 :: dict:dict(ta(), line())
 }).
 
+
+%% Are we outside or inside a catch or try/catch?
+-type catch_scope() :: 'none'
+ | 'after_old_catch'
+ | 'after_try'
+ | 'wrong_part_of_try'
+ | 'try_catch'.
+
 %% Define the lint state record.
 %% 'called' and 'exports' contain {Line, {Function, Arity}},
 %% the other function collections contain {Function, Arity}.
@@ -135,7 +143,9 @@ value_option(Flag, Default, On, OnVal, Off, OffVal, Opts) ->
 types = dict:new() %Type definitions
 :: dict:dict(ta(), #typeinfo{}),
 exp_types=gb_sets:empty() %Exported types
- :: gb_sets:set(ta())
+ :: gb_sets:set(ta()),
+ catch_scope = none %Inside/outside try or catch
+ :: catch_scope()
 }).
 
 -type lint_state() :: #lint{}.
@@ -223,7 +233,15 @@ format_error({redefine_old_bif_import,{F,A}}) ->
 format_error({redefine_bif_import,{F,A}}) ->
 io_lib:format("import directive overrides auto-imported BIF ~w/~w~n"
 		 " - use \"-compile({no_auto_import,[~w/~w]}).\" to resolve name clash", [F,A,F,A]);
-
+format_error({get_stacktrace,wrong_part_of_try}) ->
+ "erlang:get_stacktrace/0 used in the wrong part of 'try' expression. "
+ "(Use it in the block between 'catch' and 'end'.)";
+format_error({get_stacktrace,after_old_catch}) ->
+ "erlang:get_stacktrace/0 used following an old-style 'catch' "
+ "may stop working in a future release. (Use it inside 'try'.)";
+format_error({get_stacktrace,after_try}) ->
+ "erlang:get_stacktrace/0 used following a 'try' expression "
+ "may stop working in a future release. (Use it inside 'try'.)";
 format_error({deprecated, MFA, ReplacementMFA, Rel}) ->
 io_lib:format("~s is deprecated and will be removed in ~s; use ~s",
 		 [format_mfa(MFA), Rel, format_mfa(ReplacementMFA)]);
@@ -568,7 +586,10 @@ start(File, Opts) ->
 		 false, Opts)},
 	 {missing_spec_all,
 	 bool_option(warn_missing_spec_all, nowarn_missing_spec_all,
-		 false, Opts)}
+		 false, Opts)},
+ {get_stacktrace,
+ bool_option(warn_get_stacktrace, nowarn_get_stacktrace,
+ true, Opts)}
 	],
 Enabled1 = [Category || {Category,true} <- Enabled0],
 Enabled = ordsets:from_list(Enabled1),
@@ -1405,8 +1426,9 @@ call_function(Line, F, A, #lint{usage=Usage0,called=Cd,func=Func,file=File}=St)
 %% function(Line, Name, Arity, Clauses, State) -> State.
 
 function(Line, Name, Arity, Cs, St0) ->
- St1 = define_function(Line, Name, Arity, St0#lint{func={Name,Arity}}),
- clauses(Cs, St1).
+ St1 = St0#lint{func={Name,Arity},catch_scope=none},
+ St2 = define_function(Line, Name, Arity, St1),
+ clauses(Cs, St2).
 
 -spec define_function(line(), atom(), arity(), lint_state()) -> lint_state().
 
@@ -2338,22 +2360,24 @@ expr({call,Line,F,As}, Vt, St0) ->
 expr({'try',Line,Es,Scs,Ccs,As}, Vt, St0) ->
 %% Currently, we don't allow any exports because later
 %% passes cannot handle exports in combination with 'after'.
- {Evt0,St1} = exprs(Es, Vt, St0),
+ {Evt0,St1} = exprs(Es, Vt, St0#lint{catch_scope=wrong_part_of_try}),
 TryLine = {'try',Line},
 Uvt = vtunsafe(TryLine, Evt0, Vt),
 Evt1 = vtupdate(Uvt, Evt0),
- {Sccs,St2} = icrt_clauses(Scs++Ccs, TryLine, vtupdate(Evt1, Vt), St1),
+ {Sccs,St2} = try_clauses(Scs, Ccs, TryLine,
+ vtupdate(Evt1, Vt), St1),
 Rvt0 = Sccs,
 Rvt1 = vtupdate(vtunsafe(TryLine, Rvt0, Vt), Rvt0),
 Evt2 = vtmerge(Evt1, Rvt1),
 {Avt0,St} = exprs(As, vtupdate(Evt2, Vt), St2),
 Avt1 = vtupdate(vtunsafe(TryLine, Avt0, Vt), Avt0),
 Avt = vtmerge(Evt2, Avt1),
- {Avt,St};
+ {Avt,St#lint{catch_scope=after_try}};
 expr({'catch',Line,E}, Vt, St0) ->
 %% No new variables added, flag new variables as unsafe.
 {Evt,St} = expr(E, Vt, St0),
- {vtupdate(vtunsafe({'catch',Line}, Evt, Vt), Evt),St};
+ {vtupdate(vtunsafe({'catch',Line}, Evt, Vt), Evt),
+ St#lint{catch_scope=after_old_catch}};
 expr({match,_Line,P,E}, Vt, St0) ->
 {Evt,St1} = expr(E, Vt, St0),
 {Pvt,Bvt,St2} = pattern(P, vtupdate(Evt, Vt), St1),
@@ -3173,6 +3197,17 @@ is_module_dialyzer_option(Option) ->
 error_handling,race_conditions,no_missing_calls,
 specdiffs,overspecs,underspecs,unknown]).
 
+%% try_catch_clauses(Scs, Ccs, In, ImportVarTable, State) ->
+%% {UpdVt,State}.
+
+try_clauses(Scs, Ccs, In, Vt, St0) ->
+ {Csvt0,St1} = icrt_clauses(Scs, Vt, St0),
+ St2 = St1#lint{catch_scope=try_catch},
+ {Csvt1,St3} = icrt_clauses(Ccs, Vt, St2),
+ Csvt = Csvt0 ++ Csvt1,
+ UpdVt = icrt_export(Csvt, Vt, In, St3),
+ {UpdVt,St3}.
+
 %% icrt_clauses(Clauses, In, ImportVarTable, State) ->
 %% {UpdVt,State}.
 
@@ -3657,7 +3692,8 @@ has_wildcard_field([]) -> false.
 check_remote_function(Line, M, F, As, St0) ->
 St1 = deprecated_function(Line, M, F, As, St0),
 St2 = check_qlc_hrl(Line, M, F, As, St1),
- format_function(Line, M, F, As, St2).
+ St3 = check_get_stacktrace(Line, M, F, As, St2),
+ format_function(Line, M, F, As, St3).
 
 %% check_qlc_hrl(Line, ModName, FuncName, [Arg], State) -> State
 %% Add warning if qlc:q/1,2 has been called but qlc.hrl has not
@@ -3706,6 +3742,23 @@ deprecated_function(Line, M, F, As, St) ->
 	 St
 end.
 
+check_get_stacktrace(Line, erlang, get_stacktrace, [], St) ->
+ case St of
+ #lint{catch_scope=none} ->
+ St;
+ #lint{catch_scope=try_catch} ->
+ St;
+ #lint{catch_scope=Scope} ->
+ case is_warn_enabled(get_stacktrace, St) of
+ false ->
+ St;
+ true ->
+ add_warning(Line, {get_stacktrace,Scope}, St)
+ end
+ end;
+check_get_stacktrace(_, _, _, _, St) ->
+ St.
+
 -dialyzer({no_match, deprecated_type/5}).
 
 deprecated_type(L, M, N, As, St) ->
-- 
2.13.0

Places

File 2352-Warn-for-potentially-unsafe-use-of-get_stacktrace-0.patch of Package erlang

Places