dt_preview_data_is_fresh() walks the live preview-pipe node list under the wrong lock

Found: 2026-08-27, by Codex in the 05-50 review round of dev-doc PR 21912; verified
against the tree before filing.
File: src/develop/preview_data.c. No source file was edited.
Updated 2026-09-10: re-checked against f98cabfa21 on branch
dev-doc/gui-data-sharing-pr, whose src/ matches upstream master. The main defect,
the node walk in the title, was fixed on 2026-09-01 by e3d1b4fdb9 and is no longer
present. The smaller unlocked access reported alongside it is untouched and is what
remains open. Line numbers refreshed throughout; see Corrections at the end. If the
issue is retitled, the remaining defect is "dt_preview_data_is_fresh() tests pd->buf
before it takes the lock".

What still needs fixing

The early return reads pd->buf before the lock is taken at all
(src/develop/preview_data.c:171):

gboolean dt_preview_data_is_fresh(dt_preview_data_t *pd)
{
  if(!pd || !pd->module || !pd->buf) return FALSE;   /* 171 - no lock held */

  const dt_iop_module_t *const module = pd->module;
  dt_iop_gui_enter_critical_section((dt_iop_module_t *)module);   /* 174 */

dt_preview_data_store() and dt_preview_data_resize() both dt_free_align(pd->buf) and
assign a new pointer while holding that lock (:70-71 under the lock taken at :62,
and :112-113 under the lock taken at :104). The writers run on a pipe worker thread:
colorequal calls store() from process() and process_cl() (colorequal.c:1138,
:1653). The readers are GUI handlers. So this is an unsynchronized read of a field
written under a lock: a data race by the C11 memory model, and formally undefined.

The practical consequence is mild, and the report is worth reading with that in mind.
The value is used for a NULL test and then discarded, never dereferenced, so the outcome
on any real platform is a stale answer rather than a fault. The early return can only
produce a wrong FALSE, never a wrong TRUE: passing the test just means the function
goes on to take the lock and re-read pd->hash at :177. Both callers absorb that by
requesting a preview reprocess behind a g->reprocess_pending flag (colorequal.c:2361,
:2779), and the next call gets the right answer.

What is left is a race a sanitizer run will report, and an exception to the one guarantee
the service exists to provide. It is a defect in the service itself, not in the modules
that call it: the service exists so modules do not have to get this locking right, and
this entry point still touches a shared field outside the lock it advertises.

Suggested fix

Move the test inside the critical section, leaving the two non-racy pointer checks where
they are:

gboolean dt_preview_data_is_fresh(dt_preview_data_t *pd)
{
  if(!pd || !pd->module) return FALSE;

  const dt_iop_module_t *const module = pd->module;
  dt_iop_gui_enter_critical_section((dt_iop_module_t *)module);

  // No buffer yet, no value stored, or the stored data has been invalidated.
  const dt_hash_t stored_hash = pd->hash;
  gboolean fresh = pd->buf && (stored_hash != DT_INVALID_HASH);
  ...

The cost is taking an uncontended mutex on the path where no buffer exists yet. That path
is gui_focus() on a module the user just opened, which then issues a full preview
reprocess, so the lock is not what it will be waiting for.

Deleting the test instead is not equivalent. It looks redundant, because pd->hash
stays DT_INVALID_HASH until something stores data and dt_preview_data_store() commits
a hash only inside if(can_fill && pd->buf) (:82), so for both of today's callers
hash != DT_INVALID_HASH already implies a non-NULL buffer. But
dt_preview_data_set_hash() (:131), the two-step form's commit, does not look at
pd->buf at all, so a caller that ignored a NULL return from dt_preview_data_resize()
and committed anyway would leave a valid hash beside a NULL buffer. No caller does that
today - toneequal bails out on the NULL return first (toneequal.c:1087-1091) - so
moving the test preserves the current answer in every case, while deleting it trades a
one-line race for an assumption about callers that do not exist yet.

Fixed on 2026-09-01: the node walk

Kept for the record, since it is what the issue was filed about.

dt_preview_data_is_fresh() traversed module->dev->preview_pipe->nodes and dereferenced
a dt_dev_pixelpipe_iop_t from it while holding only the module's gui_lock. The list
and its pieces are freed under pipe->busy_mutex and rebuilt under dev->history_mutex,
by a pipe worker thread. gui_lock is neither, so nothing stopped the list being freed
mid-walk: dt_dev_pixelpipe_cleanup_nodes() (pixelpipe_hb.c:458, busy_mutex at
:466) frees every piece and the list, reached from dt_dev_pixelpipe_change()
(:884, cleanup_nodes() at :917) on a control worker thread by way of
dt_dev_process_image_job() (develop.c:670, the change() call at :808) and
dt_dev_process_preview_job_run() (control/jobs/develop_jobs.c:22-26).

e3d1b4fdb9 ("preview_data: guard the pipe-nodes walk in dt_preview_data_is_fresh with
busy_mutex", Tom Poczos) put the walk behind the pipe's own mutex
(preview_data.c:197), treating a busy pipe as "not fresh". All three rebuild phases hold
that mutex - cleanup_nodes() at :466-498, create_nodes() at :518-556,
synch_all() at :768-856 - so the walk can no longer overlap one.

It has to be a trylock, and the comment at preview_data.c:185-196 records why:
process() runs under busy_mutex for the whole pipe run and can itself call
dt_preview_data_store(), which takes the same module's gui_lock. is_fresh() already
holds gui_lock at that point, so a blocking lock would be the opposite order and would
AB-BA deadlock against it.

The commit message reports the original as reproduced: a segfault dereferencing a freed
piece->module in _dev_pixelpipe_cache_basichash(), from a module's
preview-pipe-finished GUI callback. It carries no Fixes #22066 line, which is why this
issue stayed open.

None of the three options this issue originally suggested were taken, and they are
moot now. For the record they were: record the piece hash at store time and drop the walk
(an API change); take dev->history_mutex around the walk; or document is_fresh() as
requiring the caller to hold a pipe guarantee.

Reachability

Unchanged. The GTK-side callers are module GUI handlers: colorequal.c:2360 in
gui_focus() and :2774 in mouse_moved(). A history change, a module reorder, an
instance add/delete or an image switch all trigger a pipe rebuild while the pointer is
over the canvas.

Corrections to the version filed as #22066

• The node walk, which was the issue's headline and its "Impact" section
  (use-after-free), is fixed. That section is gone; the history is under Fixed on
  2026-09-01 above.
• Line numbers refreshed against f98cabfa21. The filed report cites the function as
  preview_data.c:169-208 (now :169-224, the trylock and its comment account for the
  growth), cleanup_nodes() at pixelpipe_hb.c:455-480 (now :458-499, with
  busy_mutex at :466), dt_dev_pixelpipe_change() at :824 (now :884),
  dt_dev_process_image_job() at develop.c:784 (now :670, with the change() call at
  :808), and the colorequal callers at :2368 / :2782 (now :2360 / :2774).
• The remaining defect's practical cost is stated more precisely than in the filed
  version: the early return can only yield a wrong FALSE, and both callers recover from
  it through their existing reprocess path.
• The suggested fix for it is spelled out, together with why deleting the test outright is
  not the same change.

Environment

Present in the tree when filed; re-verified against f98cabfa21 on 2026-09-10, where
src/ matches upstream master. Static analysis only - nothing verified at runtime.
The reproduction quoted for the fixed half is the one reported in e3d1b4fdb9's commit
message, not one performed here.

Start in src/develop/preview_data.c at dt_preview_data_is_fresh(), especially the early pd->buf check around line 171, and compare it with the locked writes in dt_preview_data_store() and dt_preview_data_resize(). Review the colorequal.c callers and run a sanitizer-enabled check if available. Done means the shared buffer read is synchronized without changing the existing freshness behavior.