`toneequal`: GUI readers test `luminance_valid` outside the lock, then read the buffer

Found: 2026-08-27, by Codex in the 05-50 review round of dev-doc PR 21912; verified
against the tree before filing.
Tree: dev-doc/gui-data-sharing-pr; the code is unchanged from kofa/master.
File: src/iop/toneequal.c. No source file was edited.

Updated 2026-09-03 during the discipline_gap validation. Two substantive changes:
the helper has a third caller, scrolled(), which the original missed, and the
suggested fix deadlocks if applied at that caller as written. Line numbers refreshed
against 3c73bf2aaa. The material that is new relative to what was filed is drafted as
a comment in comment-22068-update.md, not yet posted.

Summary

The module implements the producer half of the validity-flag protocol correctly and the
consumer half not at all. Three GTK-thread readers test g->luminance_valid, then read
the preview buffer's pointer and dimensions in a different critical section, or in none
at all. The preview pipe can clear the flag, reallocate that buffer and refill it in the
meantime.

Producer side — correct

src/iop/toneequal.c:1119-1139, on the preview branch: clears histogram_valid and
luminance_valid inside a critical section, runs compute_luminance_mask() outside it,
then commits the hash and sets luminance_valid inside another one. The in-source
comment states the intent: "Flag the cache as being recomputed so the GUI threads never
read a partially filled buffer".

_toneeq_preview_resized() (:640-647) is the dt_preview_data_resize() callback and
clears luminance_valid while the service still holds the lock, so the flag drops
atomically with a reallocation.

Consumer side — not

The shared helper is _luminance_from_module_buffer() (:730-748), which reads the
service's fields directly rather than through the locked accessor (:743-747):

  return get_luminance_from_buffer(g->pd.buf,
                                   g->pd.width,
                                   g->pd.height,
                                   b_x,
                                   b_y);

Pointer, width and height are read as a tuple, and used, with whatever lock state the
caller happens to be in. dt_preview_data_get() exists precisely to do this under
gui_lock, and is bypassed by all three callers.

mouse_moved() (:2060-2061) — tests and reads after
dt_iop_gui_leave_critical_section() at :2057:

  if(g->cursor_valid && !dt_pipe_processing(dev->full.pipe) && g->luminance_valid)
    g->cursor_exposure = log2f(_luminance_from_module_buffer(self));

gui_post_expose() (:2275-2276) — the same shape, also outside any section.

scrolled() (:2183) — the one caller that holds the lock across the helper. It
tests the flag inside one critical section (:2169-2178, !g->luminance_valid at
:2172), releases it at :2178, then opens a second section at :2182 for the read:

2178:  dt_iop_gui_leave_critical_section(self);
2179:  if(fail) return 1;
2180:
2181:  // re-read the exposure in case it has changed
2182:  dt_iop_gui_enter_critical_section(self);
2183:  g->cursor_exposure = log2f(_luminance_from_module_buffer(self));
2184:
2185:  dt_iop_gui_leave_critical_section(self);

So this caller has the report's defect in its milder form only: the buffer read itself is
under the same lock the writer takes, so it cannot use freed memory, but the validity
test is stranded by the release at :2178 and the readout can still be of a
half-recomputed buffer. It matters for the fix, not for the impact — see below.

Why the dt_pipe_processing() test does not close it

All three readers gate on dt_pipe_processing(dev->full.pipe) — the full pipe
(:2060, :2175, :2275). The buffer in question is filled by the preview pipe,
which is an independent worker (dt_dev_process_preview_job_run(),
src/control/jobs/develop_jobs.c:22-26). A busy full pipe is neither necessary nor
sufficient for the preview pipe to be idle.

Impact

dt_preview_data_resize() calls dt_free_align(pd->buf) and reallocates when the
preview dimensions change (src/develop/preview_data.c:93-129, free at :112), so for
the two unlocked readers this is a use-after-free, not only a stale or torn readout.
Where the buffer is merely refilled in place, the consequence is a wrong exposure
readout under the cursor; that is also all scrolled() can suffer.

Suggested fix

Two steps, in this order. The second one alone deadlocks.

1. Hoist the helper call out of scrolled()'s critical section, so that all three
callers reach it with no lock held:

  // re-read the exposure in case it has changed
  const float lum = _luminance_from_module_buffer(self);

  dt_iop_gui_enter_critical_section(self);
  g->cursor_exposure = log2f(lum);
  dt_iop_gui_leave_critical_section(self);

This is separately required: holding gui_lock across the helper is the GTK half of a
lock-order inversion against the pixelpipe, filed as #22133. The helper reaches
dt_dev_distort_backtransform_plus(), which takes dev->history_mutex
(src/develop/develop.c:4008), while a pipe worker holds history_mutex and waits for
this module's gui_lock in commit_params().

2. Then route the buffer read through dt_preview_data_get(), which takes the lock
and bounds-checks (x, y, comp) — that is what the accessor is for. If the validity flag
must also be consulted, test it and read in the same critical section, or fold the flag
into the service's own freshness state.

Do not apply step 2 first. dt_preview_data_get() takes the same module gui_lock
(src/develop/preview_data.c:150), and that mutex is not recursive: it is initialised
with a NULL attribute at src/develop/imageop.c:1441, so it has the platform default
type. Called from :2183 as the code stands, the accessor would relock a mutex the same
thread already holds and hang on the first scroll over the image, with no second thread
involved.

Related reports

• #22133 (toneequal: scrolled() takes gui_lock then history_mutex) is the
  deadlock behind step 1 above. The hoist is the whole of that report's fix and the
  prerequisite for this one's; the two share one change.
• #22091 (toneequal: unlocked pipe_order test strands luminance_valid) is the
  producer-side counterpart in the same cache protocol.
• #22121 (toneequal: curve cache read unlocked after refresh) establishes the
  pipe-side half of the lock chain — commit_params() taking gui_lock from a pipe
  worker.

Environment

Line numbers verified against 3c73bf2aaa (branch dt-lockcheck; src/ identical to
4d9e40f30e, the base the discipline_gap validation used). Static analysis only;
nothing here was reproduced at runtime.

Start in src/iop/toneequal.c with _luminance_from_module_buffer() and its mouse_moved(), scrolled(), and gui_post_expose() callers, then read dt_preview_data_get() in src/develop/preview_data.c. Review the lock interactions with dt_dev_process_preview_job_run() in src/control/jobs/develop_jobs.c and the history mutex in src/develop/develop.c. Done means all validity checks and buffer reads are safe across resize and refill, without introducing the described deadlock.