diff --git a/erts/emulator/beam/erl_gc.c b/erts/emulator/beam/erl_gc.c index 3b1fd5009b5d..928015a740fd 100644 --- a/erts/emulator/beam/erl_gc.c +++ b/erts/emulator/beam/erl_gc.c @@ -559,6 +559,11 @@ delay_garbage_collection(Process *p, int need, int fcalls) } p->abandoned_heap = orig_heap; erts_adjust_memory_break(p, orig_htop - p->high_water); + + /* Point at the end of the address range to ensure that + * test for the safe range in the new heap in the + * update_record_in_place instruction fails. */ + p->high_water = (Eterm *) (Uint) -1; } #ifdef CHECK_FOR_HOLES diff --git a/erts/emulator/beam/jit/arm/instr_common.cpp b/erts/emulator/beam/jit/arm/instr_common.cpp index c06c9a399dad..397d5b88fe2f 100644 --- a/erts/emulator/beam/jit/arm/instr_common.cpp +++ b/erts/emulator/beam/jit/arm/instr_common.cpp @@ -1033,7 +1033,7 @@ void BeamModuleAssembler::emit_update_record_in_place( if (!maybe_immediate.isNil()) { auto value = load_source(maybe_immediate, ARG5); - emit_is_not_boxed(update, value.reg); + emit_is_boxed(update, value.reg); } a.ldr(ARG4, arm::Mem(c_p, offsetof(Process, high_water))); @@ -1071,6 +1071,29 @@ void BeamModuleAssembler::emit_update_record_in_place( a.add(destination.reg, untagged_src, TAG_PRIMARY_BOXED); flush_var(destination); + +#ifdef DEBUG + if (!all_safe && maybe_immediate.isNil()) { + Label pointer_ok = a.newLabel(); + + /* If p->high_water contained a garbage value, a tuple not in + * the safe part of the new heap could have been destructively + * updated. */ + comment("sanity-checking tuple pointer"); + mov_arg(ARG2, Dst); + a.ldr(ARG4, arm::Mem(c_p, offsetof(Process, heap))); + a.cmp(ARG2, HTOP); + a.ccmp(ARG2, ARG4, imm(NZCV::kNone), imm(arm::CondCode::kLO)); + a.b_hs(pointer_ok); + + emit_enter_runtime(); + a.mov(ARG1, c_p); + runtime_call<2>(beam_jit_invalid_heap_ptr); + emit_leave_runtime(); + + a.bind(pointer_ok); + } +#endif } void BeamModuleAssembler::emit_set_tuple_element(const ArgSource &Element, diff --git a/erts/emulator/beam/jit/beam_jit_common.cpp b/erts/emulator/beam/jit/beam_jit_common.cpp index 1f2957addbab..73f86ebbdefa 100644 --- a/erts/emulator/beam/jit/beam_jit_common.cpp +++ b/erts/emulator/beam/jit/beam_jit_common.cpp @@ -1365,3 +1365,29 @@ bool beam_jit_is_shallow_boxed(Eterm term) { return false; } } + +#ifdef DEBUG +void beam_jit_invalid_heap_ptr(Process *p, Eterm term) { + ASSERT((void *)term <= (void *)p->heap || (void *)term >= (void *)p->hend); + + erts_fprintf(stderr, + "term: %p\n" + "c_p: %p\n" + "heap: %p\n" + "high_water: %p\n" + "hend: %p\n" + "abandoned: %p\n", + (void *)term, + p, + p->heap, + p->high_water, + p->hend, + p->abandoned_heap); + if (p->old_heap != NULL && p->old_hend != NULL && + (void *)term < (void *)p->old_hend && + (void *)term >= (void *)p->old_heap) { + erts_fprintf(stderr, "the term is on the old heap\n"); + } + abort(); +} +#endif diff --git a/erts/emulator/beam/jit/beam_jit_common.hpp b/erts/emulator/beam/jit/beam_jit_common.hpp index d11c8768ca33..6f6422d46094 100644 --- a/erts/emulator/beam/jit/beam_jit_common.hpp +++ b/erts/emulator/beam/jit/beam_jit_common.hpp @@ -659,4 +659,8 @@ Export *beam_jit_handle_unloaded_fun(Process *c_p, bool beam_jit_is_list_of_immediates(Eterm term); bool beam_jit_is_shallow_boxed(Eterm term); +#ifdef DEBUG +void beam_jit_invalid_heap_ptr(Process *p, Eterm term); +#endif + #endif diff --git a/erts/emulator/beam/jit/x86/instr_common.cpp b/erts/emulator/beam/jit/x86/instr_common.cpp index eb93f52df210..d722a19d3730 100644 --- a/erts/emulator/beam/jit/x86/instr_common.cpp +++ b/erts/emulator/beam/jit/x86/instr_common.cpp @@ -1038,7 +1038,7 @@ void BeamModuleAssembler::emit_update_record_in_place( if (!maybe_immediate.isNil()) { mov_arg(ARG4, maybe_immediate); preserve_cache([&]() { - emit_is_not_boxed(update, ARG4, dShort); + emit_is_boxed(update, ARG4, dShort); }); } @@ -1078,6 +1078,34 @@ void BeamModuleAssembler::emit_update_record_in_place( } mov_arg(Dst, RET); + +#ifdef DEBUG + if (!all_safe && maybe_immediate.isNil()) { + Label bad_pointer = a.newLabel(), pointer_ok = a.newLabel(); + + /* If p->high_water contained a garbage value, a tuple not in + * the safe part of the new heap could have been destructively + * updated. */ + comment("sanity-checking tuple pointer"); + a.mov(ARG1, x86::Mem(c_p, offsetof(Process, heap))); + a.cmp(RET, HTOP); + a.short_().jae(bad_pointer); + + a.cmp(RET, ARG1); + a.short_().jae(pointer_ok); + + a.bind(bad_pointer); + { + emit_enter_runtime(); + a.mov(ARG1, c_p); + a.mov(ARG2, RET); + runtime_call<2>(beam_jit_invalid_heap_ptr); + emit_leave_runtime(); + } + + a.bind(pointer_ok); + } +#endif } void BeamModuleAssembler::emit_set_tuple_element(const ArgSource &Element, diff --git a/erts/emulator/test/tuple_SUITE.erl b/erts/emulator/test/tuple_SUITE.erl index c53ec5f1fa0e..11967b78f368 100644 --- a/erts/emulator/test/tuple_SUITE.erl +++ b/erts/emulator/test/tuple_SUITE.erl @@ -26,7 +26,8 @@ t_make_tuple_2/1, t_make_upper_boundry_tuple_2/1, t_make_tuple_3/1, t_append_element/1, t_append_element_upper_boundry/1, build_and_match/1, tuple_with_case/1, tuple_in_guard/1, - get_two_tuple_elements/1]). + get_two_tuple_elements/1, + record_update/1]). -include_lib("common_test/include/ct.hrl"). %% Tests tuples and the BIFs: @@ -49,12 +50,14 @@ all() -> t_append_element, t_append_element_upper_boundry, t_insert_element, t_delete_element, tuple_with_case, tuple_in_guard, - get_two_tuple_elements]. + get_two_tuple_elements, + record_update]. groups() -> []. init_per_suite(Config) -> + id(Config), A0 = case application:start(sasl) of ok -> [sasl]; _ -> [] @@ -630,10 +633,78 @@ get_two_tuple_elements(Config) -> ok. + +%% Do some basic tests of the destructive tuple update added in +%% Erlang/OTP 27 (commit 8d4df9ae9fc9d1289b3c5a37e5bc6d73abb73297). + +-record(rec, {a,b,c,d,e}). +record_update(_Config) -> + N = id(100_000), + First = id(1 bsl 64), + Last = id(First + N), + + #rec{a=Last} = record_update_1(First, Last, #rec{b=id(42)}), + #rec{a=N} = record_update_1(id(0), N, #rec{b=id(42)}), + + #rec{a=Last} = record_update_2(First, Last, #rec{b=id(42)}), + + #rec{a=List} = record_update_3(id([]), N, #rec{b=id(42)}), + N = length(List), + true = lists:all(fun(a) -> true end, List), + + ok. + +record_update_1(I, N, R) when is_integer(I) -> + do_record_update_1(I, N, R). + +do_record_update_1(I, Last, R0) -> + %% An incorrect test for an immediate would allow a boxed term + %% (such as a bignum) to be written into an existing tuple without + %% testing whether the tuple were in the safe part of the heap. + R = R0#rec{a=I}, + + %% Force a minor collection here to force the tuple over to the + %% unsafe part of the new heap (below the high-water mark). + erlang:garbage_collect(self(), [{type,minor}]), + if + I < Last -> + do_record_update_1(I + 1, Last, R); + true -> + R + end. + +record_update_2(I, Last, R0) -> + R = R0#rec{a=I}, + + %% Force a minor collection here to force the tuple over to the + %% unsafe part of the new heap (below the high-water mark). + erlang:garbage_collect(self(), [{type,minor}]), + if + I < Last -> + record_update_2(I + 1, Last, R); + true -> + R + end. + +record_update_3(L, N, R) when is_list(L); is_integer(L) -> + do_record_update_3(L, N, R). + +do_record_update_3(L, N, R0) -> + %% Test that the basic test for the tuple being in the + %% safe part of the new heap is correct. + R = R0#rec{a=L}, + + if + N > 0 -> + do_record_update_3(id([a|L]), N-1, R); + true -> + R + end. + + %% Use this function to avoid compile-time evaluation of an expression. id(I) -> I. - sys_mem_cond_run(ReqSizeMB, TestFun) when is_integer(ReqSizeMB) -> case total_memory() of TotMem when is_integer(TotMem), TotMem >= ReqSizeMB ->