From d462b7ba4ab775069499415f43a905a831b5b68f Mon Sep 17 00:00:00 2001 From: sabiwara Date: Sat, 2 Nov 2024 12:40:09 +0900 Subject: [PATCH 1/3] Proposal: shallow validation of unquoted AST --- lib/elixir/src/elixir_quote.erl | 29 ++++++++++++++++---- lib/elixir/test/elixir/kernel/quote_test.exs | 22 +++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/lib/elixir/src/elixir_quote.erl b/lib/elixir/src/elixir_quote.erl index 8e2fe1a3110..6800a256544 100644 --- a/lib/elixir/src/elixir_quote.erl +++ b/lib/elixir/src/elixir_quote.erl @@ -1,6 +1,6 @@ -module(elixir_quote). -export([escape/3, linify/3, linify_with_context_counter/3, build/7, quote/2, has_unquotes/1, fun_to_quoted/1]). --export([dot/5, tail_list/3, list/2, validate_runtime/2]). %% Quote callbacks +-export([dot/5, tail_list/3, list/2, validate_runtime/2, shallow_validate_ast/1]). %% Quote callbacks -include("elixir.hrl"). -define(defs(Kind), Kind == def; Kind == defp; Kind == defmacro; Kind == defmacrop; Kind == '@'). @@ -15,7 +15,8 @@ aliases_hygiene=nil, imports_hygiene=nil, unquote=true, - generated=false + generated=false, + shallow_validate=false }). %% fun_to_quoted @@ -215,7 +216,8 @@ build(Meta, Line, File, Context, Unquote, Generated, E) -> file=VFile, unquote=Unquote, context=VContext, - generated=Generated + generated=Generated, + shallow_validate=true }, {Q, VContext, Acc3}. @@ -254,6 +256,19 @@ is_valid(context, Context) -> is_atom(Context) andalso (Context /= nil); is_valid(generated, Generated) -> is_boolean(Generated); is_valid(unquote, Unquote) -> is_boolean(Unquote). +shallow_validate_ast(Expr) -> + case shallow_valid_ast(Expr) of + true -> Expr; + false -> argument_error(<<"tried to unquote invalid AST: ", ('Elixir.Kernel':inspect(Expr))/binary>>) + end. + +shallow_valid_ast(Expr) when is_list(Expr) -> lists:all(fun shallow_valid_ast/1, Expr); +shallow_valid_ast(Expr) when is_atom(Expr); is_binary(Expr); is_number(Expr); is_pid(Expr) -> true; +shallow_valid_ast({Left, Right}) -> shallow_valid_ast(Left) andalso shallow_valid_ast(Right); +shallow_valid_ast({Atom, Meta, Args}) when is_atom(Atom), is_list(Meta), is_atom(Args) orelse is_list(Args) -> true; +shallow_valid_ast({Call, Meta, Args}) when is_list(Meta), is_list(Args) -> shallow_valid_ast(Call); +shallow_valid_ast(_Term) -> false. + quote({unquote_splicing, _, [_]}, #elixir_quote{unquote=true}) -> argument_error(<<"unquote_splicing only works inside arguments and block contexts, " "wrap it in parens if you want it to work with one-liners">>); @@ -283,8 +298,12 @@ do_quote({quote, Meta, [Opts, Arg]}, Q) when is_list(Meta) -> {'{}', [], [quote, meta(NewMeta, Q), [TOpts, TArg]]}; -do_quote({unquote, Meta, [Expr]}, #elixir_quote{unquote=true}) when is_list(Meta) -> - Expr; +% +do_quote({unquote, Meta, [Expr]}, #elixir_quote{unquote=true, shallow_validate=Validate}) when is_list(Meta) -> + case Validate of + true -> {{'.', Meta, [?MODULE, shallow_validate_ast]}, Meta, [Expr]}; + false -> Expr + end; %% Aliases diff --git a/lib/elixir/test/elixir/kernel/quote_test.exs b/lib/elixir/test/elixir/kernel/quote_test.exs index fe06a61f021..d140f78894a 100644 --- a/lib/elixir/test/elixir/kernel/quote_test.exs +++ b/lib/elixir/test/elixir/kernel/quote_test.exs @@ -789,4 +789,26 @@ defmodule Kernel.QuoteTest.HasUnquoteTest do refute :elixir_quote.has_unquotes(ast) end + + test "unquote with invalid AST (shallow check)" do + assert_raise ArgumentError, "tried to unquote invalid AST: %{unescaped: :map}", fn -> + quote do: unquote(%{unescaped: :map}) + end + + assert_raise ArgumentError, "tried to unquote invalid AST: {:bad_meta, nil, []}", fn -> + quote do: unquote({:bad_meta, nil, []}) + end + + assert_raise ArgumentError, "tried to unquote invalid AST: {:bad_arg, nil, 1}", fn -> + quote do: unquote({:bad_arg, nil, 1}) + end + + assert_raise ArgumentError, "tried to unquote invalid AST: {:bad_tuple}", fn -> + quote do: unquote({:bad_tuple}) + end + + assert_raise ArgumentError, ~r/tried to unquote invalid AST: #Reference + quote do: unquote(make_ref()) + end + end end From 96bb33a7349906941f2e5f8a1bdc52f9c50446b5 Mon Sep 17 00:00:00 2001 From: sabiwara Date: Sun, 3 Nov 2024 13:22:02 +0900 Subject: [PATCH 2/3] Do not check deeply, improve message, handle improper lists --- lib/elixir/src/elixir_quote.erl | 22 ++++++++---- lib/elixir/test/elixir/kernel/quote_test.exs | 37 +++++++++++--------- 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/lib/elixir/src/elixir_quote.erl b/lib/elixir/src/elixir_quote.erl index 6800a256544..46ee8910c44 100644 --- a/lib/elixir/src/elixir_quote.erl +++ b/lib/elixir/src/elixir_quote.erl @@ -259,15 +259,23 @@ is_valid(unquote, Unquote) -> is_boolean(Unquote). shallow_validate_ast(Expr) -> case shallow_valid_ast(Expr) of true -> Expr; - false -> argument_error(<<"tried to unquote invalid AST: ", ('Elixir.Kernel':inspect(Expr))/binary>>) + false -> argument_error( + <<"tried to unquote invalid AST: ", ('Elixir.Kernel':inspect(Expr))/binary, + "\nDid you forget to escape term using Macro.escape/1?">>) end. -shallow_valid_ast(Expr) when is_list(Expr) -> lists:all(fun shallow_valid_ast/1, Expr); -shallow_valid_ast(Expr) when is_atom(Expr); is_binary(Expr); is_number(Expr); is_pid(Expr) -> true; -shallow_valid_ast({Left, Right}) -> shallow_valid_ast(Left) andalso shallow_valid_ast(Right); -shallow_valid_ast({Atom, Meta, Args}) when is_atom(Atom), is_list(Meta), is_atom(Args) orelse is_list(Args) -> true; -shallow_valid_ast({Call, Meta, Args}) when is_list(Meta), is_list(Args) -> shallow_valid_ast(Call); -shallow_valid_ast(_Term) -> false. +shallow_valid_ast(Expr) when is_list(Expr) -> valid_ast_list(Expr); +shallow_valid_ast(Expr) -> valid_ast_elem(Expr). + +valid_ast_list([]) -> true; +valid_ast_list([Head | Tail]) -> valid_ast_elem(Head) andalso valid_ast_list(Tail); +valid_ast_list(_Improper) -> false. + +valid_ast_elem(Expr) when is_list(Expr); is_atom(Expr); is_binary(Expr); is_number(Expr); is_pid(Expr) -> true; +valid_ast_elem({Left, Right}) -> valid_ast_elem(Left) andalso valid_ast_elem(Right); +valid_ast_elem({Atom, Meta, Args}) when is_atom(Atom), is_list(Meta), is_atom(Args) orelse is_list(Args) -> true; +valid_ast_elem({Call, Meta, Args}) when is_list(Meta), is_list(Args) -> shallow_valid_ast(Call); +valid_ast_elem(_Term) -> false. quote({unquote_splicing, _, [_]}, #elixir_quote{unquote=true}) -> argument_error(<<"unquote_splicing only works inside arguments and block contexts, " diff --git a/lib/elixir/test/elixir/kernel/quote_test.exs b/lib/elixir/test/elixir/kernel/quote_test.exs index d140f78894a..5d720c5a5ce 100644 --- a/lib/elixir/test/elixir/kernel/quote_test.exs +++ b/lib/elixir/test/elixir/kernel/quote_test.exs @@ -791,24 +791,27 @@ defmodule Kernel.QuoteTest.HasUnquoteTest do end test "unquote with invalid AST (shallow check)" do - assert_raise ArgumentError, "tried to unquote invalid AST: %{unescaped: :map}", fn -> - quote do: unquote(%{unescaped: :map}) - end - - assert_raise ArgumentError, "tried to unquote invalid AST: {:bad_meta, nil, []}", fn -> - quote do: unquote({:bad_meta, nil, []}) - end - - assert_raise ArgumentError, "tried to unquote invalid AST: {:bad_arg, nil, 1}", fn -> - quote do: unquote({:bad_arg, nil, 1}) - end - - assert_raise ArgumentError, "tried to unquote invalid AST: {:bad_tuple}", fn -> - quote do: unquote({:bad_tuple}) + for term <- [ + %{unescaped: :map}, + 1..10, + {:bad_meta, nil, []}, + {:bad_arg, nil, 1}, + {:bad_tuple}, + make_ref(), + [:improper | :list], + [nested: {}] + ] do + message = """ + tried to unquote invalid AST: #{inspect(term)} + Did you forget to escape term using Macro.escape/1?\ + """ + + assert_raise ArgumentError, message, fn -> quote do: unquote(term) end end + end - assert_raise ArgumentError, ~r/tried to unquote invalid AST: #Reference - quote do: unquote(make_ref()) - end + test "unquote with invalid AST is not checked deeply" do + assert quote do: unquote(foo: [1 | 2]) == [foo: [1 | 2]] + assert quote do: unquote(foo: [bar: %{}]) == [foo: [bar: %{}]] end end From 2a74ab8c772c46dd42d1e5f80559dc1d461d1387 Mon Sep 17 00:00:00 2001 From: sabiwara Date: Sun, 3 Nov 2024 13:22:20 +0900 Subject: [PATCH 3/3] Fix other tests --- lib/elixir/lib/macro.ex | 3 ++- .../test/elixir/code_normalizer/quoted_ast_test.exs | 8 ++++++-- lib/elixir/test/elixir/kernel/expansion_test.exs | 4 ++-- lib/elixir/test/elixir/macro_test.exs | 10 +++++++--- lib/mix/test/mix/tasks/xref_test.exs | 8 +++++++- 5 files changed, 24 insertions(+), 9 deletions(-) diff --git a/lib/elixir/lib/macro.ex b/lib/elixir/lib/macro.ex index f86e9571cae..6a1923eaf03 100644 --- a/lib/elixir/lib/macro.ex +++ b/lib/elixir/lib/macro.ex @@ -814,7 +814,8 @@ defmodule Macro do iex> value = {:a, :b, :c} iex> quote do: unquote(value) - {:a, :b, :c} + ** (ArgumentError) tried to unquote invalid AST: {:a, :b, :c} + Did you forget to escape term using Macro.escape/1? `escape/2` is used to escape *values* (either directly passed or variable bound), while `quote/2` produces syntax trees for diff --git a/lib/elixir/test/elixir/code_normalizer/quoted_ast_test.exs b/lib/elixir/test/elixir/code_normalizer/quoted_ast_test.exs index 8b5877f799a..30730bafa9f 100644 --- a/lib/elixir/test/elixir/code_normalizer/quoted_ast_test.exs +++ b/lib/elixir/test/elixir/code_normalizer/quoted_ast_test.exs @@ -405,12 +405,16 @@ defmodule Code.Normalizer.QuotedASTTest do end test "range" do - assert quoted_to_string(quote(do: unquote(-1..+2))) == "-1..2" + assert quoted_to_string(quote(do: -1..+2)) == "-1..+2" assert quoted_to_string(quote(do: Foo.integer()..3)) == "Foo.integer()..3" - assert quoted_to_string(quote(do: unquote(-1..+2//-3))) == "-1..2//-3" + assert quoted_to_string(quote(do: -1..+2//-3)) == "-1..+2//-3" assert quoted_to_string(quote(do: Foo.integer()..3//Bar.bat())) == "Foo.integer()..3//Bar.bat()" + + # invalid AST + assert quoted_to_string(-1..+2) == "-1..2" + assert quoted_to_string(-1..+2//-3) == "-1..2//-3" end test "when" do diff --git a/lib/elixir/test/elixir/kernel/expansion_test.exs b/lib/elixir/test/elixir/kernel/expansion_test.exs index c762a5ca5d5..8a95d3d6c39 100644 --- a/lib/elixir/test/elixir/kernel/expansion_test.exs +++ b/lib/elixir/test/elixir/kernel/expansion_test.exs @@ -2871,11 +2871,11 @@ defmodule Kernel.ExpansionTest do test "handles invalid expressions" do assert_compile_error(~r"invalid quoted expression: {1, 2, 3}", fn -> - expand_env(quote(do: unquote({1, 2, 3})), __ENV__) + expand_env({1, 2, 3}, __ENV__) end) assert_compile_error(~r"invalid quoted expression: #Function\<", fn -> - expand(quote(do: unquote({:sample, fn -> nil end}))) + expand({:sample, fn -> nil end}) end) assert_compile_error(~r"invalid pattern in match", fn -> diff --git a/lib/elixir/test/elixir/macro_test.exs b/lib/elixir/test/elixir/macro_test.exs index 452dbef6cf4..227127855f8 100644 --- a/lib/elixir/test/elixir/macro_test.exs +++ b/lib/elixir/test/elixir/macro_test.exs @@ -839,7 +839,7 @@ defmodule MacroTest do end test "converts invalid AST with inspect" do - assert Macro.to_string(quote do: unquote(1..3)) == "1..3" + assert Macro.to_string(1..3) == "1..3" end end @@ -1172,12 +1172,16 @@ defmodule MacroTest do end test "range" do - assert macro_to_string(quote(do: unquote(-1..+2))) == "-1..2" + assert macro_to_string(quote(do: -1..+2)) == "-1..+2" assert macro_to_string(quote(do: Foo.integer()..3)) == "Foo.integer()..3" - assert macro_to_string(quote(do: unquote(-1..+2//-3))) == "-1..2//-3" + assert macro_to_string(quote(do: -1..+2//-3)) == "-1..+2//-3" assert macro_to_string(quote(do: Foo.integer()..3//Bar.bat())) == "Foo.integer()..3//Bar.bat()" + + # invalid AST + assert macro_to_string(-1..+2) == "-1..2" + assert macro_to_string(-1..+2//-3) == "-1..2//-3" end test "when" do diff --git a/lib/mix/test/mix/tasks/xref_test.exs b/lib/mix/test/mix/tasks/xref_test.exs index 5d9a7509491..1d231e14be5 100644 --- a/lib/mix/test/mix/tasks/xref_test.exs +++ b/lib/mix/test/mix/tasks/xref_test.exs @@ -63,7 +63,13 @@ defmodule Mix.Tasks.XrefTest do } output = [ - %{callee: {A, :b, 1}, caller_module: B, file: "lib/b.ex", line: 3} + %{callee: {A, :b, 1}, caller_module: B, file: "lib/b.ex", line: 3}, + %{ + callee: {:elixir_quote, :shallow_validate_ast, 1}, + caller_module: A, + file: "lib/a.ex", + line: 4 + } ] assert_all_calls(files, output)