Skip to content

Fix term_is_uint32 bigint handling on 32-bit#2380

Open
bettio wants to merge 2 commits into
atomvm:release-0.7from
bettio:fix-term_is_uint32
Open

Fix term_is_uint32 bigint handling on 32-bit#2380
bettio wants to merge 2 commits into
atomvm:release-0.7from
bettio:fix-term_is_uint32

Conversation

@bettio

@bettio bettio commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

On 32-bit builds term_is_uint32 returned true for big integers whose
low 64 bits fit uint32_t, so term_to_uint32 silently truncated them:
erlang:crc32/2, erlang:crc32_combine/3 and crypto:pbkdf2_hmac/5
computed with truncated values instead of raising badarg. 64-bit
builds are unaffected.

Reject boxed integers larger than BOXED_TERMS_REQUIRED_FOR_INT64
terms, so big integers fall through to false, and add big integer
regression cases to the crc32 Erlang test.

Also introduce test-term, a C unit test covering the term.h integer
predicates and conversions against per-case expected flags; it runs
in CI on all platforms, including the emulated 32-bit targets.

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

Comment thread tests/test-term.c Fixed
Comment thread tests/test-term.c Fixed
@bettio bettio changed the title Fix term is uint32 Fix term_is_uint32 bigint handling on 32-bit Jul 24, 2026
bettio added 2 commits July 24, 2026 11:42
term_is_uint32 was returning true on 32-bit builds for big integers
whose low 64 bits fit uint32_t, so term_to_uint32 truncated them.

Signed-off-by: Davide Bettio <davide@uninstall.it>
Check the term_is_* integer predicates and term_to_* conversions
against per-case expected flags, and run the new test-term binary in CI.

Signed-off-by: Davide Bettio <davide@uninstall.it>
@petermm

petermm commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review of e190b09895e78ba916dbfa0cb4c36cbcd9d82e94

Verdict: changes requested

The exact boxed-size check prevents normalized multi-precision integers above UINT32_MAX from
being truncated on 32-bit builds, but it also rejects an in-range value when that value has a padded
bigint representation. AtomVM can produce such terms through normal Erlang bitstring matching, so
the predicate needs to inspect the bigint's magnitude rather than treating every bigint as out of
range.

Findings

[P2] Preserve in-range values stored in a bigint — src/libAtomVM/term.h:1749

term_is_uint32 documents a value-based contract, but the new exact-size condition makes the
answer depend on the integer's storage representation. Wide integer extraction takes every
byte-aligned field over 64 bits through extract_nbits_integer
(src/libAtomVM/opcodesswitch.h:4411-4420). That path uses term_bigint_size_requirements, which
deliberately pads even a one-digit magnitude to more than BOXED_TERMS_REQUIRED_FOR_INT64 terms
(src/libAtomVM/term.h:1631-1645). Consequently, this valid value is rejected:

<<One:72/unsigned-big-integer>> = id(<<0, 0, 0, 0, 0, 0, 0, 0, 1>>),
erlang:crc32(One, <<>>)

On a local native build containing this commit's production change, AtomVM prints One as 1 but
the CRC call raises badarg. On a 32-bit build, the parent implementation read the low 64 bits and
correctly accepted this particular value; the new size gate therefore introduces a regression on
the architecture being fixed. Non-canonical SMALL_BIG_EXT input with leading zero bytes can reach
the same state.

Keep rejecting bigints whose significant magnitude exceeds one 32-bit digit, but accept zero- or
one-digit positive bigints and teach the paired converter how to read that representation. One
small implementation is:

diff --git a/src/libAtomVM/term.h b/src/libAtomVM/term.h
@@
 static inline bool term_is_uint32(term t)
 {
     if (term_is_pos_boxed_integer(t)) {
+        size_t boxed_size = term_boxed_size(t);
+        if (boxed_size == 0) {
+            return true;
-        if (term_boxed_size(t) == BOXED_TERMS_REQUIRED_FOR_INT) {
+        } else if (boxed_size == BOXED_TERMS_REQUIRED_FOR_INT) {
 #if AVM_INT_MAX == INT32_MAX
             return true;
 #elif AVM_INT_MAX == INT64_MAX
             avm_int_t unboxed = term_unbox_int(t);
             return unboxed <= UINT32_MAX;
 #else
 #error "Unsupported AVM_INT_MAX definition"
 #endif

 #if BOXED_TERMS_REQUIRED_FOR_INT != BOXED_TERMS_REQUIRED_FOR_INT64
-        } else if (term_boxed_size(t) == BOXED_TERMS_REQUIRED_FOR_INT64) {
+        } else if (boxed_size == BOXED_TERMS_REQUIRED_FOR_INT64) {
             avm_int64_t unboxed64 = term_unbox_int64(t);
             return unboxed64 <= UINT32_MAX;
 #endif
+        } else if (term_is_bigint(t)) {
+            const intn_digit_t *bigint;
+            size_t bigint_len;
+            intn_integer_sign_t bigint_sign;
+            term_to_bigint(t, &bigint, &bigint_len, &bigint_sign);
+            return bigint_sign == IntNPositiveInteger
+                && intn_count_digits(bigint, bigint_len) <= 1;
         }
@@
     TERM_DEBUG_ASSERT(term_is_uint32(t));
 
     if (term_is_boxed(t)) {
+        size_t boxed_size = term_boxed_size(t);
+        if (boxed_size == 0) {
+            return 0;
+        }
+        if (term_is_bigint(t)) {
+            const intn_digit_t *bigint;
+            size_t bigint_len;
+            intn_integer_sign_t bigint_sign;
+            term_to_bigint(t, &bigint, &bigint_len, &bigint_sign);
+            UNUSED(bigint_len);
+            UNUSED(bigint_sign);
+            return bigint[0];
+        }
 #if BOXED_TERMS_REQUIRED_FOR_INT != BOXED_TERMS_REQUIRED_FOR_INT64
         if (boxed_size == BOXED_TERMS_REQUIRED_FOR_INT) {
 #endif

Add a public-path regression alongside the existing boundary test:

diff --git a/tests/erlang_tests/test_crc32.erl b/tests/erlang_tests/test_crc32.erl
@@
 test_crc32_boundary() ->
     16#FFFFFFFF = erlang:crc32(?MODULE:id(16#FFFFFFFF), ?MODULE:id(<<>>)),
+    <<WideZero:72/unsigned-big-integer>> =
+        ?MODULE:id(<<0, 0, 0, 0, 0, 0, 0, 0, 0>>),
+    <<WideOne:72/unsigned-big-integer>> =
+        ?MODULE:id(<<0, 0, 0, 0, 0, 0, 0, 0, 1>>),
+    0 = erlang:crc32(?MODULE:id(WideZero), ?MODULE:id(<<>>)),
+    1 = erlang:crc32(?MODULE:id(WideOne), ?MODULE:id(<<>>)),
     ok.

Canonicalizing all wide-bitstring and external-term producers would also solve the representation
problem, but that is a broader change than making this predicate and converter honor their stated
numeric contract.

Non-blocking suggestion

Exercise the claimed PBKDF2 fix through its public API

The changelog specifically names crypto:pbkdf2_hmac/5, but this commit adds regression coverage
only through CRC callers. The existing test_bad_iter/0 ignores its try expression's result and
returns ok unconditionally, so it passes even when the crypto call unexpectedly succeeds. On the
old 32-bit implementation, (1 bsl 64) + 1 truncates to one iteration and is cheap enough to expose
the bug safely.

diff --git a/tests/erlang_tests/test_crypto_pbkdf2_hmac.erl b/tests/erlang_tests/test_crypto_pbkdf2_hmac.erl
@@
 test_bad_iter() ->
-    try
-        crypto:pbkdf2_hmac(sha256, <<"password">>, <<"salt">>, 0, 20)
-    catch
-        error:_ ->
-            exp_err
-    end,
+    ok = expect_error(fun() ->
+        crypto:pbkdf2_hmac(sha256, <<"password">>, <<"salt">>, 0, 20)
+    end),
+    ok = expect_error(fun() ->
+        crypto:pbkdf2_hmac(
+            sha256, <<"password">>, <<"salt">>, (1 bsl 64) + 1, 20
+        )
+    end),
     ok.
+
+expect_error(F) ->
+    try F() of
+        _ -> unexpected_return
+    catch
+        error:_ -> ok
+    end.

Verification performed

  • Inspected the commit against parent c4afa59b5 and traced all term_is_uint32 callers.
  • Traced bigint creation through wide bitstring extraction and external-term decoding.
  • Reproduced the padded-bigint rejection with the native build/src/AtomVM executable.
  • Ran git diff --check e190b098^ e190b098 successfully.
  • Consulted Oracle for an independent correctness review and incorporated its confirmed findings.

The affected 32-bit test suite was not executed locally; the 32-bit regression follows from the
platform-specific preprocessor branch and the shared bigint layout code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants