From 0398c6868940af8561d3401c00441071c57d7ac5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20H=C3=B6gberg?= Date: Wed, 27 Mar 2019 15:32:29 +0100 Subject: erts: Remove unsafe bs_get_binary2 optimization from loader A load-time optimization assumed that match contexts had no further uses when a bs_get_binary2 overwrote the match context's register, and figured it would be safe to reuse the match context's memory for the resulting binary. This is no longer safe as of OTP 22, as a match context may be reused after being passed to another function. --- erts/emulator/beam/beam_load.c | 19 ++++++------------- erts/emulator/beam/bs_instrs.tab | 11 ----------- erts/emulator/beam/ops.tab | 1 - erts/emulator/test/bs_match_misc_SUITE.erl | 21 +++++++++++++++++++-- 4 files changed, 25 insertions(+), 27 deletions(-) (limited to 'erts/emulator') diff --git a/erts/emulator/beam/beam_load.c b/erts/emulator/beam/beam_load.c index 21740caa2c..d0936060b8 100644 --- a/erts/emulator/beam/beam_load.c +++ b/erts/emulator/beam/beam_load.c @@ -3347,19 +3347,12 @@ gen_get_binary2(LoaderState* stp, GenOpArg Fail, GenOpArg Ms, GenOpArg Live, NATIVE_ENDIAN(Flags); if (Size.type == TAG_a && Size.val == am_all) { - if (Ms.type == Dst.type && Ms.val == Dst.val) { - GENOP_NAME_ARITY(op, i_bs_get_binary_all_reuse, 3); - op->a[0] = Ms; - op->a[1] = Fail; - op->a[2] = Unit; - } else { - GENOP_NAME_ARITY(op, i_bs_get_binary_all2, 5); - op->a[0] = Ms; - op->a[1] = Fail; - op->a[2] = Live; - op->a[3] = Unit; - op->a[4] = Dst; - } + GENOP_NAME_ARITY(op, i_bs_get_binary_all2, 5); + op->a[0] = Ms; + op->a[1] = Fail; + op->a[2] = Live; + op->a[3] = Unit; + op->a[4] = Dst; } else if (Size.type == TAG_i) { GENOP_NAME_ARITY(op, i_bs_get_binary_imm2, 6); op->a[0] = Ms; diff --git a/erts/emulator/beam/bs_instrs.tab b/erts/emulator/beam/bs_instrs.tab index 652460a66d..9cad2b03c5 100644 --- a/erts/emulator/beam/bs_instrs.tab +++ b/erts/emulator/beam/bs_instrs.tab @@ -1136,7 +1136,6 @@ i_bs_get_utf16.execute(Fail, Flags, Dst) { } bs_context_to_binary := ctx_to_bin.fetch.execute; -i_bs_get_binary_all_reuse := ctx_to_bin.fetch_bin.execute; ctx_to_bin.head() { Eterm context; @@ -1159,16 +1158,6 @@ ctx_to_bin.fetch(Src) { } } -ctx_to_bin.fetch_bin(Src, Fail, Unit) { - context = $Src; - mb = ms_matchbuffer(context); - size = mb->size - mb->offset; - if (size % $Unit != 0) { - $FAIL($Fail); - } - offs = mb->offset; -} - ctx_to_bin.execute() { Uint hole_size; Uint orig = mb->orig; diff --git a/erts/emulator/beam/ops.tab b/erts/emulator/beam/ops.tab index 6832e65b1b..abbddbb41f 100644 --- a/erts/emulator/beam/ops.tab +++ b/erts/emulator/beam/ops.tab @@ -1262,7 +1262,6 @@ bs_get_binary2 Fail=f Ms=xy Live=u Sz=sq Unit=u Flags=u Dst=d => \ i_bs_get_binary_imm2 xy f? t W t d i_bs_get_binary2 xy f t? s t d i_bs_get_binary_all2 xy f? t t d -i_bs_get_binary_all_reuse xy f? t # Fetching float from binaries. bs_get_float2 Fail=f Ms=xy Live=u Sz=s Unit=u Flags=u Dst=d => \ diff --git a/erts/emulator/test/bs_match_misc_SUITE.erl b/erts/emulator/test/bs_match_misc_SUITE.erl index 17759d78f3..cae4eb54d2 100644 --- a/erts/emulator/test/bs_match_misc_SUITE.erl +++ b/erts/emulator/test/bs_match_misc_SUITE.erl @@ -24,7 +24,7 @@ kenneth/1,encode_binary/1,native/1,happi/1, size_var/1,wiger/1,x0_context/1,huge_float_field/1, writable_binary_matched/1,otp_7198/1,unordered_bindings/1, - float_middle_endian/1]). + float_middle_endian/1,unsafe_get_binary_reuse/1]). -include_lib("common_test/include/ct.hrl"). @@ -36,7 +36,8 @@ all() -> [bound_var, bound_tail, t_float, little_float, sean, kenneth, encode_binary, native, happi, size_var, wiger, x0_context, huge_float_field, writable_binary_matched, - otp_7198, unordered_bindings, float_middle_endian]. + otp_7198, unordered_bindings, float_middle_endian, + unsafe_get_binary_reuse]. %% Test matching of bound variables. @@ -556,5 +557,21 @@ unordered_bindings(CompressedLength, HashSize, PadLength, T) -> Padding:PadLength/binary,PadLength>> = T, {Content,Mac,Padding}. +%% ERL-901: A load-time optimization assumed that match contexts had no further +%% uses when a bs_get_binary2 overwrote the match context's register, and +%% figured it would be safe to reuse the match context's memory for the +%% resulting binary. +%% +%% This is no longer safe as of OTP 22, as a match context may be reused after +%% being passed to another function. +unsafe_get_binary_reuse(Config) when is_list(Config) -> + <<_First, Rest/binary>> = <<"hello">>, + ubgr_1(Rest), + <> = Rest, + $e = Second, + ok. + +ubgr_1(<<_CP/utf8, Rest/binary>>) -> id(Rest); +ubgr_1(_) -> false. id(I) -> I. -- cgit v1.2.3