Merge pull request #8539 from bjorng/bjorn/destructive-tuple-update/O…

…TP-19108

Fix unsafe edge cases for in-place update of records

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

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,

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

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

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,

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 ->