[pguyot]

Continuation of:

• Reject negative dynamic segment sizes in bit syntax matching #2377

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

[petermm]

AMP, pick and choose:

Review of 5742ddaaf — Add support for non-byte-aligned bitstrings

Verdict: changes requested

The implementation is broad and well tested for normal behavior, but it introduces three high-confidence memory-safety/correctness regressions and one 32-bit representation regression. The allocation failures are especially important because reserving process-heap words does not guarantee that the separate refc-binary allocation will succeed.

Oracle was asked to independently review the commit, with emphasis on term representation, GC/rooting, overflow, interpreter/JIT parity, and external-term handling. The findings below were then checked against the affected code and callers.

Findings

1. Critical (32-bit): list_to_bitstring/1 can wrap its size and overwrite the heap

Location: src/libAtomVM/nifs.c:7561-7591, src/libAtomVM/nifs.c:7620-7630

fold_bitstring_list accumulates the result size in a size_t without checking either bits + 8 or bits + term_bit_size(t). On a 32-bit target, a list containing eight references to the same 64 MiB binary has an intended size of exactly 2^32 bits. The first pass wraps total_bits to zero and allocates a zero-byte binary. The second pass then copies each 64 MiB element into that undersized allocation, causing a large out-of-bounds write.

The newly allocated binary is also used without checking term_create_empty_binary for refc allocation failure.

Suggested fix: reject accumulation overflow before copying, and check the binary allocation before obtaining its data pointer. InteropMemoryAllocFail gives the existing out_of_memory behavior; a dedicated result mapped to system_limit would more closely match OTP for an unrepresentable size.

diff --git a/src/libAtomVM/nifs.c b/src/libAtomVM/nifs.c
@@
         if (term_is_integer(t)) {
             avm_int_t value = term_to_int(t);
             if (UNLIKELY(value < 0 || value > UCHAR_MAX)) {
                 temp_stack_destroy(&temp_stack);
                 return InteropBadArg;
             }
+            if (UNLIKELY(bits > SIZE_MAX - 8)) {
+                temp_stack_destroy(&temp_stack);
+                return InteropMemoryAllocFail;
+            }
             if (dst) {
                 uint8_t byte = (uint8_t) value;
                 bitstring_copy_bits(dst, bits, &byte, 8);
             }
             bits += 8;
             t = temp_stack_pop(&temp_stack);
         } else if (term_is_bitstring(t)) {
             size_t n = term_bit_size(t);
+            if (UNLIKELY(n > SIZE_MAX - bits)) {
+                temp_stack_destroy(&temp_stack);
+                return InteropMemoryAllocFail;
+            }
             if (dst && n != 0) {
                 bitstring_copy_bits(dst, bits, (const uint8_t *) term_binary_data(t), n);
             }
@@
     term bin = term_create_empty_binary(alloc_bytes, &ctx->heap, ctx->global);
+    if (UNLIKELY(term_is_invalid_term(bin))) {
+        RAISE_ERROR(OUT_OF_MEMORY_ATOM);
+    }
     if (total_bits != 0) {

2. High: unaligned slice allocation failure is dereferenced as a term

Location: src/libAtomVM/bitstring.c:417-431

Affected interpreter callers: src/libAtomVM/opcodesswitch.h:4220-4229, :4611-4622, :6286-6298, and :6310-6322

Affected JIT callers: libs/jit/src/jit.erl:1447-1466, :3225-3243, and :3264-3278

An unaligned slice is materialized with term_create_empty_binary. For a result large enough to use a refc binary, that function can return term_invalid_term() even after memory_ensure_free, because the refc data buffer is allocated separately. bitstring_slice immediately passes the invalid term to term_binary_data, producing an invalid pointer dereference. The callers must also convert an invalid result into out_of_memory rather than storing it in a VM register.

Core fix:

diff --git a/src/libAtomVM/bitstring.c b/src/libAtomVM/bitstring.c
@@
     size_t result_bytes = (len_bits + 7) / 8;
     // term_create_empty_binary zero-fills, as bitstring_copy_bits_from requires
     term bin = term_create_empty_binary(result_bytes, heap, glb);
+    if (UNLIKELY(term_is_invalid_term(bin))) {
+        return bin;
+    }
     uint8_t *dst = (uint8_t *) term_binary_data(bin);

Every interpreter caller then needs the equivalent of:

@@
     term t = bitstring_slice(bs_bin, bs_offset, size_bits, &ctx->heap, ctx->global);
+    if (UNLIKELY(term_is_invalid_term(t))) {
+        RAISE_ERROR(OUT_OF_MEMORY_ATOM);
+    }
     WRITE_REGISTER(dreg, t);

The JIT paths need the same branch after PRIM_BITSTRING_SLICE and PRIM_BITSTRING_CREATE_TAIL, calling PRIM_RAISE_ERROR with OUT_OF_MEMORY_ATOM.

3. High: JIT private_append/all drops a partial source when the final result is byte-aligned

Location: libs/jit/src/jit.erl:2382-2407, :2948-2977

Interpreter comparison: src/libAtomVM/opcodesswitch.h:5749-5765

The JIT rejects reuse only when the total output is not byte-aligned. In the special first-segment private_append/all path it obtains the accumulator length with term_binary_size, which excludes trailing bits, and term_reuse_binary likewise copies only whole bytes in its fallback path. For an accumulator such as <<1:1>> followed by seven bits, the total is byte-aligned and passes the check, but the source bit contributes an original size of zero and is silently omitted. The interpreter explicitly rejects a non-byte-aligned source in this reuse path.

For example, if emitted with first-segment private_append, this must not produce <<0>>:

Src = <<1:1>>,
<<Src/bitstring, 0:7>>.

Suggested fix: test the source's exact bit size before reusing it; reject a partial source with unsupported to match the current interpreter behavior. Only convert to bytes after that check, and advance the construction offset by the exact source bit count.

diff --git a/libs/jit/src/jit.erl b/libs/jit/src/jit.erl
@@ first_pass_bs_create_bin_insert_value(private_append, ...)
-    % Get original size before reusing
-    {MSt1, OriginalSize} = term_binary_size(Src, MMod, MSt0),
+    % Reuse currently supports only a byte-aligned accumulator, as in the interpreter.
+    {MSt1, SrcReg} = MMod:copy_to_native_register(MSt0, Src),
+    {MSt2, OriginalBits} = term_bit_size({free, SrcReg}, MMod, MSt1),
+    MSt3 = MMod:if_block(MSt2, {OriginalBits, '&', 16#7, '!=', 0}, fun(BSt0) ->
+        MMod:call_primitive_last(BSt0, ?PRIM_RAISE_ERROR, [
+            ctx, jit_state, offset, ?UNSUPPORTED_ATOM
+        ])
+    end),
+    {MSt4, OriginalSize} = MMod:shift_right(MSt3, OriginalBits, 3),
     % Reuse the source binary (content is already there, no need to copy)
-    {MSt2, CreatedBin} = MMod:call_primitive(MSt1, ?PRIM_TERM_REUSE_BINARY, [
+    {MSt5, CreatedBin} = MMod:call_primitive(MSt4, ?PRIM_TERM_REUSE_BINARY, [
         ctx, {free, Src}, {free, BinaryTotalSizeInBytes}
     ]),

The register-state numbering/ownership in the remainder of the clause must be adjusted accordingly; the important behavior is to validate OriginalBits, use OriginalSize only for byte allocation/reuse, and use OriginalBits for the next construction offset.

4. Medium (32-bit): packing trailing bits into the offset truncates valid sub-binary offsets

Location: src/libAtomVM/term.c:1361-1369, src/libAtomVM/term.h:2057-2059, :3102-3111

The previous representation stored the full byte offset in boxed[2]. The new representation stores (offset << 3) | trailing_bits. On a 32-bit build, offsets above SIZE_MAX >> 3 (approximately 512 MiB) lose their upper three bits. term_binary_data later decodes the truncated value and points into unrelated earlier data. The related term_bit_size(t) = term_binary_size(t) * 8 + trailing calculation also wraps for binaries beyond the same boundary.

This affects ordinary byte-aligned sub-binaries too, so it narrows an existing representable range rather than only limiting the new feature.

The robust fix is to increase TERM_BOXED_SUB_BINARY_SIZE and store trailing bits in a separate word, updating the GC/JIT layout constants and accesses. If retaining the packed representation, every constructor and bit-size operation must instead reject values above SIZE_MAX >> 3 with system_limit; masking or casting cannot recover the discarded bits.

Verification performed

• git diff --check HEAD^ HEAD — passed.
• Reconfigured and rebuilt test-bitstring from the reviewed commit — passed.
• Ran build/tests/test-bitstring — passed.

The existing unit test exercises normal bit-copy boundaries and printing, but it does not cover refc allocation failure, 32-bit size overflow, or the modern JIT private_append/all parity case above.

Recommended regression coverage

1. Add interpreter/JIT parity coverage for a partial first private_append/all accumulator followed by enough bits to make the final output byte-aligned.
2. Add refc-allocation fault injection for unaligned bs_get_binary, bs_get_tail, and list_to_bitstring.
3. Add 32-bit boundary tests for the list sizing fold and sub-binary offset representation without requiring a real multi-gigabyte allocation (for example, by extracting and testing the checked arithmetic/layout helper).

[bettio]

First review round. I didn't finish review yet but I start reporting the issues.

Found the following issues outside the changed files:

• io_lib cannot format bitstrings. io_lib:format("~p", [<<1:1>>]). Same applies for `w.

• binary:decode_hex/1 returns silently wrong data for bitstrings; encode_hex accepts them.

binary:decode_hex(<<"AB", 1:9>>)  %% here: <<171>>      OTP: error                                
binary:encode_hex(<<1:4>>)        %% here: <<"1">>      OTP: badarg                               

• tests/erlang_tests/test_is_bitstring_is_binary.erl: I think we should update this file:

 34     % bitstrings are currently unsupported                                                          
 35     %   true = is_bitstring(id(<<1:1>>)),                                                           

 44     % bitstrings are currently unsupported                                                          
 45     %   false = is_binary(id(<<1:1>>)), 

[pguyot]

I think all comments addressed so far, plus some more things I noticed reviewing again on my end (e.g. difference with beam doc).

[bettio]

We are getting close, there are still a few remaining issues.

Also let's add more tests (following are all falining):

C = ?MODULE:id($a),
F = ?MODULE:id(1.0),
<<176, 1:1>> = <<1:1, C/utf8>>,
<<22, 18>> = <<1:4, C/utf8, 2:4>>,
<<128, 48, 1:1>> = <<1:1, C/utf16>>,
<<128, 0, 0, 48, 1:1>> = <<1:1, C/utf32>>,
{97, <<>>} = (fun(<<_:1, X/utf8, R/bits>>) -> {X, R} end)(id(<<1:1, $a/utf8>>)),
{1, 97, 2} = (fun(<<A:4, X/utf8, B:4>>) -> {A, X, B} end)(id(<<1:4, $a/utf8, 2:4>>)),
97 = (fun(<<_:1, X/utf16>>) -> X end)(id(<<1:1, $a/utf16>>)),
97 = (fun(<<_:1, X/utf32>>) -> X end)(id(<<1:1, $a/utf32>>)),
<<159, 192, 0, 0, 0:1>> = <<1:1, F:32/float>>,
<<19, 248, 0, 0, 2>> = <<1:4, F:32/float, 2:4>>,
1.0 = (fun(<<_:1, V:32/float>>) -> V end)(id(<<1:1, 1.0:32/float>>)).

[bettio]

writes at offset >> 3 and drops the bit offset (same at :423 utf16, :457 utf32).

With unaligned offsets now allowed in bs_create_bin this silently clobbers the preceding bits: C = id($a), <<1:1, C/utf8>> gives <<97,0:1>>, BEAM <<176,1:1>>.

Idea: encode into a 4-byte stack buffer and bitstring_copy_bits(dst, offset, buf, size * 8); both interpreter and JIT go through these helpers.

[bettio]

decodes from offset >> 3: <<:1, X/utf8>> = id(<<1:1, $a/utf8>>) is a badmatch (BEAM: 97) and <<:1, X/utf16>> = id(<<1:1, $a/utf16>>) returns 32816.

Idea: when offset & 7, copy up to 4 bytes (bounded by the remaining bits) with bitstring_copy_bits_from into a zeroed temp buffer and decode from that.

See also line 438 and line 471.

[bettio]

A float segment at an unaligned offset ends in badarg here (and at line 3812), but F = id(1.0), <<1:1, F:32/float>> is valid and BEAM builds <<159,192,0,0,0:1>>.

This issue was already existing, but now reachable for every bitstring.

idea: IEEE bits are an integer, so bitstring_insert_any_integer already does the unaligned write for 16/32/64 bits in both endiannesses.

Let's address this on an additional commit since it is not an issue introduced from this PR.

[bettio]

Same for extraction: bitstring_extract_f16/f32/f64 return false at an unaligned offset, so case id(<<1:1, 1.0:32/float>>) of <<_:1, F:32/float>> -> F; _ -> nomatch end silently returns nomatch (BEAM: 1.0).

This PR changed the integer case at line 4501 to raise unsupported instead of silently failing; floats should follow, or extract the bits with bitstring_extract_any_integer and reinterpret.

[bettio]

Returning false ends in badarg, while the interpreter raises unsupported for the same unaligned >64-bit segment.

verified on the x86_64 JIT build with N = id(1 bsl 70), <<1:1, N:71>>.

Also tighten the unsupported | badarg acceptance in test_no_bs_create_bin.erl.

[bettio]

I would rather suggest using size_t here in this function.

[bettio]

Bit-by-bit loop: <<_:1, Rest/bits>> on a 1 MiB binary runs 8M iterations. A byte loop (src[i] << s) | (src[i + 1] >> (8 - s)) is about 8x faster; the unaligned branch of bitstring_copy_bits_incomplete_bytes (line 352) has the same shape.

[bettio]

This copies whenever len_bits is not a multiple of 8, even when offset is byte-aligned, so a bitstring with trailing bits can never share storage: every <<_:8, Rest/bits>> or sized slice materializes the whole remainder, bit by bit.

Since a peer can turn any payload into a bitstring (BIT_BINARY_EXT instead of BINARY_EXT, or one extra bit on the wire), linear parsers become quadratic on attacker-controlled input. When offset % 8 == 0, share the source as split_binary/2 does at nifs.c:3773, under the same refc/SUB_BINARY_MIN policy as term_maybe_create_sub_binary.

[bettio]

Also one more issue in opcodesswitch.h: a value in [2^63, 2^64) is a bignum here, so it takes the term_to_bigint path below and a non-byte-aligned segment ends in unsupported at line 5972 (badarg on the JIT, same dispatch at jit.c:1700).

That makes <<Flag:1, Hash:64>> fail for half of all 64-bit hashes: H = id(16#9E3779B97F4A7C15), <<1:1, H:64>> raises on both runtimes while BEAM builds <<207,27,188,220,191,165,62,10,1:1>>; matching the same field back (<<_:1, X:64>>) already works.

differences-with-beam.md:167 promises that integers fitting in 64 bits have no restriction.

Idea when big_sign is positive, intn_is_uint64(big_src_value, big_len) holds and the field is at most 64 bits, call bitstring_insert_integer with (avm_int64_t) intn_to_uint64(big_src_value); the low 64 bits of the two's-complement pattern are exactly the field contents, and the 64-bit limit keeps the sign-fill of wider fields (which assumes a signed value) out of the picture. Mirror it in jit_bitstring_insert_integer.

[bettio]

Found a couple of issues with older opcodes (so basically they are hard to reach). If it doesn't take too much time, do you mind fixing them?
Otherwise I can accept this PR and take care of this issue later.

[bettio]

This line must change because VERIFY_IS_BITSTRING now lets a bitstring source through, but the all branch sets size_bits = src_bits without the src_bits % unit check that this PR added to bs_append and bs_private_append.

For a /binary segment BEAM raises badarg; AtomVM now silently builds a result.

Possible fix:

} else if (size == ALL_ATOM) {
    if (UNLIKELY(unit == 0 || (src_bits % unit) != 0)) {
        RAISE_ERROR(BADARG_ATOM);
    }
    size_bits = src_bits;

with +no_bs_create_bin +no_bs_match +no_type_opt:

f(B) -> <<1:1, B/binary>>.         % f(<<1:1>>)              OTP 27: badarg   AtomVM: <<192,0>>
h(A, B) -> <<A/bitstring, B/binary>>.  % h(<<5:3>>, <<7:3>>)  OTP 27: badarg   AtomVM: <<188,0:3>>

[bettio]

This check was the only thing stopping the size that bs_add computes a few lines above: OP_BS_ADD returns (src1 + src2) * unit, while BEAM defines bs_add as src1 + src2 * unit. With the alignment check gone, the legacy opcodes now allocate that wrong total and silently build wrong-size binaries.

%% compiled with OTP 27 +no_bs_create_bin
f(B) -> <<1:1, B/binary>>.   % f(<<1,2>>)   OTP 27: <<128,129,0:1>>   AtomVM: <<128,129,0>>

The compiler emits bs_add(0, 1, 1) then bs_add(1, byte_size(B), 8); AtomVM computes 24 bits instead of 17 and returns <<128,129,0>>, OTP 27 returns <<128,129,0:1>>. Possible fix at 3425:

WRITE_REGISTER(dreg, term_from_int(src1_val + src2_val * unit));

[bettio]

Agree with keeping results out of a8. One thing I noticed, in case it's not expected: with results now in a9–a15, set_registers_args0 can run out of parking registers and fail with {badmatch,[]} at line 2113. Smallest example:

f(_, C) -> <<C/utf16-little, C/utf32-little>>.

[bettio]

Checked against the JIT compiler at the base commit (18b3587): it compiles this, the PR does not, so it is a regression of this commit rather than a pre-existing limit.

Trigger is two little-endian utf segments in one bs_create_bin:
n(C) -> <<C/utf16-little, C/utf32-little>>. % base: ok, PR: {badmatch,[]}
whereas <<A/utf8, C/utf16-little>>, <<V:N, C/utf32-little>>, <<A/utf8, C/utf16>>, <<A/utf16, C/utf32>>, <<A/utf16-little, C:8>> and <<A:8, C/utf32-little>> compile on both.

Frames: set_registers_args0 (jit_xtensa.erl:2113) <- set_registers_args (:2008) <- call_func_ptr (:1906) <- the utf32 insert clause (jit.erl:2836). With the utf16 result now parked in a9-a15 instead of a8, AvailableScratchGP is empty when the utf32 call has to move an argument out of a parameter register, and [Avail | AvailGPT] = AvailGP fails. Either give set_registers_args0 a fallback when AvailGP is empty (spill, or a8 when the function pointer is not there) or keep this change but handle that case before merging, since an ESP32 module with that construct no longer JIT-compiles.

[bettio]

Not a correctness problem, lookups still work because the key comparison runs after the hash.
hash_binary folds term_binary_size(t) whole bytes only, so <<0:1>>, <<1:1>>, <<7:3>> and any
two bitstrings that differ only in their partial byte land in the same bucket. Since this hunk
is what makes bitstrings hashable, consider folding term_bit_size(t) and the masked last byte
in as well, here or in a follow-up commit.

Also a TODO for the future might be a good option too.

[bettio]

Actually, offset 0 fails, every byte-aligned offset fails the same way, and when a segment follows, the failure turns into silently wrong bytes, which is the #2443 behaviour. Non-byte-aligned offsets work.

We might fix it, see my comment in opcodesswitch.h:5981

[bettio]

This condition sends a bignum through bitstring_bigint_to_field64 only when the offset or the
size is not a multiple of 8.

A byte-aligned field still goes to intn_to_integer_bytes with avail, so <<H:32>> and <<7:8, H:32>> raise badarg and <<7:8, H:32, 0:32>> returns wrong bytes, for H = id(16764730074108805316), while <<1:1, H:32>> is correct.

This is the #2443 behaviour, and I think the same issue exists at line 3681 and jit.c:1713.

[bettio]

About #2444: fixing it in this PR looks like the easiest option, since it touches the same code. Otherwise postponing it is fine, I left the issue open on purpose as a reminder.

[bettio]

It seems that all 32-bit builds are failing on CI:

test_bs_bigint_unaligned:
CRASH 
======
pid: <0.1.0>

Stacktrace:
[{test_bs_bigint_unaligned,bi_sweep_values,3,[{file,"/home/runner/work/AtomVM/AtomVM/tests/erlang_tests/test_bs_bigint_common.hrl"},{line,406}]},{test_bs_bigint_unaligned,bi_sweep_sizes,2,[{file,"/home/runner/work/AtomVM/AtomVM/tests/erlang_tests/test_bs_bigint_common.hrl"},{line,392}]},{test_bs_bigint_unaligned,bi_sweep_prefixes,1,[{file,"/home/runner/work/AtomVM/AtomVM/tests/erlang_tests/test_bs_bigint_common.hrl"},{line,386}]},{test_bs_bigint_unaligned,bigint_test,0,[{file,"/home/runner/work/AtomVM/AtomVM/tests/erlang_tests/test_bs_bigint_common.hrl"},{line,31}]},{test_bs_bigint_unaligned,start,0,[{file,"/home/runner/work/AtomVM/AtomVM/tests/erlang_tests/test_bs_bigint_unaligned.erl"},{line,29}]}]

cp: #CP<module: 0, label: 42, offset: 2048>

x[0]: error
x[1]: {badmatch,<<25:5>>}
x[2]: {5,5,155,2,[{0,43490},{0,1532},{0,13536},{0,13932},{0,17538}],error}

Stack 
-----

[]
[]
<<9:5>>
<<9:5>>
0
-9223372036854775809
[18446744073709551616,1180591620717411315769,-1180591620717411315769,1606938044258990275541962092341162602522202993782792835301383,-1606938044258990275541962092341162602522202993782792835301385,57896044618658097711785492504343953926634992332820282019728792003956564819968,-57896044618658097711785492504343953926634992332820282019728792003956564819968,115792089237316195423570985008687907853269984665640564039457584007913129639935,-115792089237316195423570985008687907853269984665640564039457584007913129639935]
2
0
#CP<module: 0, label: 39, offset: 264>
[3,7,8,9,15,16,17,31,32,33,63,64,65,66,71,72,73,100,127,128,129,200,255]
2
0
#CP<module: 0, label: 36, offset: 258>
[1,3,4,7,8,9,15]
#CP<module: 0, label: 4, offset: 362>
#CP<module: 0, label: 125, offset: 100>
#CP<module: 0, label: 0, offset: 1056>


Mailbox
-------


Monitors
--------


**End Of Crash Report**
Expected 0 but result is not an integer
test_bs_bigint_unaligned:FAILED