Skip to content

[1c] Fix native code bugs in ARimageFsetDisplay.cpp (NULL deref, exit(), memory leaks, pointer truncation) #19

Description

@kalwalt

Summary

Fix multiple native-code bugs and unsafe patterns in emscripten/ARimageFsetDisplay.cpp

Environment

  • Product/Service: FeatureSET-Display — Emscripten/wasm native layer
  • File: emscripten/ARimageFsetDisplay.cpp

Problem Description

Several distinct bugs and unsafe patterns exist in the native C++ layer:

  1. NULL-pointer dereference (lines 109–128): loadNFTMarker logs an error when ar2ReadSurfaceSet returns NULL but continues execution, then dereferences the null pointer.
  2. exit() terminates the wasm runtime (lines 148, 154): exit(0) / exit(-1) on file errors tear down the entire wasm runtime in the browser instead of returning an error code to JS.
  3. Memory leak in setup(): setup() mallocs imgBW with width*height*4, but loadNFTMarker later overwrites the pointer with surfaceSet[...]->imgBW — the original malloc'd buffer leaks. A second malloc based on uninitialized num_F_points_NFT also leaks.
  4. Pointer truncation in log statements: ARLOGi("...%d", arc->imgBW) and (int)arc->imgBW truncate 64-bit pointers — use %p / uintptr_t.
  5. Bounds check after write (line 153): The if (surfaceSetCount == PAGES_MAX) exit(-1) guard fires after the array has already been written; the check must be reordered to precede the write.

Expected Behavior

  • loadNFTMarker returns an error code when ar2ReadSurfaceSet returns NULL; execution does not continue.
  • File errors return error codes to JS instead of calling exit().
  • No memory leaks in setup() or loadNFTMarker.
  • Pointer values logged correctly with %p / uintptr_t.
  • Bounds check occurs before writing to the array.

Actual Behavior

  • NULL dereference crash possible after a failed surface-set load.
  • exit() calls kill the wasm module on file errors, breaking the page.
  • imgBW malloc leak on every loadNFTMarker call.
  • Pointer values silently truncated in logs on 64-bit targets.
  • Out-of-bounds array write possible before the PAGES_MAX guard triggers.

Tasks

  • Guard loadNFTMarker against NULL return from ar2ReadSurfaceSet and return an error code
  • Replace exit(0) / exit(-1) with proper error-code returns (lines 148, 154)
  • Free the original imgBW malloc before overwriting the pointer in loadNFTMarker; audit the num_F_points_NFT-based malloc
  • Fix ARLOGi calls to use %p / (uintptr_t)arc->imgBW
  • Reorder the PAGES_MAX bounds check to occur before the array write

Impact

High — NULL dereference and exit() calls are crash/runtime-termination bugs. Memory leaks compound on repeated marker loads.

Additional Context

These fixes pair naturally with rebuilding the wasm binary anyway (see issue 3b — toolchain refresh). Fixing exit() calls is a prerequisite for any production use in a browser tab.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions