Skip to content

Commit ea3dc92

Browse files
pguyotpguyot
committed
Fix several issues with OP_BS_MATCH_STRING
1. OP_BS_MATCH_STRING would ignore matched binary string during comparisons, thus possibly making comparison of unallocated data. Probably harmless on microcontrollers, but Valgrind has the right to complain. 2. OP_BS_MATCH_STRING would also be disabled if MINIMUM_OTP_COMPILER_VERSION was greater than 25, but it can be generated (rarely) by OTP-27. 3. term_set_match_state_offset was called including in cases where there was no match. This may have been harmless as Erlang compiler may always dispose match state when match fails. Signed-off-by: Paul Guyot <pguyot@kallisys.net>
1 parent 1cb7e56 commit ea3dc92

File tree

2 files changed

+83
-36
lines changed

2 files changed

+83
-36
lines changed

‎src/libAtomVM/opcodesswitch.h

Lines changed: 43 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -4750,7 +4750,6 @@ HOT_FUNC int scheduler_entry_point(GlobalContext *glb)
47504750
}
47514751
#endif
47524752

4753-
#if MINIMUM_OTP_COMPILER_VERSION <= 25
47544753
case OP_BS_MATCH_STRING: {
47554754
uint32_t fail;
47564755
DECODE_LABEL(fail, pc)
@@ -4771,58 +4770,67 @@ HOT_FUNC int scheduler_entry_point(GlobalContext *glb)
47714770
avm_int_t bs_offset = term_get_match_state_offset(src);
47724771
term bs_bin = term_get_match_state_binary(src);
47734772

4773+
TRACE("bs_match_string/4, fail=%u src=%p bits=%u offset=%u\n", (unsigned) fail, (void *) src, (unsigned) bits, (unsigned) offset);
4774+
47744775
size_t remaining = 0;
47754776
const uint8_t *str = module_get_str(mod, offset, &remaining);
47764777
if (IS_NULL_PTR(str)) {
47774778
TRACE("bs_match_string: Bad offset in strings table.\n");
47784779
RAISE_ERROR(BADARG_ATOM);
47794780
}
47804781

4781-
TRACE("bs_match_string/4, fail=%u src=%p bits=%u offset=%u\n", (unsigned) fail, (void *) src, (unsigned) bits, (unsigned) offset);
4782-
4783-
if (bits % 8 == 0 && bs_offset % 8 == 0) {
4784-
avm_int_t bytes = bits / 8;
4785-
avm_int_t byte_offset = bs_offset / 8;
4786-
4787-
if (memcmp(term_binary_data(bs_bin) + byte_offset, str, MIN(remaining, (unsigned int) bytes)) != 0) {
4788-
TRACE("bs_match_string: failed to match\n");
4789-
JUMP_TO_ADDRESS(mod->labels[fail]);
4790-
}
4782+
if (term_binary_size(bs_bin) * 8 - bs_offset < MIN(remaining * 8, bits)) {
4783+
TRACE("bs_match_string: failed to match (binary is shorter)\n");
4784+
JUMP_TO_ADDRESS(mod->labels[fail]);
47914785
} else {
4792-
// Compare unaligned bits
4793-
const uint8_t *bs_str = (const uint8_t *) term_binary_data(bs_bin) + (bs_offset / 8);
4794-
uint8_t bin_bit_offset = 7 - (bs_offset - (8 *(bs_offset / 8)));
4795-
uint8_t str_bit_offset = 7;
4796-
size_t remaining_bits = bits;
4797-
while (remaining_bits > 0) {
4798-
uint8_t str_ch = *str;
4799-
uint8_t bin_ch = *bs_str;
4800-
uint8_t str_ch_bit = (str_ch >> str_bit_offset) & 1;
4801-
uint8_t bin_ch_bit = (bin_ch >> bin_bit_offset) & 1;
4802-
if (str_ch_bit ^ bin_ch_bit) {
4786+
if (bits % 8 == 0 && bs_offset % 8 == 0) {
4787+
avm_int_t bytes = bits / 8;
4788+
avm_int_t byte_offset = bs_offset / 8;
4789+
4790+
if (memcmp(term_binary_data(bs_bin) + byte_offset, str, MIN(remaining, (unsigned int) bytes)) != 0) {
48034791
TRACE("bs_match_string: failed to match\n");
48044792
JUMP_TO_ADDRESS(mod->labels[fail]);
4805-
}
4806-
if (str_bit_offset) {
4807-
str_bit_offset--;
48084793
} else {
4809-
str_bit_offset = 7;
4810-
str++;
4794+
term_set_match_state_offset(src, bs_offset + bits);
48114795
}
4812-
if (bin_bit_offset) {
4813-
bin_bit_offset--;
4814-
} else {
4815-
bin_bit_offset = 7;
4816-
bs_str++;
4796+
} else {
4797+
// Compare unaligned bits
4798+
const uint8_t *bs_str = (const uint8_t *) term_binary_data(bs_bin) + (bs_offset / 8);
4799+
uint8_t bin_bit_offset = 7 - (bs_offset - (8 *(bs_offset / 8)));
4800+
uint8_t str_bit_offset = 7;
4801+
size_t remaining_bits = bits;
4802+
while (remaining_bits > 0) {
4803+
uint8_t str_ch = *str;
4804+
uint8_t bin_ch = *bs_str;
4805+
uint8_t str_ch_bit = (str_ch >> str_bit_offset) & 1;
4806+
uint8_t bin_ch_bit = (bin_ch >> bin_bit_offset) & 1;
4807+
if (str_ch_bit ^ bin_ch_bit) {
4808+
TRACE("bs_match_string: failed to match\n");
4809+
JUMP_TO_ADDRESS(mod->labels[fail]);
4810+
break;
4811+
}
4812+
if (str_bit_offset) {
4813+
str_bit_offset--;
4814+
} else {
4815+
str_bit_offset = 7;
4816+
str++;
4817+
}
4818+
if (bin_bit_offset) {
4819+
bin_bit_offset--;
4820+
} else {
4821+
bin_bit_offset = 7;
4822+
bs_str++;
4823+
}
4824+
remaining_bits--;
4825+
}
4826+
if (remaining_bits == 0) {
4827+
term_set_match_state_offset(src, bs_offset + bits);
48174828
}
4818-
remaining_bits--;
48194829
}
48204830
}
4821-
term_set_match_state_offset(src, bs_offset + bits);
48224831
#endif
48234832
break;
48244833
}
4825-
#endif
48264834

48274835
#if MINIMUM_OTP_COMPILER_VERSION <= 21
48284836
case OP_BS_SAVE2: {

‎tests/erlang_tests/test_bs.erl

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020

2121
-module(test_bs).
2222

23-
-export([start/0, id/1]).
23+
-export([start/0, id/1, join/2]).
2424

2525
start() ->
2626
test_pack_small_ints({2, 61, 20}, <<23, 180>>),
@@ -95,6 +95,7 @@ start() ->
9595
ok = test_large(),
9696

9797
ok = test_copy_bits_string(),
98+
ok = test_bs_match_string_select(),
9899

99100
0.
100101

@@ -391,4 +392,42 @@ test_copy_bits_string() ->
391392
<<42, 0, 1, 182>> = Y1,
392393
ok.
393394

395+
% With OTP27, this generates the following code:
396+
% {test,bs_start_match3,{f,197},1,[{x,0}],{x,0}}.
397+
% {bs_match,{f,197},
398+
% {x,0},
399+
% {commands,[{ensure_at_least,16,1},
400+
% {integer,1,{literal,[]},16,1,{x,1}}]}}.
401+
% {bs_get_position,{x,0},{x,2},2}.
402+
% {select_val,{tr,{x,1},{t_integer,{0,65535}}},
403+
% {f,197},
404+
% {list,[{integer,24940},
405+
% {f,196},
406+
% {integer,28271},
407+
% {f,195},
408+
% {integer,28523},
409+
% {f,193}]}}.
410+
% {label,193}.
411+
% {test,bs_match_string,{f,194},[{x,0},104,{string,<<"_simultaneous">>}]}.
412+
% {bs_match,{f,194},{x,0},{commands,[{ensure_exactly,0}]}}.
413+
% {move,{atom,ok},{x,0}}.
414+
% {jump,{f,198}}.
415+
%
416+
% We ensure here (using valgrind) that bs_match_string doesn't try to compare
417+
% beyond heap where <<"okay">> is.
418+
test_bs_match_string_select() ->
419+
R = <<"ok">>,
420+
L = <<"ay">>,
421+
Z =
422+
case join(R, L) of
423+
<<"ok">> -> nok_ok;
424+
<<"ok_simultaneous">> -> nok_simultaneous;
425+
<<"alive">> -> nok_alive;
426+
_Other -> ok
427+
end,
428+
id(Z).
429+
394430
id(X) -> X.
431+
432+
join(X, Y) ->
433+
<<X/binary, Y/binary>>.

0 commit comments

Comments
 (0)