Merge pull request atomvm#1615 from pguyot/w14/fix-op_bs_match_string

Fix several issues with OP_BS_MATCH_STRING

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

Author: bettio

src/libAtomVM/opcodesswitch.h

@@ -4750,7 +4750,6 @@ HOT_FUNC int scheduler_entry_point(GlobalContext *glb)
             }
 #endif
 
-#if MINIMUM_OTP_COMPILER_VERSION <= 25
             case OP_BS_MATCH_STRING: {
                 uint32_t fail;
                 DECODE_LABEL(fail, pc)
@@ -4771,58 +4770,67 @@ HOT_FUNC int scheduler_entry_point(GlobalContext *glb)
                     avm_int_t bs_offset = term_get_match_state_offset(src);
                     term bs_bin = term_get_match_state_binary(src);
 
+                    TRACE("bs_match_string/4, fail=%u src=%p bits=%u offset=%u\n", (unsigned) fail, (void *) src, (unsigned) bits, (unsigned) offset);
+
                     size_t remaining = 0;
                     const uint8_t *str = module_get_str(mod, offset, &remaining);
                     if (IS_NULL_PTR(str)) {
                         TRACE("bs_match_string: Bad offset in strings table.\n");
                         RAISE_ERROR(BADARG_ATOM);
                     }
 
-                    TRACE("bs_match_string/4, fail=%u src=%p bits=%u offset=%u\n", (unsigned) fail, (void *) src, (unsigned) bits, (unsigned) offset);
-
-                    if (bits % 8 == 0 && bs_offset % 8 == 0) {
-                        avm_int_t bytes = bits / 8;
-                        avm_int_t byte_offset = bs_offset / 8;
-
-                        if (memcmp(term_binary_data(bs_bin) + byte_offset, str, MIN(remaining, (unsigned int) bytes)) != 0) {
-                            TRACE("bs_match_string: failed to match\n");
-                            JUMP_TO_ADDRESS(mod->labels[fail]);
-                        }
+                    if (term_binary_size(bs_bin) * 8 - bs_offset < MIN(remaining * 8, bits)) {
+                        TRACE("bs_match_string: failed to match (binary is shorter)\n");
+                        JUMP_TO_ADDRESS(mod->labels[fail]);
                     } else {
-                        // Compare unaligned bits
-                        const uint8_t *bs_str = (const uint8_t *) term_binary_data(bs_bin) + (bs_offset / 8);
-                        uint8_t bin_bit_offset = 7 - (bs_offset - (8 *(bs_offset / 8)));
-                        uint8_t str_bit_offset = 7;
-                        size_t remaining_bits = bits;
-                        while (remaining_bits > 0) {
-                            uint8_t str_ch = *str;
-                            uint8_t bin_ch = *bs_str;
-                            uint8_t str_ch_bit = (str_ch >> str_bit_offset) & 1;
-                            uint8_t bin_ch_bit = (bin_ch >> bin_bit_offset) & 1;
-                            if (str_ch_bit ^ bin_ch_bit) {
+                        if (bits % 8 == 0 && bs_offset % 8 == 0) {
+                            avm_int_t bytes = bits / 8;
+                            avm_int_t byte_offset = bs_offset / 8;
+
+                            if (memcmp(term_binary_data(bs_bin) + byte_offset, str, MIN(remaining, (unsigned int) bytes)) != 0) {
                                 TRACE("bs_match_string: failed to match\n");
                                 JUMP_TO_ADDRESS(mod->labels[fail]);
-                            }
-                            if (str_bit_offset) {
-                                str_bit_offset--;
                             } else {
-                                str_bit_offset = 7;
-                                str++;
+                                term_set_match_state_offset(src, bs_offset + bits);
                             }
-                            if (bin_bit_offset) {
-                                bin_bit_offset--;
-                            } else {
-                                bin_bit_offset = 7;
-                                bs_str++;
+                        } else {
+                            // Compare unaligned bits
+                            const uint8_t *bs_str = (const uint8_t *) term_binary_data(bs_bin) + (bs_offset / 8);
+                            uint8_t bin_bit_offset = 7 - (bs_offset - (8 *(bs_offset / 8)));
+                            uint8_t str_bit_offset = 7;
+                            size_t remaining_bits = bits;
+                            while (remaining_bits > 0) {
+                                uint8_t str_ch = *str;
+                                uint8_t bin_ch = *bs_str;
+                                uint8_t str_ch_bit = (str_ch >> str_bit_offset) & 1;
+                                uint8_t bin_ch_bit = (bin_ch >> bin_bit_offset) & 1;
+                                if (str_ch_bit ^ bin_ch_bit) {
+                                    TRACE("bs_match_string: failed to match\n");
+                                    JUMP_TO_ADDRESS(mod->labels[fail]);
+                                    break;
+                                }
+                                if (str_bit_offset) {
+                                    str_bit_offset--;
+                                } else {
+                                    str_bit_offset = 7;
+                                    str++;
+                                }
+                                if (bin_bit_offset) {
+                                    bin_bit_offset--;
+                                } else {
+                                    bin_bit_offset = 7;
+                                    bs_str++;
+                                }
+                                remaining_bits--;
+                            }
+                            if (remaining_bits == 0) {
+                                term_set_match_state_offset(src, bs_offset + bits);
                             }
-                            remaining_bits--;
                         }
                     }
-                    term_set_match_state_offset(src, bs_offset + bits);
                 #endif
                 break;
             }
-#endif
 
 #if MINIMUM_OTP_COMPILER_VERSION <= 21
             case OP_BS_SAVE2: {

tests/erlang_tests/test_bs.erl

@@ -20,7 +20,7 @@
 
 -module(test_bs).
 
--export([start/0, id/1]).
+-export([start/0, id/1, join/2]).
 
 start() ->
     test_pack_small_ints({2, 61, 20}, <<23, 180>>),
@@ -95,6 +95,7 @@ start() ->
     ok = test_large(),
 
     ok = test_copy_bits_string(),
+    ok = test_bs_match_string_select(),
 
     0.
 
@@ -391,4 +392,42 @@ test_copy_bits_string() ->
     <<42, 0, 1, 182>> = Y1,
     ok.
 
+% With OTP27, this generates the following code:
+%    {test,bs_start_match3,{f,197},1,[{x,0}],{x,0}}.
+%    {bs_match,{f,197},
+%              {x,0},
+%              {commands,[{ensure_at_least,16,1},
+%                         {integer,1,{literal,[]},16,1,{x,1}}]}}.
+%    {bs_get_position,{x,0},{x,2},2}.
+%    {select_val,{tr,{x,1},{t_integer,{0,65535}}},
+%                {f,197},
+%                {list,[{integer,24940},
+%                       {f,196},
+%                       {integer,28271},
+%                       {f,195},
+%                       {integer,28523},
+%                       {f,193}]}}.
+%  {label,193}.
+%    {test,bs_match_string,{f,194},[{x,0},104,{string,<<"_simultaneous">>}]}.
+%    {bs_match,{f,194},{x,0},{commands,[{ensure_exactly,0}]}}.
+%    {move,{atom,ok},{x,0}}.
+%    {jump,{f,198}}.
+%
+% We ensure here (using valgrind) that bs_match_string doesn't try to compare
+% beyond heap where <<"okay">> is.
+test_bs_match_string_select() ->
+    R = <<"ok">>,
+    L = <<"ay">>,
+    Z =
+        case join(R, L) of
+            <<"ok">> -> nok_ok;
+            <<"ok_simultaneous">> -> nok_simultaneous;
+            <<"alive">> -> nok_alive;
+            _Other -> ok
+        end,
+    id(Z).
+
 id(X) -> X.
+
+join(X, Y) ->
+    <<X/binary, Y/binary>>.