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