fix(kpm): bound matcher image ids and the reference image count

[kalwalt]

Summary

Addresses the two Qodo findings on the 0.9.0 release sync #73. Both weaknesses predate #71, but #71 widened them by processing every match instead of only the best one. The fix lands here on dev, and #73 (dev → master) picks it up automatically.

1. Stale matcher images after reloading a smaller reference set

kpmSetRefDataSet() never clears the FREAK matcher, and db_id restarts at 0, so a replaced reference set's extra images stay in the matcher and are still matched. Their ids keep old pageIndices entries that can point past the new, smaller result[].

• KpmHandle gains dbImageNum: the number of images the current reference set registered. The handle is arMallocClear-ed, so it starts at 0.
• kpmMatching ignores, with an ARLOGe, any match whose id is >= dbImageNum, or whose page position falls outside result[].

This removes the out-of-bounds access. It does not make reloading correct: ids below the new count still refer to the previous set's features. Clearing the matcher on reload is the proper fix, and it belongs to the non-idempotent kpmSetRefDataSet (webarkit/jsartoolkitNFT#612).

2. No bound on the reference image count

pageIDs[] and pageIndices[] hold DB_IMAGE_MAX (1024) entries, one per registered image (pages × scales), but nothing checked the total. kpmSetRefDataSet() now counts the images first, and rejects the dataset with an error before changing any state.

Testing

WebARKitLib's CI builds the WebARKit/ tracker, not lib/SRC/KPM. So this was built and tested through jsartoolkitNFT (Docker, emsdk 4.0.17), which compiles this code into its WASM. There were no warnings in the changed files, and the detection, multi-marker and marker-limit Vitest suites pass 82/82.

Refs webarkit/jsartoolkitNFT#635, webarkit/jsartoolkitNFT#612

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Bound KPM reference images and stale matcher IDs

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Reject datasets exceeding the 1,024-image mapping capacity before mutating state.
• Track current matcher images and ignore stale or invalid result mappings.
• Prevent out-of-bounds result access when reference sets shrink.

Diagram

graph TD
    A["Reference Dataset"] --> B{"Count Within Limit"} -->|Yes| C["KPM Handle"] --> D["FREAK Matcher"] --> E{"Match Mapping Valid"} -->|Yes| F["Page Results"]
    B -->|No| G["Reject Dataset"]
    E -->|No| H["Ignore Match"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Clear matcher during dataset reload

• ➕ Fully removes stale features from the previous reference set.
• ➕ Corrects matches whose reused low IDs remain within dbImageNum.
• ➕ Eliminates reliance on filtering stale matcher output.
• ➖ Requires matcher lifecycle support or reconstruction.
• ➖ Broadens scope around the non-idempotent dataset-loading path.
• ➖ Carries greater regression risk than the defensive bounds fix.

2. Use dynamic image mappings

• ➕ Removes the fixed 1,024-image mapping capacity.
• ➕ Keeps mapping storage aligned with the loaded dataset size.
• ➖ Introduces allocation and ownership changes in KpmHandle.
• ➖ Does not independently solve stale matcher contents after reload.
• ➖ May permit impractically large datasets without broader resource limits.

Recommendation: Use the PR's defensive bounds checks as the scoped safety fix: they prevent memory corruption without changing matcher ownership. Follow separately with matcher clearing or reconstruction in kpmSetRefDataSet() to make reference-set replacement semantically correct, including stale features whose IDs fall below the new image count.

Files changed (2) +30 / -1

Bug fix (2) +30 / -1

kpmMatching.cppValidate reference capacity and matcher result mappings +29/-1

Validate reference capacity and matcher result mappings

• Counts reference images before mutating KPM state and rejects datasets exceeding DB_IMAGE_MAX. Records the number of currently registered images, then logs and skips matcher results with stale IDs or page indices outside result[].

lib/SRC/KPM/kpmMatching.cpp

kpmPrivate.hTrack the current matcher image count +1/-0

Track the current matcher image count

• Adds dbImageNum to KpmHandle so matching can distinguish IDs registered by the current reference dataset from stale matcher entries.

lib/SRC/KPM/kpmPrivate.h

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Reloaded references can crash matching ✓ Resolved 🐞 Bug ≡ Correctness

Description

kpmMatching treats every match below dbImageNum as current, but registrations restart at zero
while the matcher rejects duplicate keys and the facade still overwrites the corresponding
three-dimensional point vectors. After replacing a dataset, an old keyframe can therefore produce
reference indices into a shorter new vector, reaching unchecked refDataSet[matchData[i].ref]
indexing during pose estimation.

Code

lib/SRC/KPM/kpmMatching.cpp[R673-675]

+    if (imageMatch.id < 0 || imageMatch.id >= kpmHandle->dbImageNum) {
+        ARLOGe("kpmMatching: ignoring stale matcher image %d (current images: %d).\n", imageMatch.id, kpmHandle->dbImageNum);
+        continue;

Evidence

Each dataset registration starts IDs at zero, but duplicate IDs are rejected without replacing the
old keyframe; the facade nevertheless replaces the point vector under that ID. The added range check
accepts these reused IDs, and pose construction later indexes the new vector with reference indices
generated from the retained old keyframe.

lib/SRC/KPM/kpmMatching.cpp[287-318]
lib/SRC/KPM/FreakMatcher/facade/visual_database_facade.cpp[76-94]
lib/SRC/KPM/FreakMatcher/matchers/visual_database-inline.h[141-150]
lib/SRC/KPM/FreakMatcher/matchers/visual_database-inline.h[350-371]
lib/SRC/KPM/kpmMatching.cpp[673-675]
lib/SRC/KPM/kpmMatching.cpp[699-702]
lib/SRC/KPM/kpmMatching.cpp[749-758]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The numeric `dbImageNum` guard cannot identify stale keyframes whose reused IDs remain within the new dataset's range. Duplicate registrations retain the old keyframe while replacing its associated point vector, allowing pose estimation to index incompatible data.
## Fix Focus Areas
- lib/SRC/KPM/kpmMatching.cpp[280-318]
- lib/SRC/KPM/FreakMatcher/facade/visual_database_facade.cpp[76-94]
- lib/SRC/KPM/FreakMatcher/matchers/visual_database-inline.h[141-150]
## Recommended Fix
Clear or recreate the matcher before registering a replacement reference dataset so every accepted ID refers to the current keyframe and its matching point vector. Update `dbImageNum` only after the fresh matcher has been populated successfully.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Large reference sets can overrun arrays ✓ Resolved 🐞 Bug ≡ Correctness

Description

kpmSetRefDataSet accumulates signed imageNum values in the signed int imageTotal and compares
only the potentially overflowed final sum with DB_IMAGE_MAX. When positive page counts make the
mathematical total exceed INT_MAX, the preflight can be bypassed and the registration loop can
drive db_id beyond both fixed arrays.

Code

lib/SRC/KPM/kpmMatching.cpp[R197-200]

+        int imageTotal = 0;
+        for( i = 0; i < refDataSet->pageNum; i++ ) {
+            imageTotal += refDataSet->pageInfo[i].imageNum;
+        }

Evidence

Both the source counts and accumulator are signed integers, while the arrays have only 1024 entries
and the later writes have no independent bounds check. Consequently, overflow of the new accumulator
defeats the only added protection against an excessive number of registration iterations.

include/KPM/kpm.h[83-87]
lib/SRC/KPM/kpmMatching.cpp[197-203]
lib/SRC/KPM/kpmPrivate.h[45-45]
lib/SRC/KPM/kpmPrivate.h[90-92]
lib/SRC/KPM/kpmMatching.cpp[312-315]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new image-count preflight performs unchecked signed addition, so overflow can invalidate the comparison intended to protect the fixed-size image-mapping arrays.
## Fix Focus Areas
- lib/SRC/KPM/kpmMatching.cpp[197-203]
- lib/SRC/KPM/kpmMatching.cpp[312-315]
## Recommended Fix
Validate that every `imageNum` is nonnegative and reject each count before adding it when `imageNum > DB_IMAGE_MAX - imageTotal`. Keep the running total bounded by `DB_IMAGE_MAX`, and optionally retain an independent `db_id < DB_IMAGE_MAX` assertion before each array write.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗