mp_fdjac2: guard non-finite numerical derivatives

[rocketmark]

Summary

mp_fdjac2's only protection against a non-finite finite-difference derivative is assert(isfinite(fjac[ij])), which a release build (-DNDEBUG) strips out entirely — and the two-sided derivative path has no check at all. If the user function returns a degenerate residual for some parameter value, wa[i]/fvec[i] can be non-finite, writing NaN/Inf straight into fjac.

That NaN then poisons fnorm/ratio in mpfit()'s outer Levenberg-Marquardt loop. Since any comparison against NaN is false, the iteration counter (which only advances inside the ratio>=p0001 success branch) never increments, so the maxiter check that's supposed to bound the loop never fires.

Demonstration

BEFORE fix:
User function returns a degenerate residual for some trial parameter value
mp_fdjac2: writes NaN/Inf into fjac (assert is a no-op under -DNDEBUG)
mpfit(): outer loop's ratio comparison against NaN is always false
Iteration counter never advances; maxiter check never fires
Result: solver thread spins indefinitely instead of returning MP_MAXITER

AFTER fix:
User function returns a degenerate residual for some trial parameter value
mp_fdjac2: detects non-finite fjac, sanitizes to 0
mpfit(): column treated as having no measurable gradient (same fallback mp_qrfac already uses for a zero-norm column)
Result: solver proceeds normally and returns a clean status (success or MP_MAXITER), never hangs

Impact

Any caller whose residual function can produce a degenerate/non-finite value for some parameter trial is exposed — e.g. a rank-deficient or otherwise ill-posed fit reached transiently during normal operation (in our case, immediately after a scene/calibration reset). On a release build this manifests as a CPU-pinned thread that never returns and never logs anything, rather than a clean failure return, since the only existing guard (assert) is compiled out.

Change

One file, redist/mpfit/mpfit.c, function mp_fdjac2. Added if (!isfinite(fjac[ij])) fjac[ij] = 0; immediately after each of the three places fjac[ij] is computed from a finite-difference (one-sided non-debug, one-sided debug, two-sided non-debug), ahead of the existing assert where one exists.

Found via

Reproduced on real hardware (Raspberry Pi 4B running the GSS/MPFIT poser): after a tracker reinit while actively tracking, the GSS solver thread would intermittently spin at ~95-100% CPU for several minutes with zero log output, confirmed via gdb thread dumps (stuck inside mpfit() → mp_fdjac2, FPU status register showing divide-by-zero/invalid-operation flags set, ~465k spin iterations/sec with no progress).

Notably, the same libsurvive build reproduced this reliably on one Raspberry Pi OS release but not on an earlier one under the identical trigger sequence — we were unable to identify a specific OS-level change responsible (kernel, systemd/udev, and firmware/EEPROM package diffs between the releases were all ruled out), which suggests the divide-by-zero is being reached via a timing-sensitive path rather than anything OS-version-specific in nature, but doesn't rule one out. This patch closes the gap regardless of root timing cause and has held up under repeated reinit-while-tracking testing and an overnight soak on the affected hardware.

[bl4ckb0ne]

If we now have a way to handle finite values without crashing, I don't think the assert here and on line 1243 are relevant anymore. They should be removed in favour of your new check.

[throni3git]

this is possibly related to #310 as in the /* Non-debug path for speed */ path there is some hickup from a copy-paste error.

[rocketmark]

this is possibly related to #310 as in the /* Non-debug path for speed */ path there is some hickup from a copy-paste error.

Ok. I'll compare and do a test to see if the merge #310 covers it. We can possibly still simplify the checks to be a bit less brittle. Will follow up here if still valid, or close the PR if not.

[bl4ckb0ne]

I think it would be good to have both, #310 only covered the non-debug path, the other one can still have non-finite values.

[throni3git]

as the calculations were originally coded at [1], this check should be put there as well. now unfortunately, the page seems to be down :(

[1] https://pages.physics.wisc.edu/~craigm/idl/cmpfit.html

[rocketmark]

I think it would be good to have both, #310 only covered the non-debug path, the other one can still have non-finite values.

Updated and pushed. I'm going to look at this other issue throni3git mentioned in case it is related.

[bl4ckb0ne]

@throni3git i found it on the wayback machine here 1

[bl4ckb0ne]

Could you squash all three commits together please, keep only the first commit message because it is the only relevant one.

There's also a rebase conflict that needs to be addressed.

[throni3git]

@throni3git i found it on the wayback machine here 1

thanks, that's good. there seems to be an even newer version 1.5 (strangely dating to 2022 which is before i looked at it in 2024). maybe this is also worth it bringing in...

[rocketmark]

Could you squash all three commits together please, keep only the first commit message because it is the only relevant one.

There's also a rebase conflict that needs to be addressed.

fixed and pushed.

[bl4ckb0ne]

LGTM, thanks for bearing through those iterations