shearwater: fix userdata corruption in predator parser samples loop

[mikeller]

shearwater_predator_parser_samples_foreach serves two purposes:
an internal calibration pre-pass called with callback=NULL and a
dc_parser_sensor_calibration_t * as userdata, and normal sample
enumeration called with a real callback and the caller's own userdata.

The guard that restricted calibration writes to the pre-pass checked
only that userdata was non-NULL, so during normal sample enumeration it
cast the caller's userdata pointer to dc_parser_sensor_calibration_t *
and wrote into it. For any CCR dive with internal (non-external) ppo2,
this wrote the value 1 into the first byte of whatever struct the caller
passed, e.g. the low byte of divecomputer::when in Subsurface (a small
timestamp corruption) or a FILE * member in dctool (a crash).

Fix: add !callback to the guard so the internal calibration writes only
happen during the pre-pass (callback == NULL).

Fixes #127.

[Copilot]

🟢 Approval recommended

The change correctly limits calibration writes to the documented pre-pass path (callback == NULL) and the calibration pre-pass call site is present and consistent with this contract.

Pull request overview

Fixes a bug in the Shearwater Predator samples loop where the function’s dual-use of userdata (calibration pre-pass vs. normal enumeration) could corrupt caller-provided userdata during normal sample callbacks.

Changes:

• Restrict internal calibration writes to only occur during the calibration pre-pass by additionally requiring callback == NULL.

File summaries

File	Description
src/shearwater_predator_parser.c	Gates calibration-struct writes on !callback to prevent writing into caller userdata during normal sample enumeration.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[glance-]

This fixes the issue I've seen in #127 .

I don't like the fact that the "inner" shearwater_predator_parser_samples_foreach is called for all dives, and not just the ones where the calibration guestimate is needed.

Also the fact that it kinda abuses the parameters to signal something else than what they normally mean, and pass it's data via the userdata parameter. I'd prefer if the state data was kept in the parser_t instead of passed via the userdata callback. That would have prevented this bug to begin with.

Both are just related to this, and not blocking this fix.

[mikeller]

Agreed on both points. I've been thinking about the right way to address them.

On the userdata abuse specifically: the real problem isn't where the accumulator values live, it's that shearwater_predator_parser_samples_foreach has two incompatible calling conventions hidden behind one signature, distinguished only by callback == NULL. Moving the struct fields into the parser struct would just relocate the problem without making the dual-use explicit or type-safe. The cleaner fix is to extract a dedicated shearwater_predator_parser_calibration_prepass() static function that takes a dc_parser_sensor_calibration_t * directly, leaving samples_foreach as the sole owner of the vtable calling convention. That removes the !callback && userdata guard from inside the loop and makes the two paths explicitly distinct.

Once that refactor is in place, the unconditional pre-pass becomes easy to address: for non-CCR/SCR dives, both things the pre-pass collects (external_ppo2_used and the DiveCAN accumulators) are either unused or already gated on needs_divecan_calibration_estimate, so the pre-pass can simply be skipped.

I'll do both as follow-up commits.