DM-55992: Fix SEGV when failing to make SplineMap into a SIP WCS

[timj]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 83.24873% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.73%. Comparing base (756d5ac) to head (2861a52).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/fitschan.c	83.24%	4 Missing and 29 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   61.63%   61.73%   +0.10%     
==========================================
  Files          83       83              
  Lines       96521    96706     +185     
  Branches    30597    30636      +39     
==========================================
+ Hits        59487    59700     +213     
+ Misses      20843    20803      -40     
- Partials    16191    16203      +12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[TallJimbo]

Commit messages have some painfully Claude-voiced text, if that sort of thing matters in this repo. Otherwise looks good, though I'm curious from the description why this was as rare as it seemed to be.

[timj]

Analysis from Claude:

→ The rarity is not luck with undefined behavior. It is a discrete switch: SplineMap.OutUnit.

The segfault needs two things at once: SIPIntWorld must have accepted a SIP description, and FitLine must then fail. At FitsTol=1e-4 over a real image box, FitLine fails for every one of these FrameSets. So the only thing that varies across your 700 is whether the SIP description is accepted at all.

→ Why acceptance is a coin toss: SIPIntWorld tests the post-PolyMap Mapping over a box in the wrong units. It uses astLinearApprox over ±NAXIS, but that box is measured in the PolyMap's output space, where one pixel is about 4.9e-4 units. So the box is roughly 4 million pixels wide, while the spline's knots span about 0.46 units (~950 pixels). Every sample point lands outside the knots.

→ OutUnit then decides everything. Default is 0: out-of-knot points come back AST__BAD, astLinearApprox fails, SIPIntWorld returns NULL, and astWrite cleanly returns 0. Your lsst2.ast has OutUnit = 1 explicitly set, so those points pass through unchanged and the check sees a perfectly linear map (fit offsets exactly 0.0). SIP accepted, FitLine fails, null deref.

→ Verified, not inferred. With both fixes reverted in an ASAN build: shipped fixture → SEGV fitschan.c:22140 in MakeIntWorld. Same fixture with OutUnit flipped to 0 → upper linapprox=0, SIPIntWorld returned NULL, astWrite -> 0, no crash. Nothing else in the run changed.

→ So on the LSST side, the thing to grep for is whatever sets OutUnit=1 on the SplineMap. Those are your handful. The rest were always taking the correct-refusal path, which is why ASAN over the old corpus never saw it.

→ Also worth knowing: CRPIX was AST__BAD for this fixture no matter what, because the spline's inverse is undefined at the IWC origin. So OutUnit=1 plus a loose FitsTol wouldn't have crashed, it would have silently written a header with SIP coefficients and no CTYPE. That is the second commit's bug, and it means some of your successful writes may be quietly broken rather than absent.

[timj]

Actually, it turns out that there were two real bugs:

1. The code was not picking a box in the right place when calculating correctness.
2. The fitstol in units of pixels was being used for something in units of degrees.

[timj]

More Claude analysis. It has some concern that this fix will mean that some of the new difference images will have slightly incorrect FITS WCS.

What this fixes

Four defects in the FITS-WCS write path, found while writing the pixels-to-sky FrameSet of a DP2 difference image (visit=2025042400322, detector=150) whose pixel-to-tangent-plane leg contains a SplineMap. That FrameSet is added as ast_tester/fixtures/wcsconv/inputs/lsst2.ast.

Commit	Defect
Fix segfault writing a non-linear Mapping through a FITS-WCS FitsChan	MakeIntWorld applied the SIP CD values to partmat rows that a failed FitLine had left NULL. Null dereference.
Reject a SIP description that has no reference pixel	SIPIntWorld returned AST__BAD CRPIX values as if they were good, producing a header with SIP coefficients and no CTYPE, CRPIX or CD cards, reported as a successful write.
Do not write alternate axis descriptions without the primary ones	WcsFromStore and PCFromStore reported success on the strength of an alternate description alone, which no FITS reader can use.
Test SIP linearity over the region the Mapping is used over	SIPIntWorld validated the post-polynomial Mapping over a box disjoint from the data, with a tolerance out by the plate scale.

Why the segfault was as rare as it looked

It is not luck with undefined behaviour. It is a discrete switch: SplineMap.OutUnit.

The crash needs two things at once — SIPIntWorld must accept a SIP description, and FitLine must then fail. At the FitsTol=1e-4 lsst-images uses, FitLine fails for every one of these FrameSets, so the only variable is SIP acceptance.

SIPIntWorld tests the Mapping following the PolyMap over a box centred on the SIP reference point. Here that point is ~20000 pixels from the detector, so every sample lands outside the spline knots. With OutUnit = 1 (set explicitly in this fixture; the class default is 0) those points pass through unchanged, the check sees an exactly linear Mapping, and SIP is accepted. Verified by flipping that one attribute with both fixes reverted:

• OutUnit = 1 → SEGV fitschan.c:22140 in MakeIntWorld
• OutUnit = 0 → astLinearApprox fails, SIPIntWorld returns NULL, astWrite cleanly returns 0

Nothing else in the run changes. Whatever sets OutUnit=1 upstream marks the images that crash; the rest were always taking the correct-refusal path, which is why ASAN over the existing corpus never saw it.

The linearity test was not testing linearity

Two independent problems at src/fitschan.c:29064, both measured on the fixture:

Box centred on the wrong point. map_upper's input is pixel offsets from the SIP reference point. The code tested ±NAXIS about the origin, which covers the image only if that point sits inside it.

tested: [-4072,-4072] .. [ 4072, 4072]
used: [12104,13264] .. [18783,17899] <- image corners through map_lower + PolyMap

Disjoint, not merely under-sampled.

Tolerance in the wrong units. astLinearApprox's tol is documented as a displacement in the output space; FitsTol is documented in pixels. Both calls passed fitstol raw. For map_lower (output = grid pixels) that is correct. For map_upper (output = IWC degrees) the default FitsTol=0.1 was applied as 0.1 degrees — 1827 pixels at the measured 0.197 arcsec/pixel.

This changes the answer, not just the rigour. Over the correct box the fit returns offsets -9.0e-6, -2.1e-5 deg, which trip the function's own origin-shift rejection; over the bogus box they are exactly 0.0 and it passes. The measured non-linearity of that leg over the real region is 1e-6 to 1e-5 deg (0.018 to 0.18 pixels), against a test that permitted 1827.

The fix derives the box by transforming a 3x3 grid spanning the image through the lower Mapping and the PolyMap, and scales the tolerance by the displacement a one pixel step produces in the upper Mapping's output.

Impact on existing output

Nothing in the repo changes. All 1253 tests pass with no reference regenerated, including every SIP fixture. That is structural rather than lucky: when the Mapping after the polynomial is exactly a CD matrix — what the SIP conventions expect and what every existing fixture has — its linear fit is identical over any box at any tolerance.

Output changes only where that Mapping is not exactly linear. Those headers do need regenerating, because the values written were never validated: MakeIntWorld discards the CRPIX and CD it derived from its own FitLine pass and substitutes SIPIntWorld's (src/fitschan.c:22234), which came from a fit over a region never required to overlap the data. The usual "within FitsTol anyway" argument does not apply to them.

On the synthetic case added to testfitschan, the old SIP header was 0.013 pixels off over the image and the new plain-linear one is 0.006. One constructed case, not a bound.

Tests

• ast_tester/fixtures/wcsconv/inputs/lsst2.ast with oracle coverage and a wcsconv case.
• testfitschan pins the crash, the missing primary axis description, and the alternate-only header.
• A new testfitschan case builds a FrameSet with the reference point outside the image and a cubic term negligible there but not over the image. It used to be written with SIP coefficients and a CD matrix fitted 20000 pixels away; it is now written as a plain linear approximation. Verified failing (Error 912) before the fix and passing after.

Future work

Not addressed here, each worth its own ticket:

1. The origin-shift threshold is absolute and unrelated to FitsTol. SIPIntWorld rejects on fabs(fit[0]) > 1.0E-7 in IWC units. Now that the fit is taken about the real region, a curved upper Mapping with a distant reference point trips it even when the implied error is well inside FitsTol. That is the mechanism by which the new test rejects rather than emitting a slightly-off SIP header. Defensible, but it should either be expressed in FitsTol terms or documented as a deliberate hard limit.

2. SIP-derived CRPIX and CD bypass the FitsTol contract entirely. There is no end-to-end check that the written SIP description reproduces the FrameSet over the image. FitLine validates the Mapping with the PolyMap removed, and its result is then overwritten. A cheap guard would be to round-trip the assembled description against the FrameSet at a grid of image positions before accepting it.

3. Downstream regeneration. Re-run the FITS approximation over the affected corpus against old and new AST and diff the headers; anything that changes was affected. Note that some FrameSets which previously produced a SIP header will now produce a plain linear approximation, and some will produce nothing at all if no approximation meets FitsTol — so consumers may see a WCS disappear rather than merely shift.

4. SplineMap.OutUnit deserves a warning in its documentation. Passing out-of-bounds points through unchanged makes a SplineMap look exactly linear to any sampling that misses the knots, which is how it silently defeated the linearity check here.

[timj]

@dsberry could you please take a look at the fitschan changes since it is possible that Claude found a real bug (rather than just a missing if check) with the region the linearity check was being applied.

[TallJimbo]

I can't claim I've understood the full sequence of events, but:

So on the LSST side, the thing to grep for is whatever sets OutUnit=1 on the SplineMap. Those are your handful. The rest were always taking the correct-refusal path, which is why ASAN over the old corpus never saw it.

doesn't sit well with me as a rarity explanation. I don't know if we're setting that or not, but we're certainly not only setting it on some detectors.

[timj]

@TallJimbo Okay so I asked it to drill down:

OutUnit is necessary but not the discriminator. The real answer is that there are two independent coincidences, and the SEGV needs both. Here's what the traces show.

→ First: SIP acceptance is not a near-miss, it's essentially guaranteed for these FrameSets, whatever the offset. astLinearApprox fits from just four points — the centres of the box faces (src/mapping.c:6690), i.e. (±NAXIS, 0) and (0, ±NAXIS) in pixel offsets from the reference point. Those four sweep out a diamond in the tangent plane, not a square. In your fixture the two arms run along (-0.701, +0.713) and (+0.990, +0.143), while the knot box sits at (0.806, 0.676) — between the arms. So no sample point ever lands on the spline, OutUnit makes it the identity there, the fit is exactly the CD matrix with offsets of exactly 0.0, and SIP is accepted.

→ Enlarging the box does not save you, which is what killed my earlier explanation. I swept NAXIS to 16000 (a ±0.875 deg box, well past the knots at 0.574 deg): the arms just get longer and still thread past. G9 PASS, SEGV, every time. Distance from the tangent point is not the variable.

→ Second, and this is the actual lottery: whether FitLine fails. FitLine tests the Mapping SIPIntWorld hands back — the original with the SIP polynomial removed. Because that polynomial carried most of the mapping, where the image lands in the spline's input space in the reduced mapping is a different question from where it lands in the real one. Inside the knots, the spline bites, FitLine fails, the rows stay NULL, you crash. Outside, the reduced mapping is exactly linear, FitLine passes, and there is no crash.

→ Proof that direction, not distance, decides it. I moved the knot region to the same 1.0522 deg from the reference point but along one sampling arm instead of between them:

base NAXIS=4072 G9 PASS FitLine FAIL -> SEGV
onarm NAXIS=4072 G9 PASS FitLine ok -> astWrite returns 1

Same offset magnitude. Only the position angle differs.

→ The important part: the non-crashing branch is not a clean refusal. It writes a header, and the header is garbage. The onarm case emitted 41 cards: NAXIS, RADESYS, and 38 SIP coefficients, with zero CTYPE, CRPIX, CRVAL or CD cards — and astWrite returned 1. That's the defect the second commit fixes, and it is the common outcome for this class of FrameSet, not the rare one.

→ So: the handful of segfaults are the visible tip. Expect a much larger population that silently produced an unusable header instead. Two cheap checks on your side: count products whose WCS carries A_ORDER but no CTYPE1, and re-run as_fits_wcs over the corpus against old and new AST and diff. I can only demonstrate the mechanism on this one fixture plus synthetic variants of it; which branch each of your thousands took has to be measured there.

Replacement for that section of the PR body:

Why the segfault was as rare as it looked

The crash needs two independent coincidences, and only one of them is rare.

SIP acceptance is effectively guaranteed for these FrameSets.
astLinearApprox fits from four points only — the centres of the box faces
(src/mapping.c:6690), so (±NAXIS, 0) and (0, ±NAXIS) in pixel offsets from
the SIP reference point. Those four sweep out a diamond in the tangent plane. In
this fixture the arms run along (-0.701,+0.713) and (+0.990,+0.143) while the
spline's knots sit at (0.806,0.676), between them, so no sample point ever
touches the spline. With OutUnit set the spline is the identity there, the fit
comes back as exactly the CD matrix with origin offsets of exactly 0.0, and the
SIP description passes. Enlarging the box does not help: swept to NAXIS=16000
(a ±0.875 deg box, against knots starting at 0.574 deg) the arms simply lengthen
and still thread past. Distance from the tangent point is not the variable.

What decides crash versus silent corruption is whether FitLine then fails.
FitLine tests the Mapping SIPIntWorld returns, which is the original with the
SIP polynomial removed. Since that polynomial carried most of the mapping, where
the image falls in the spline's knots under the reduced mapping is a different
question from where it falls under the real one. Inside the knots: FitLine
fails, the partmat rows stay NULL, SEGV. Outside: the reduced mapping is
exactly linear, FitLine passes, no crash.

Moving the knot region to the same 1.0522 deg from the reference point but along
one sampling arm rather than between them flips the outcome with nothing else
changed:

variant	NAXIS	origin-shift test	FitLine	outcome
as shipped	4072	pass	fail	SEGV
knots on a sampling arm	4072	pass	ok	astWrite returns 1

The non-crashing branch is the worse one. That second row emitted 41 cards —
NAXIS, RADESYS and 38 SIP coefficients — with no CTYPE, CRPIX, CRVAL or
CD card at all, and reported success. That is the defect the second commit
fixes, and for this class of FrameSet it is the common outcome rather than the
rare one. The segfaults are the visible tip; a larger population is likely to
have produced unusable headers instead. Worth checking downstream for products
carrying A_ORDER with no CTYPE1.

[TallJimbo]

Ok, that does match my experience: I had a workaround for the "write some headers, but not a valid WCS" problem on the branch when I ran this on the DP2 difference images, so I wouldn't have seen those failures in my counts.

[timj]

So since the code was testing the linear fit in a box that was in the wrong place it never encountered the splines. Looks like it's only using 4 points so even in the right box it's possible that those 4 points end up in places that do not have any correction.

---

The fit sampled (±4072, 0) and (0, ±4072) around a reference point ~20000 pixels from the data. Those points never enter the spline's knots, OutUnit hands back the identity there, so map_upper reduces to the exact CD matrix: deviation exactly 0, offsets exactly 0.0. The test was measuring a Mapping the spline wasn't part of.

→ With the box derived from the image, the spline is in the fit. Measured, commits 1 and 2 reverted so a wrong acceptance would still crash:

box [12103.9,13263.9]..[18783.1,17898.8] utol=5.55e-09 -> FAIL
SIPIntWorld -> NULL astWrite -> 0 no crash

Same clean rejection for the onarm variant and the half-offset one. Since map_upper is map3 ∘ SplineMap and map3 is an exact matrix, every bit of that measured non-linearity is the spline. There is nothing else in there to fail.

→ The box is the load-bearing half of that commit, not the tolerance. With the old box the deviation is exactly zero, so no tolerance however tight would have caught it. With the correct box either half rejects on its own: the deviation exceeds the converted tolerance, and the origin offsets come out at -9.0e-6, -2.1e-5 deg, which trip the existing 1e-7 test even at the old unconverted tolerance.

→ Where "completely" is too strong, three residual holes. Worth knowing before you rely on it:

The sampling is still sparse — 4 fit points at the box face centres plus the box vertices as test points. The fix corrects where they land, not how many there are. A perturbation that happens to be linear at exactly those points still slips through.

The box is a bounding box of the transformed image, so when the mapping rotates or skews it is larger than the true footprint. Its face-centre and vertex samples can fall outside the footprint, and if a spline's knots covered only the footprint those samples would be out of range again. Doesn't bite on this fixture (the knots are generous), but the hole is real.

Nothing validates the assembled SIP description end to end. MakeIntWorld still discards its own FitLine CRPIX and CD in favour of SIPIntWorld's, and no check compares the finished description against the FrameSet over the image. That's the future-work item that would close the class of bug rather than this instance.