preview_data: guard the pipe-nodes walk in dt_preview_data_is_fresh with busy_mutex

[TomPoczos]

Summary

dt_preview_data_is_fresh() (introduced in 8a26f35 / #21397, the shared preview-data service for interactive editing) walks dev->preview_pipe->nodes and hashes the matching piece from the GUI thread while holding only the module's own gui_lock. pipe->nodes is actually protected by pipe->busy_mutex: dt_dev_pixelpipe_cleanup_nodes() frees every piece under that lock whenever a history change rebuilds the pipe topology (DT_DEV_PIPE_REMOVE in dt_dev_pixelpipe_change), and that can run concurrently on the pipe-processing thread. A caller that walks pipe->nodes off the GUI thread without busy_mutex can therefore dereference a piece that gets freed out from under it.

Root-caused from a real crash (SIGSEGV dereferencing a freed piece->module in _dev_pixelpipe_cache_basichash(), backtrace attached below) hit by an out-of-tree module whose preview-pipe-finished GUI callback calls dt_preview_data_is_fresh(). colorequal.c's own interactive hue-editing mode (also from #21397) calls dt_preview_data_is_fresh() from mouse-hover/scroll handlers and is exposed to the same race in principle, just from a narrower window that makes it harder to trigger in practice.

• Take pipe->busy_mutex around the walk.
• Must be a trylock, not a blocking lock: a module's process() runs under busy_mutex for the whole pipe run (dt_dev_pixelpipe_process) and can itself call dt_preview_data_store() (colorequal.c does, twice), which takes this same module's gui_lock — the opposite order — so a blocking lock here would AB-BA deadlock against it.
• A pipe that's currently busy can't have fresh data for us anyway, so treating "busy" as "not fresh" costs nothing.

Crash backtrace (relevant frames)

#6  _dev_pixelpipe_cache_basichash (pipe=..., position=36, roi=...) at src/develop/pixelpipe_cache.c:133
#7  dt_dev_pixelpipe_cache_hash (...) at src/develop/pixelpipe_cache.c:164
#8  dt_dev_pixelpipe_piece_hash (...) at src/develop/pixelpipe_hb.c:4250
#9  dt_preview_data_is_fresh (...) at src/develop/preview_data.c:201
#10 <out-of-tree module's preview-pipe-finished GUI callback>

Test plan

• cmake --build build --target darktable — clean build, no new warnings, on top of current master (fd59685185)
• Traced the actual lock-acquisition order from the crash's own backtrace (a worker thread inside a module's process(), holding busy_mutex via the pipe-run wrapper, blocked on this module's gui_lock) to confirm the trylock is required and correct, not just a plausible-looking fix
• No minimal standalone repro of the race's timing itself (it depends on a history-change-triggered pipe rebuild landing inside the GUI thread's freshness check) — would appreciate a maintainer/CI sanity check on colorequal/toneequal's existing interactive-mode paths

🤖 Generated with Claude Code

https://claude.ai/code/session_01WpGKm49b4w1L51YETXyGpU

[TomPoczos]

Just so that you get something other than AI slop, I was playing around with a private module when I ran into a rather easy to reproduce segfault. Turned out it's not from my code and this appears to be the fix. Thought that it should be in the official codebase.

[wpferguson]

@jenshannoschwalm

[TurboGit]

I'll let @jenshannoschwalm review and have the final word but to me this looks correct.

[Copilot]

🟢 Approval recommended

The change correctly matches the pixelpipe’s locking contract (protecting pipe->nodes with busy_mutex) while using trylock to avoid the documented lock-order deadlock risk.

Pull request overview

This PR fixes a concurrency hazard in the preview-data freshness check by ensuring dt_preview_data_is_fresh() only walks preview_pipe->nodes while holding the pixelpipe’s busy_mutex, preventing use-after-free during concurrent pipe topology rebuilds.

Changes:

• Acquire pipe->busy_mutex via trylock around the pipe->nodes walk and subsequent hash computation.
• Treat a busy pipe (trylock fails) as “not fresh” to avoid blocking and potential lock-order deadlocks with gui_lock.

File summaries

File	Description
src/develop/preview_data.c	Wraps the preview-pipe node traversal/hash in a busy_mutex trylock to prevent races with node cleanup/rebuild.

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.

[jenshannoschwalm]

Yes, correct to me too.