From b9ba45314f7a5246d5ded383f4534119fa84194e Mon Sep 17 00:00:00 2001 From: Paul Guyot Date: Sun, 23 Apr 2023 11:29:28 +0200 Subject: [PATCH] Add workaround for crash related to `call_ext_last/3` BEAM's compiler and especially beam_trim optimization pass can generate an incorrect deallocation `n_words` parameter for `call_ext_last/3`. As a workaround, deallocate after the nif call, and before any error handling that may need the stack to find catch handlers. See: https://github.com/erlang/otp/issues/7152 Add test that currently crashes on master without this change Signed-off-by: Paul Guyot --- src/libAtomVM/opcodesswitch.h | 17 ++- tests/erlang_tests/CMakeLists.txt | 2 + .../erlang_tests/test_throw_call_ext_last.erl | 133 ++++++++++++++++++ tests/test.c | 1 + 4 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 tests/erlang_tests/test_throw_call_ext_last.erl diff --git a/src/libAtomVM/opcodesswitch.h b/src/libAtomVM/opcodesswitch.h index 7b1b6d28f..c8fbb6062 100644 --- a/src/libAtomVM/opcodesswitch.h +++ b/src/libAtomVM/opcodesswitch.h @@ -1595,9 +1595,6 @@ static bool maybe_call_native(Context *ctx, AtomString module_name, AtomString f TRACE_CALL_EXT(ctx, mod, "call_ext_last", index, arity); - ctx->cp = ctx->e[n_words]; - ctx->e += (n_words + 1); - const struct ExportedFunction *func = mod->imported_funcs[index].func; if (func->type == UnresolvedFunctionCall) { @@ -1617,11 +1614,25 @@ static bool maybe_call_native(Context *ctx, AtomString module_name, AtomString f } ctx->x[0] = return_value; + // We deallocate after (instead of before) as a + // workaround for issue + // https://github.com/erlang/otp/issues/7152 + + ctx->cp = ctx->e[n_words]; + ctx->e += (n_words + 1); + DO_RETURN(); break; } case ModuleFunction: { + // In the non-nif case, we can deallocate before + // (and it doesn't matter as the code below does + // not access ctx->e or ctx->cp) + + ctx->cp = ctx->e[n_words]; + ctx->e += (n_words + 1); + const struct ModuleFunction *jump = EXPORTED_FUNCTION_TO_MODULE_FUNCTION(func); mod = jump->target; diff --git a/tests/erlang_tests/CMakeLists.txt b/tests/erlang_tests/CMakeLists.txt index c08946d0e..279a4239c 100644 --- a/tests/erlang_tests/CMakeLists.txt +++ b/tests/erlang_tests/CMakeLists.txt @@ -425,6 +425,7 @@ compile_erlang(bs_context_to_binary_with_offset) compile_erlang(bs_restore2_start_offset) compile_erlang(test_refc_binaries) compile_erlang(test_sub_binaries) +compile_erlang(test_throw_call_ext_last) compile_erlang(bs_append_extra_words) compile_erlang(test_monotonic_time) @@ -839,6 +840,7 @@ add_custom_target(erlang_test_modules DEPENDS test_refc_binaries.beam test_sub_binaries.beam + test_throw_call_ext_last.beam test_function_exported.beam test_list_to_tuple.beam diff --git a/tests/erlang_tests/test_throw_call_ext_last.erl b/tests/erlang_tests/test_throw_call_ext_last.erl new file mode 100644 index 000000000..fadb361b9 --- /dev/null +++ b/tests/erlang_tests/test_throw_call_ext_last.erl @@ -0,0 +1,133 @@ +% +% This file is part of AtomVM. +% +% Copyright 2023 Paul Guyot +% +% Licensed under the Apache License, Version 2.0 (the "License"); +% you may not use this file except in compliance with the License. +% You may obtain a copy of the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, +% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +% See the License for the specific language governing permissions and +% limitations under the License. +% +% SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later +% + +-module(test_throw_call_ext_last). + +-export([start/0, loop/1]). + +-record(state, { + bin +}). + +start() -> + ok = run_test(fun() -> test_count_binary() end), + {error, {heap_delta, _Delta}} = run_test(fun() -> test_spawn_fun_sub_binary() end), + 0. + +test_spawn_fun_sub_binary() -> + Bin = create_binary(1024), + BinarySize = erlang:byte_size(Bin), + %% + %% Spawn a function, passing a refc binary through the args + %% + LargeSubBin = binary:part(Bin, 1, BinarySize - 1), + Pid = erlang:spawn(fun() -> loop(#state{bin = LargeSubBin}) end), + PidHeapSize0 = get_heap_size(Pid), + %% + %% Make sure we can get what we spawned + %% + LargeSubBin = send(Pid, get), + %% + %% Free the refc binary; heap should decrease + %% + ok = send(Pid, free), + PidHeapSize2 = get_heap_size(Pid), + case PidHeapSize2 - PidHeapSize0 of + 0 -> ok; + % should be call_ext_last + Delta -> throw({heap_delta, Delta}) + end, + ok = send(Pid, halt), + ok. + +test_count_binary() -> + _ = create_binary(1024), + ok. + +%% +%% helper functions +%% + +get_heap_size() -> + erlang:garbage_collect(), + {heap_size, Size} = erlang:process_info(self(), heap_size), + Size * erlang:system_info(wordsize). + +get_heap_size(Pid) -> + send(Pid, get_heap_size). + +send(Pid, Msg) -> + Ref = erlang:make_ref(), + Pid ! {self(), Ref, Msg}, + receive + {Ref, Reply} -> Reply + end. + +loop(State) -> + erlang:garbage_collect(), + receive + {Pid, Ref, get} -> + Pid ! {Ref, State#state.bin}, + loop(State); + {Pid, Ref, free} -> + Pid ! {Ref, ok}, + loop(State#state{bin = undefined}); + {Pid, Ref, get_heap_size} -> + Pid ! {Ref, get_heap_size()}, + loop(State); + {Pid, Ref, {ref, Bin}} -> + Pid ! {Ref, ok}, + loop(State#state{bin = Bin}); + {Pid, Ref, halt} -> + Pid ! {Ref, ok} + end. + +create_binary(N) when is_integer(N) -> + S = create_string(N, []), + R = erlang:list_to_binary(S), + R; +create_binary(S) when is_list(S) -> + list_to_binary(S). + +create_string(0, Accum) -> + Accum; +create_string(N, Accum) -> + create_string(N - 1, [N rem 256 | Accum]). + +run_test(Fun) -> + Self = self(), + _Pid = spawn(fun() -> execute(Self, Fun) end), + receive + ok -> + ok; + Error -> + Error + end. + +execute(Pid, Fun) -> + Result = + try + Fun(), + ok + catch + _:Error -> + {error, Error} + end, + Pid ! Result. diff --git a/tests/test.c b/tests/test.c index 75a084c11..de2f82e41 100644 --- a/tests/test.c +++ b/tests/test.c @@ -354,6 +354,7 @@ struct Test tests[] = { TEST_CASE(test_map), TEST_CASE_ATOMVM_ONLY(test_refc_binaries, 0), TEST_CASE(test_sub_binaries), + TEST_CASE_ATOMVM_ONLY(test_throw_call_ext_last, 0), TEST_CASE_EXPECTED(ceilint, 1), TEST_CASE_EXPECTED(ceilbadarg, -1),