IOP mask display toggles shared without locking

IOP modules share mask-display GUI state with pipe threads without any locking

Found by code analysis (Claude + Codex, cross-checked) during the gui_data threading
audit, 2026-08-19. No known crash.

Description

A widget callback on the GTK main thread writes GUI state that controls a module's mask
display — the toggle that makes the module show its internal mask instead of the image,
and in one case the mask channel that toggle selects — and process() / process_cl()
read that same field on a pixelpipe worker thread. Nothing synchronises the two: these
modules never call dt_iop_gui_enter_critical_section() for the field, and most never
call it at all.

All nine instances are the same shape and take the same fix, which is why they are
reported together rather than one issue per module. They are independent of each other
and can be fixed one at a time.

Affected modules

module	field	direction
colorequal.c	mask_mode	callback writes, pipe reads
filmicrgb.c	show_mask	callback writes, pipe reads
colorbalancergb.c	mask_display, mask_type	callback writes, pipe reads
colorzones.c	display_mask	callback writes, pipe and commit_params() read
colorzones.c	channel	callback writes, pipe reads
contrastntexture.c	details_display	callback writes, pipe reads
demosaic.c	dual_mask, cs_mask, cs_boost_mask	callback writes, pipe reads
highlights.c	hlr_mask_mode	callback writes, pipe and commit_params() read
lens.cc	vig_masking	_visualize_callback() writes, gui_focus() reads then clears, process() / process_cl() read
toneequal.c	mask_display	callback writes, pipe reads; some other accesses to the same field are already locked

lens.cc and toneequal.c are the same defect as the rest; they are called out
separately only because both modules do use the mutex elsewhere — lens.cc in six other
places, toneequal.c even for two other accesses to this very field — so a "does this
module lock at all?" scan passes over them. (lens.cc is also the file a src/iop/*.c
glob misses.)

colorzones.c and highlights.c are read from commit_params() as well, which also
runs on a pipe thread — so a fix confined to process() is incomplete for those two.

Evidence

colorequal.c — seven GTK-thread sites write g->mask_mode: two quad-button callbacks,
the notebook tab-switch callback, gui_changed(), gui_update(), gui_focus() and
reload_defaults(). A fix has to cover all of them, not just the obvious button
callbacks.

// quad-button callback, GTK main thread
g->mask_mode = (dt_bauhaus_widget_get_quad_active(quad)) ? g->channel + 1 : 0;

// process() and process_cl(), pixelpipe worker thread
const int mask_mode = g && fullpipe ? g->mask_mode : 0;

filmicrgb.c — a toggle-button callback writes g->show_mask = !(g->show_mask); and
both process() and process_cl() read it directly, without snapshotting into a local.

colorzones.c — _channel_tabs_switch_callback() writes the selected channel on the GTK
main thread at src/iop/colorzones.c:2301, with no critical section anywhere in the
callback:

// :2301 -- GTK main thread
g->channel = (dt_iop_colorzones_channel_t)page_num;

process() dispatches to process_display() when g->display_mask is set (:585), and
process_display() reads the channel on the pipe thread and uses it to index the LUT:

// :440 -- pixelpipe worker, no lock held
const dt_iop_colorzones_channel_t display_channel = g->channel;
...
// :470
out[3] = fabsf(lookup(d->lut[display_channel], select) - .5f) * 4.f;

Switching tabs while a mask frame is in flight renders the previous or next channel rather
than the tab the GUI shows. The notebook page number stays inside the channel enum, so no
out-of-bounds index is established — the same bounds argument made for colorbalancergb's
opacities[g->mask_type] below. The value is read once into a local here, so the mid-frame
re-read hazard does not apply; the stale-value and formal-race arguments do.

channel has many other unlocked GTK sites — the drawing, motion, scroll and button
callbacks on the curve area, _interpolator_callback() (:2342-2346),
color_picker_apply() (:2399), _action_process_zones() (:2499) and gui_update()
(:2741). Only the two writes in _channel_tabs_switch_callback() (:2301, :2305) race
the pipe, but the fix has to leave the rest consistent — the same "all seven GTK sites"
caution made for colorequal:mask_mode above.

toneequal.c — the full-pipe branch reads g->mask_display at src/iop/toneequal.c:1157
to choose between display_luminance_mask() and ordinary tone-equalizer processing:

// :1155-1163 -- pixelpipe worker, no lock held
if(self->dev->gui_attached && dt_pipe_is_full(piece->pipe))
{
  if(g->mask_display)
  {
    display_luminance_mask(in, luminance, out, roi_in, roi_out);
    piece->pipe->mask_display = DT_DEV_PIXELPIPE_DISPLAY_PASSTHRU;
  }
  else
    apply_toneequalizer(in, luminance, out, roi_in, roi_out, d);
}

GTK writes the same field without the lock in show_luminance_mask_callback() (:1948,
:1952, :1954), gui_update() (:1731) and gui_focus() (:2401-2402). Two other
accesses to this very field, in _develop_ui_pipe_started_callback(), are locked
(:3013-3022); and gui_focus() takes the lock three lines earlier for g->has_focus
(:2394-2396), then reads and clears mask_display outside it. That is an inconsistent
discipline on one field, not protection.

A toggle or focus loss racing an in-flight full-pipe call produces one frame whose mask
state does not match the button. dt_iop_refresh_center() (:1957) and
dt_dev_reprocess_center() request later work; neither excludes the already-running pipe,
which is the argument the "Why dt_dev_reprocess_*() is not a substitute for the lock"
section below makes.

Impact

• Stale value in an already-running pipe. These callbacks write the field and then
  call dt_dev_reprocess_center(), which only sets pipe->changed |= DT_DEV_PIPE_SYNCH,
  invalidates buffers and queues a redraw (src/develop/develop.c). It neither waits for
  a running pipe nor issues a barrier, so a pipe already inside process() can produce a
  frame showing the wrong thing. The run submitted after the write is ordered against
  it by the job queue (see below), so this is the in-flight case, not every frame.
• Mid-frame inconsistency. Several process() implementations read the field more
  than once, so a change landing mid-call can make one half of the frame disagree with
  the other. colorbalancergb.c re-reads g->mask_type once per pixel
  (opacities[g->mask_type]), which is the sharpest instance. Without a lock or an
  atomic the compiler is entitled to re-load the field like that, and to split or
  duplicate accesses generally.

Nothing here corrupts memory in the concrete sense: none of these fields is a pointer,
and every indexed read stays in bounds even with a racing index (opacities is a
float[4] and MASK_NONE == 3). Nor is hardware tearing the concern — they are
naturally aligned int, gboolean and enum on every platform darktable supports
(src/is_supported_platform.h). They are still formally data races, so the standard
promises nothing; the bounds arguments describe what the generated code does today, not
what the language owes you.

The other reason to fix this is that the pattern is being copied into new modules.

Why dt_dev_reprocess_*() is not a substitute for the lock

Stated precisely, so the report is not read as overclaiming: the next pipe run is
ordered against the write. The queued redraw is dispatched on the GTK thread, which calls
dt_dev_process_image() and the preview equivalents; those submit a reserved job with
dt_control_add_job_res(). Publication takes and releases control->res_mutex
(src/control/jobs.c) and the reserved worker acquires the same mutex before executing,
and that release/acquire pair orders the earlier GTK-thread write ahead of the newly
started process(). dt_dev_add_history_item() gives a similar edge through
dev->history_mutex.

Two reasons that is still not a licence to skip the lock. Whether such a route exists at
all is internal framework detail rather than a module-facing contract — the
synchronisation effect of the mutex pair is real, but nothing obliges the framework to
keep routing pipe submission that way. And the edge only covers the run submitted
afterwards: it does nothing for the pipe already in flight, and it does not stop the
compiler re-reading the field mid-process().

Suggested fix

Wrap both the write and the read in dt_iop_gui_enter_critical_section() /
dt_iop_gui_leave_critical_section(). Four cautions:

• The mutex is not recursive. Keep the critical section around the field access only,
  and do not call helpers from inside it that take the same lock.
• Copy the field into a local variable inside the section and use the local afterwards.
  That removes the mid-frame inconsistency and keeps the lock off the per-pixel path in
  colorbalancergb.c.
• On the commit_params() path, guard the section with self->dev->gui_attached && g.
  gui_lock is only initialised by dt_iop_gui_init(), so entering it during export
  locks a mutex that was never set up.
• Take one coherent snapshot for related state. colorzones needs display_mask and
  channel read under the same lock acquisition: locking the two fields independently can
  still pair a toggle value with a channel from a different GUI generation.

Relationship to #21891

Soft dependency. The fix is start taking gui_lock, and #21891 is about that lock being
destroyed under a running pipe. Doing this first is safe, but it adds a
destroyed-mutex lock to modules that currently never touch gui_lock, on a path that
already commits a use-after-free write — a reason to sequence #21891 first, not a blocker.

Related

#21915 (overlay), #21916 (zonesystem), #21917 (colormapping), #21918 (ashift),
#21919 (rgblevels) came out of the same audit. So did the unfiled
hotpixels, atrous, denoiseprofile, retouch, channelmixerrgb and
colorreconstruction reports in this directory.

toneequal.c has two further, independent unlocked-field defects that do not overlap with
mask_display: the luminance cache (#22068, #22091) and the curve-drawing cache
(#22121). A developer fixing
toneequal locking should read all of them together, and must respect the non-recursive
mutex when doing so.

Start with the affected files under src/iop: colorequal.c, filmicrgb.c, colorbalancergb.c, colorzones.c, contrastntexture.c, demosaic.c, highlights.c, lens.cc, and toneequal.c. Trace each listed GTK callback against process(), process_cl(), or commit_params(), then verify every shared mask value is snapshotted under the GUI critical section, with related colorzones state captured coherently. Account for the non-recursive lock and the gui_attached guard on commit_params() paths.