Add support for non-byte-aligned bitstrings

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