Skip to content

Fix offset and mod_offset sign using size_t#2266

Merged
bettio merged 1 commit intoatomvm:release-0.7from
pguyot:w15/fix-offset-sign
Apr 12, 2026
Merged

Fix offset and mod_offset sign using size_t#2266
bettio merged 1 commit intoatomvm:release-0.7from
pguyot:w15/fix-offset-sign

Conversation

@pguyot
Copy link
Copy Markdown
Collaborator

@pguyot pguyot commented Apr 11, 2026

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
Copy link
Copy Markdown
Contributor

petermm commented Apr 11, 2026

Amp: pick and choose:

PR Review: Fix offset and mod_offset sign using size_t

Commit: 47effcacFix offset and mod_offset sign using size_t

Verdict

Mostly correct. The size_t migration is sound, (size_t)-1 sentinel is safe, and *l_off = 0 is semantically equivalent (only reached when new_label_offset == mod_offset). However, the migration is incomplete and introduces a few minor issues.


Findings

1. Use %zu instead of %u cast for size_t (narrowing on LP64)

File: src/libAtomVM/context.c

The (unsigned) cast silently truncates on 64-bit platforms. Use the proper format specifier:

-        fprintf(stderr, "cp: #CP<module: %i, label: %i, offset: %u>\n\n",
-            cp_mod->module_index, label, (unsigned) offset);
+        fprintf(stderr, "cp: #CP<module: %i, label: %i, offset: %zu>\n\n",
+            cp_mod->module_index, label, offset);
-            fprintf(stderr, "#CP<module: %i, label: %i, offset: %u>\n", cp_mod->module_index, label, (unsigned) offset);
+            fprintf(stderr, "#CP<module: %i, label: %i, offset: %zu>\n", cp_mod->module_index, label, offset);

2. Use (size_t) cast instead of (unsigned) in comparison

File: src/libAtomVM/module.c

The (unsigned) cast narrows pointer arithmetic on LP64. Should match the LHS type:

-        while (mod_offset > (unsigned) (l - code)) {
+        while (mod_offset > (size_t) (l - code)) {

3. Incomplete migration — stacktrace_create_raw signatures still use int

File: src/libAtomVM/stacktrace.h / stacktrace.c

current_offset parameter is still int in stacktrace_create_raw and stacktrace_create_raw_mfa. These should be size_t for consistency with the rest of the change.

4. Latent null-deref in EMU path of module_cp_to_label_offset

File: src/libAtomVM/module.c (EMU path, around the label search loop)

When label == NULL but l_off != NULL, the code dereferences *label:

if (l_off) {
    *l_off = mod_offset - (mod->labels[*label] - code);  // *label is NULL deref!
}

Fix by using a local variable:

+        int resolved_label = i - 1;
         if (label) {
-            *label = i - 1;
+            *label = resolved_label;
         }
         if (l_off) {
-            *l_off = mod_offset - (mod->labels[*label] - code);
+            *l_off = mod_offset - (mod->labels[resolved_label] - code);
         }

5. Documentation mismatch for l_off parameter

File: src/libAtomVM/module.h

The header comment says l_off is "offset of label from module start" but the implementation returns the offset from the resolved label to the CP (i.e., mod_offset - label_offset). The doc should be corrected:

- * @param l_off if not null, set to offset of label from module start
+ * @param l_off if not null, set to offset of cp from the resolved label

Not a bug

  • *l_off = 0 in JIT path — This replaces mod_offset - new_label_offset, but that branch is only entered when new_label_offset == mod_offset, so the result is always 0. Semantically identical.
  • (size_t)-1 sentinelSIZE_MAX cannot collide with any real offset (CP encoding is 24-bit). Safe to use as "no previous offset" marker.

Out of scope (future consideration)

  • Several backing storage types remain narrower than size_t (unsigned int in LineRefOffset, line_refs_offsets, module_insert_line_ref_offset). This is fine since CP encoding is limited to 24 bits, but should be documented or made consistently uint32_t if a broader cleanup is desired.

Signed-off-by: Paul Guyot <pguyot@kallisys.net>
@pguyot pguyot force-pushed the w15/fix-offset-sign branch from 47effca to 8264338 Compare April 11, 2026 07:59
@pguyot
Copy link
Copy Markdown
Collaborator Author

pguyot commented Apr 11, 2026

Thanks. Cast to unsigned was on purpose. I'm starting to add comments for LLM-based review tools like we do alter code to circumvent false positive warnings of compilers.

@bettio bettio merged commit 0299f17 into atomvm:release-0.7 Apr 12, 2026
311 of 318 checks passed
@pguyot pguyot deleted the w15/fix-offset-sign branch April 12, 2026 16:03
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