Skip to content

fix(rgbd): prevent SIGSEGV in linemod::orUnaligned8u from stale alignment fast path - #4194

Open
SomSamantray wants to merge 7 commits into
opencv:4.xfrom
SomSamantray:fix/linemod-orunaligned8u-simd-crash
Open

fix(rgbd): prevent SIGSEGV in linemod::orUnaligned8u from stale alignment fast path#4194
SomSamantray wants to merge 7 commits into
opencv:4.xfrom
SomSamantray:fix/linemod-orunaligned8u-simd-crash

Conversation

@SomSamantray

Copy link
Copy Markdown

Reopens #4192, which was closed because it included an accidental internal planning artifact (docs/plans/*.md) unrelated to the fix. That file has been removed from this branch; only the actual fix and its test remain.

Problem

cv::linemod::Detector::match() could crash the process with SIGSEGV whenever the input image's width (or its size after one pyrDown() pyramid level) wasn't a multiple of 16 — a size restriction nowhere documented or enforced, so ordinary images could trigger it unpredictably.

The crash was inside orUnaligned8u() (modules/rgbd/src/linemod.cpp), which computes whether its src pointer is 16-byte aligned once, before its row loop, and reuses that single decision for every row. But src/dst advance by src_stride/dst_stride each row, and those strides aren't guaranteed to be multiples of 16 (spread()'s destination stride is exactly the image's row width in bytes). Once a stride isn't 16-aligned, the pointer drifts in and out of alignment from row to row — so the branch decision made at row 0 goes stale, and a later row can hit a plain __m128i* dereference (which the compiler lowers to an aligned movdqa) on a pointer that is no longer actually aligned.

Fix

Removes the once-per-call alignment fast path entirely and always accesses src/dst with unaligned SSE intrinsics (_mm_loadu_si128/_mm_lddqu_si128/_mm_storeu_si128). The remaining SSE3-vs-SSE2 branch reflects genuine hardware capability, not a (now provably unsound) alignment assumption. Output is unchanged for every previously-working input; only the crash is eliminated.

Fixes opencv/opencv#29559
Related: opencv/opencv#29576 (same crash signature via Python bindings; very likely the same root cause, not independently verified here)

Validation

Since building the full opencv + opencv_contrib dependency chain just to run one test wasn't practical for this fix's turnaround, the fix was verified with a standalone harness that reproduces orUnaligned8u()'s exact pre-fix and post-fix logic outside the build (same compiler/flags OpenCV uses for its SSE2/SSE3 code paths):

  • Pre-fix logic crashes on a destination row stride that isn't a multiple of 16 (the real production trigger — matched to how spread() actually calls this function), on an initially-offset pointer, and on mixed unaligned src/dst.
  • Post-fix logic completes correctly (bit-identical to an independent scalar reference) on all of the above, across both the SSE3 and SSE2-fallback code paths.

A regression test (modules/rgbd/test/test_linemod.cpp) is included, exercising the fix through the public Detector::addTemplate()/match() API with a synthetic image sized so its row width isn't 16-byte-aligned at every pyramid level — reproducing the original crash through the real call path, then asserting an actual correct match (not just crash-freedom). It has not been compiled locally in this environment; it follows the module's existing test conventions and its logic mirrors the already-verified standalone harness.

One known coverage gap: the SSE2-only (no-SSE3) fallback branch this fix also touches isn't independently exercised by the new test on typical SSE3-capable CI hardware, since checkHardwareSupport(CPU_SSE3) is true almost everywhere the test would run. The SSE3 and SSE2 branches are identical in shape (only the load intrinsic differs), so risk is low, but flagging for visibility.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

SomSamantray and others added 6 commits August 13, 2026 18:22
orUnaligned8u() computed src alignment once per call and reused that
decision for every row, but src/dst pointers drift in and out of
16-byte alignment across rows whenever the row stride isn't a
multiple of 16 (as happens in spread()'s dst.step1()). Once drifted,
the stale aligned-load/store branch faults with SIGSEGV.

Remove the alignment fast path entirely and always use unaligned SSE
load/store intrinsics for both src and dst; the remaining SSE3 vs
SSE2 branch reflects hardware capability, not an alignment
assumption.

Fixes opencv/opencv#29559
Likely also fixes opencv/opencv#29576 (same crash via Python bindings)
Adds modules/rgbd/test/test_linemod.cpp, exercising spread() (the
only caller of orUnaligned8u) with a quantized image row width that
is not a multiple of 16, reproducing the per-row alignment drift that
caused the SIGSEGV fixed in the previous commit. Included via direct
source inclusion since orUnaligned8u/spread are static internals not
exposed by the public API; cross-checks output against an independent
scalar reimplementation of spread(), not just crash-freedom.

Not built or run locally per this fix's verification approach
(standalone harness, see commit message above) -- validated by code
review against sibling test files and by construction against the
same mechanism the standalone harness already confirmed. Will run
under OpenCV's own CI once this PR is opened.

Part of opencv/opencv#29559
Matches the established pattern in test_odometry.cpp for
reproducibility via the test framework's seed control.
Code review (two independent reviewers, correctness + adversarial)
found that #include "../src/linemod.cpp" -- the previous approach --
would break the build: linemod.cpp defines out-of-line member bodies
for CV_EXPORTS_W classes already compiled into opencv_rgbd, which
opencv_test_rgbd links against, causing dllimport-redefinition
(MSVC) or duplicate-symbol (link) errors. The cited precedents for
this pattern turned out to be header includes, not .cpp includes.

Rewritten to exercise the fix through cv::linemod::Detector's public
API instead: Detector::match() unconditionally calls spread() while
building its linear-memory pyramid on every invocation, and
ColorGradientPyramid::quantize() doesn't pad row width, so a
synthetic image sized with a non-16-aligned row width reaches the
exact stride-drift bug via addTemplate()+match() -- exercising the
real crash path end-to-end and asserting an actual correct match,
not just crash-freedom.

Fixes review findings on opencv/opencv#29559
docs/plans/*.md was an agent working-notes artifact, not part of the
actual patch; it isn't relevant to reviewers and doesn't belong in
the OpenCV tree.

@fallenmi fallenmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The production SIMD change looks directionally correct, but the added regression is red on the exact head and never reaches the code it is intended to cover.

I built aa55cc728f1815f3c84d2bb9bace4141c5826ede with current OpenCV 4.x core 755546643a671420fae0c20ec5d4cd24a228da8e on macOS arm64. Running only Rgbd_Linemod.match_survives_non16aligned_row_width fails at addTemplate(): template_id is -1, so the test stops at line 72 before Detector::match() can call spread() / orUnaligned8u().

Please make the fixture deterministically extract a template on supported architectures, then assert that the test actually reaches the match path. Since arm64 takes the scalar implementation, the crash regression also needs an x86/SSE execution gate (or an equivalent focused SIMD oracle) so a green non-x86 run cannot be mistaken for coverage of the faulty aligned-load path.

The exact head and test binary compile cleanly; the blocker is specifically that the newly submitted test itself fails and does not exercise the fix. GitHub currently exposes no check runs for this PR.

AI disclosure: I used OpenAI Codex to inspect and build the exact revisions, run the focused regression, and draft this review. I verified the commits, failure, and control flow locally.

…ift fix

The test added for opencv/opencv#29559 was failing before it ever reached
match()/spread()/orUnaligned8u(), and provided no real coverage on non-x86
CI runners. Both issues were raised by review on this PR.

- addTemplate() always returned -1: extractTemplate() restricts candidate
  features to the mask's border ring (mask minus its 1px erosion), and
  the previous all-255 mask has no such ring under BORDER_REPLICATE
  erosion. Switch to no mask at all -- with a checkerboard object confined
  to a black-bordered sub-region, feature candidates (and therefore the
  template bounding box) are naturally confined to the object regardless.

- Even with a fixed mask, match() found no candidate above the similarity
  threshold: the object filled the whole frame, leaving no room for
  matchClass()'s fixed 8*T search-refinement border. Scale the image up
  and confine the object to a centered region with real margin.

- Corrected the test's own alignment premise: linearize() requires each
  pyramid level's quantized image size be an exact multiple of that
  level's T (T_pyramid = {5, 8}). Under exact 2x pyrDown, that forces
  level 0's width to be 16-aligned whenever level 1's width is
  8-aligned, so the two levels can never both be non-16-aligned
  simultaneously as the previous version assumed. Level 1 alone being
  non-16-aligned is sufficient to reach the buggy code path.

- Added a runtime cv::checkHardwareSupport(CV_CPU_SSE2) gate that throws
  cvtest::SkipTestException when unavailable (precedented in
  modules/core/test/test_intrin.cpp), since orUnaligned8u()'s SSE2/SSE3
  paths only compile on x86 -- a green run on e.g. arm64 CI exercises only
  the scalar fallback and would otherwise be mistaken for SIMD coverage.

Verified locally end-to-end (with the SSE2 gate temporarily bypassed) by
building opencv_test_rgbd against upstream/4.x core: addTemplate() now
succeeds and match() finds a 100% similarity self-match. The committed
gate correctly reports an explicit SKIP (not a false pass) on this
session's non-SSE2 build hardware. The production fix in linemod.cpp is
unchanged.

@SomSamantray SomSamantray left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed repro — you were right on both counts. Pushed a fix that addresses both.

Test never reached addTemplate() success (template_id == -1): confirmed the root cause is in ColorGradientPyramid::extractTemplate() — when a mask is supplied, it restricts candidate features to the mask's border ring (mask minus its 1px BORDER_REPLICATE erosion). The test's mask was 255 everywhere including the image edges, so erosion never shrinks it and the ring is empty regardless of image content. Switched to passing no mask at all; with the checkerboard object confined to a black-bordered sub-region of the frame, candidate features (and the resulting template bounding box) are naturally confined to the object.

While fixing that I also hit two more issues the original fixture had with both now fixed:

  • Even with candidates found, match() returned zero results: the checkerboard filled the whole frame, leaving no room for matchClass()'s fixed 8*T search-refinement border. The image is now larger with the object confined to a centered region with real margin.
  • The original width/height premise (both pyramid levels non-16-aligned) is actually unsatisfiable together with linearize()'s requirement that each level's quantized size be an exact multiple of that level's T (T_pyramid = {5, 8}) — under exact 2x pyrDown(), requiring level 1's width to be a multiple of 8 forces level 0's width to be a multiple of 16. Only level 1 is non-16-aligned now; that alone is sufficient to reach orUnaligned8u()'s buggy path.

No SIMD coverage on non-x86 CI: added a runtime cv::checkHardwareSupport(CV_CPU_SSE2) gate that throws cvtest::SkipTestException (same pattern as modules/core/test/test_intrin.cpp) when SSE2 isn't available, so a non-x86 run reports an explicit skip rather than a pass that could be mistaken for coverage.

Verified locally end-to-end by building opencv_test_rgbd against upstream/4.x core: with the SSE2 gate temporarily bypassed for validation, addTemplate() now succeeds and match() finds a 100% similarity self-match on the exact non-16-aligned pyramid level. On this session's build hardware (aarch64, no SSE2), the committed test correctly reports as skipped rather than a false pass. I don't have x86/SSE hardware here to verify the positive SIMD-coverage case directly, so that leg relies on CI.

linemod.cpp (the production fix) is untouched by this update.

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.

[Bug] linemod::match crashes with SIGSEGV due to misaligned store in orUnaligned8u

2 participants