From 86b61a0523da90fa4b4c52aaadabbff44d656eee Mon Sep 17 00:00:00 2001 From: kalwalt Date: Wed, 23 Sep 2026 23:45:31 +0200 Subject: [PATCH 1/2] fix(kpm): bound matcher image ids and the reference image count Two review findings on #73 (pre-existing, widened by #71's per-page results): - kpmSetRefDataSet() never clears the FREAK matcher, so when a reference set is replaced by a smaller one its extra images stay in the matcher and are still matched. Their ids keep old pageIndices entries that can point past the new, smaller result[]. Record how many images the current set registered (dbImageNum) and ignore, with an error, any match at or above it, or mapping outside result[]. Clearing the matcher on reload is the proper fix and belongs to the non-idempotent kpmSetRefDataSet (webarkit/jsartoolkitNFT#612). - pageIDs[] and pageIndices[] hold DB_IMAGE_MAX (1024) entries, one per registered image, but nothing bounded the image count. Reject a dataset with more images than that before changing any state. Built and tested through jsartoolkitNFT (the only build that compiles lib/SRC/KPM): detection, multi-marker and marker-limit suites, 82/82. Refs webarkit/jsartoolkitNFT#635, webarkit/jsartoolkitNFT#612 Co-Authored-By: Claude Opus 5.5 --- lib/SRC/KPM/kpmMatching.cpp | 30 +++++++++++++++++++++++++++++- lib/SRC/KPM/kpmPrivate.h | 1 + 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/lib/SRC/KPM/kpmMatching.cpp b/lib/SRC/KPM/kpmMatching.cpp index 4246ace..4c2eb76 100644 --- a/lib/SRC/KPM/kpmMatching.cpp +++ b/lib/SRC/KPM/kpmMatching.cpp @@ -189,6 +189,21 @@ int kpmSetRefDataSet( KpmHandle *kpmHandle, KpmRefDataSet *refDataSet ) ARLOGe("kpmSetRefDataSet(): refDataSet.\n"); return -1; } +#if BINARY_FEATURE + // pageIDs[] and pageIndices[] hold one entry per registered image (one per + // page per scale). Refuse a dataset that would not fit before changing any + // state, instead of writing past them. + { + int imageTotal = 0; + for( i = 0; i < refDataSet->pageNum; i++ ) { + imageTotal += refDataSet->pageInfo[i].imageNum; + } + if( imageTotal > DB_IMAGE_MAX ) { + ARLOGe("kpmSetRefDataSet(): %d reference images exceed DB_IMAGE_MAX (%d).\n", imageTotal, DB_IMAGE_MAX); + return -1; + } + } +#endif // Copy the refPoints into the kpmHandle's dataset. if( kpmHandle->refDataSet.refPoint != NULL ) { @@ -300,6 +315,7 @@ int kpmSetRefDataSet( KpmHandle *kpmHandle, KpmRefDataSet *refDataSet ) kpmHandle->freakMatcher->addFreakFeaturesAndDescriptors(points,descriptors,points_3d,kpmHandle->refDataSet.pageInfo[k].imageInfo[m].width,kpmHandle->refDataSet.pageInfo[k].imageInfo[m].height,db_id++); } } + kpmHandle->dbImageNum = db_id; } #endif @@ -651,7 +667,19 @@ for (int pageLoop = 0; pageLoop < kpmHandle->resultNum; pageLoop++) { const vision::image_matches_t& imageMatches = kpmHandle->freakMatcher->matches(); std::map > candidatesPerPage; for (const vision::image_match_t& imageMatch : imageMatches) { - candidatesPerPage[kpmHandle->pageIndices[imageMatch.id]].push_back(&imageMatch); + // kpmSetRefDataSet() does not clear the matcher, so after a reference set is + // replaced by a smaller one its extra images are still matched. Their ids are + // at or above dbImageNum and map to page positions this result[] may not have. + if (imageMatch.id < 0 || imageMatch.id >= kpmHandle->dbImageNum) { + ARLOGe("kpmMatching: ignoring stale matcher image %d (current images: %d).\n", imageMatch.id, kpmHandle->dbImageNum); + continue; + } + const int pageIndex = kpmHandle->pageIndices[imageMatch.id]; + if (pageIndex < 0 || pageIndex >= kpmHandle->resultNum) { + ARLOGe("kpmMatching: image %d maps to page position %d, outside 0..%d.\n", imageMatch.id, pageIndex, kpmHandle->resultNum - 1); + continue; + } + candidatesPerPage[pageIndex].push_back(&imageMatch); } ARLOGd("kpmMatching: %d image match(es) across %d page(s)\n", (int)imageMatches.size(), (int)candidatesPerPage.size()); diff --git a/lib/SRC/KPM/kpmPrivate.h b/lib/SRC/KPM/kpmPrivate.h index bc87681..0b3a04a 100644 --- a/lib/SRC/KPM/kpmPrivate.h +++ b/lib/SRC/KPM/kpmPrivate.h @@ -89,6 +89,7 @@ struct _KpmHandle { int resultNum; int pageIDs[DB_IMAGE_MAX]; int pageIndices[DB_IMAGE_MAX]; // position in refDataSet.pageInfo (and result[]) of each db_id's page + int dbImageNum; // images registered by the last kpmSetRefDataSet(): ids 0..dbImageNum-1 are current }; #endif // !__kpmPrivate_h__ From 09d0479dd881e11f2a3680b92f472764a963a9c6 Mon Sep 17 00:00:00 2001 From: kalwalt Date: Thu, 24 Sep 2026 00:04:03 +0200 Subject: [PATCH 2/2] fix(kpm): register each reference set in a fresh matcher Review findings on #74: - Reusing the matcher across kpmSetRefDataSet() calls kept every keyframe it held. An id registered again was refused as a duplicate (the old keyframe stayed) while the facade still replaced its 3D points, so pose estimation could pair old matches with a new, shorter point list and index past it. The dbImageNum check cannot see this: the id is in range. Recreate the matcher before registering the new set; it carries no configuration beyond construction, so the new instance is equivalent. The id and page-position checks in kpmMatching stay as invariant guards. - The DB_IMAGE_MAX preflight summed imageNum values in a signed int before comparing, so a corrupt dataset could overflow past the check. Check each count against the room left before adding it, and reject negative counts. Built and tested through jsartoolkitNFT: detection, multi-marker and marker-limit Vitest suites 82/82; Node example detects 10/10. Refs webarkit/jsartoolkitNFT#612, webarkit/jsartoolkitNFT#635 Co-Authored-By: Claude Opus 5.5 --- lib/SRC/KPM/kpmMatching.cpp | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/lib/SRC/KPM/kpmMatching.cpp b/lib/SRC/KPM/kpmMatching.cpp index 4c2eb76..28ec29b 100644 --- a/lib/SRC/KPM/kpmMatching.cpp +++ b/lib/SRC/KPM/kpmMatching.cpp @@ -194,13 +194,16 @@ int kpmSetRefDataSet( KpmHandle *kpmHandle, KpmRefDataSet *refDataSet ) // page per scale). Refuse a dataset that would not fit before changing any // state, instead of writing past them. { + // Check each count against the room left before adding it, so a corrupt + // or hostile dataset cannot overflow the running total past the check. int imageTotal = 0; for( i = 0; i < refDataSet->pageNum; i++ ) { - imageTotal += refDataSet->pageInfo[i].imageNum; - } - if( imageTotal > DB_IMAGE_MAX ) { - ARLOGe("kpmSetRefDataSet(): %d reference images exceed DB_IMAGE_MAX (%d).\n", imageTotal, DB_IMAGE_MAX); - return -1; + const int imageNum = refDataSet->pageInfo[i].imageNum; + if( imageNum < 0 || imageNum > DB_IMAGE_MAX - imageTotal ) { + ARLOGe("kpmSetRefDataSet(): page %d has %d reference images; the set exceeds DB_IMAGE_MAX (%d).\n", i, imageNum, DB_IMAGE_MAX); + return -1; + } + imageTotal += imageNum; } } #endif @@ -284,6 +287,16 @@ int kpmSetRefDataSet( KpmHandle *kpmHandle, KpmRefDataSet *refDataSet ) free(featureVector.sf); } #else + // Register the new set in a fresh matcher. Reusing the old one kept every + // keyframe it already held: an id the new set registers again is refused + // as a duplicate (the old keyframe stays) while its 3D points are replaced, + // so pose estimation paired old matches with new, shorter point lists. The + // matcher carries no configuration beyond construction (kpmCreateHandle), + // so a new instance is equivalent to the original. + delete kpmHandle->freakMatcher; + kpmHandle->freakMatcher = new vision::VisualDatabaseFacade; + kpmHandle->dbImageNum = 0; + if (kpmHandle->refDataSet.num != 0) { featureVector.num = kpmHandle->refDataSet.num; @@ -667,11 +680,11 @@ for (int pageLoop = 0; pageLoop < kpmHandle->resultNum; pageLoop++) { const vision::image_matches_t& imageMatches = kpmHandle->freakMatcher->matches(); std::map > candidatesPerPage; for (const vision::image_match_t& imageMatch : imageMatches) { - // kpmSetRefDataSet() does not clear the matcher, so after a reference set is - // replaced by a smaller one its extra images are still matched. Their ids are - // at or above dbImageNum and map to page positions this result[] may not have. + // Invariant checks. kpmSetRefDataSet() registers each set in a fresh + // matcher, so every id is below dbImageNum and maps into result[]; should + // that ever break, log it rather than index the mapping arrays out of bounds. if (imageMatch.id < 0 || imageMatch.id >= kpmHandle->dbImageNum) { - ARLOGe("kpmMatching: ignoring stale matcher image %d (current images: %d).\n", imageMatch.id, kpmHandle->dbImageNum); + ARLOGe("kpmMatching: matcher image %d is outside the current set (%d images).\n", imageMatch.id, kpmHandle->dbImageNum); continue; } const int pageIndex = kpmHandle->pageIndices[imageMatch.id];