colorequal: scrolled() maps the cursor with preview dimensions read outside the lock

Found by coding agents via static analysis, not verified by executing code. 2026-09-12.
Validated against commit c20f5e3566, branch dev-doc/gui-data-sharing-pr, in
/workspace/dev-doc-update.

Summary

scrolled() in src/iop/colorequal.c turns the cursor position into a buffer pixel
using g->pd.width and g->pd.height read without the module GUI lock
(colorequal.c:3187-3190), and only then reads the pixel through
dt_preview_data_get(), which takes that lock itself
(src/develop/preview_data.c:150). A preview pipe run can replace the buffer and its
dimensions in between (preview_data.c:66-75, under the same lock), so the coordinates
belong to the old size while the read uses the new one.

The read itself is safe: the accessor re-checks the coordinates against the current
dimensions under the lock (preview_data.c:156), so nothing is read out of range. The
consequence is a wrong pixel, hence a wrong hue, hence the scroll adjusting the wrong hue
band of the active channel and writing that edit into history
(_adjust_params_gaussian(), colorequal.c:3107-3136). If the stale coordinates fall
outside the new size instead, the accessor returns FALSE, scrolled() returns 0, and the
scroll zooms the image rather than adjusting anything.

The fix is to do the mapping and the read by hand inside one critical section, as
mouse_moved() in the same file already does (colorequal.c:2727-2738), and to correct
the "race-safe" comment at colorequal.c:3181.

Priority: medium (damage, rare) — see Priority below.

Details

The defect site

src/iop/colorequal.c, scrolled() (function starts at :3159):

3181:  // Read the hue directly from the cached preview buffer (race-safe).
...
3185:  float hue_rad = 0.f;
3186:  gboolean have_hue = FALSE;
3187:  if(g->pd.buf && g->pd.width > 0 && g->pd.height > 0)          // no lock held
3188:  {
3189:    const int cx = CLAMP((int)(x * g->pd.width),  0, (int)g->pd.width  - 1);
3190:    const int cy = CLAMP((int)(y * g->pd.height), 0, (int)g->pd.height - 1);
3191:    have_hue = dt_preview_data_get(&g->pd, cx, cy, 0, &hue_rad);  // takes the lock
3192:  }
3193:  if(!have_hue) return 0;

x and y are normalized image coordinates, so cx and cy are only meaningful
together with the dimensions they were computed from.

The writer

dt_preview_data_store(), src/develop/preview_data.c:50, runs on a pipe worker thread.
It takes the module GUI lock at :62 and, when the size changed, frees the old buffer,
allocates the new one and publishes the new dimensions together:

66:  if(pd->width != width || pd->height != height)
67:  {
68:    float *const new_buf = dt_alloc_align_float(nelems);
69:    if(new_buf)
70:    {
71:      dt_free_align(pd->buf);
72:      pd->buf = new_buf;
73:      pd->width = width;
74:      pd->height = height;

colorequal calls it from process() (colorequal.c:1138) and process_cl()
(:1653), both only for the preview pipe. scrolled() runs on the GTK thread.

The reader

dt_preview_data_get(), preview_data.c:142-167, locks at :150, checks
x < pd->width && y < pd->height && comp < pd->components at :156 and indexes with the
current width at :158. g->pd.components is 3 for this module
(colorequal.c:3741), so component 0 is the hue in radians.

So the sequence is: coordinates computed against the dimensions of fill N, value read
from fill N+1. Both are valid buffers; the coordinates simply do not point at the pixel
under the cursor any more.

Separately, the reads at :3187-3190 are unsynchronized reads of fields another thread
writes under a lock. On the platforms darktable targets these are ordinary aligned loads
and will not tear, but the mapping problem above stands regardless.

What the user sees

The hue taken from the wrong pixel goes to _adjust_params_gaussian()
(colorequal.c:3212), which moves every node of the active channel within the Gaussian
window around that hue (the loop at :3115) and then calls dt_dev_add_history_item()
(:3136). The user scrolls over one color and a different color band changes. The change
is visible at once (the sliders move) and can be undone, but it is a history item the
user did not ask for.

Trigger

The window is the few instructions between :3190 and the lock taken inside the accessor
at preview_data.c:150, and it needs a preview pipe run that changes the preview size to
commit exactly there: resizing the darkroom window, dragging a side panel, changing zoom
or switching image, while the user is scrolling over the image.

The documentation already states the rule

dev-doc/GUI_Threading.md:788-798 says the mapping and the read need a single hold of the
lock, names colorequal's mouse_moved() as the example, and states the exact
consequence of not doing it: "dt_preview_data_get() re-checks its coordinates against
the current size under its own lock, so dimensions read earlier cannot make it read out
of range, but after a resize they make it read the wrong pixel." No documentation issue
is filed with this report; the page describes the requirement correctly.

Suggested fix

Do the mapping and the read in one critical section, without the accessor. The lock is
not recursive and dt_preview_data_get() takes it itself, so it cannot be called from
inside the section. mouse_moved() in the same file has the shape
(colorequal.c:2727-2738):

  float hue_rad = 0.f;
  gboolean have_hue = FALSE;
  dt_iop_gui_enter_critical_section(self);
  const float *buf     = g->pd.buf;
  const int    bwidth  = g->pd.width;
  const int    bheight = g->pd.height;
  if(buf != NULL && bwidth > 0 && bheight > 0)
  {
    const int cx = CLAMP((int)(x * bwidth),  0, bwidth  - 1);
    const int cy = CLAMP((int)(y * bheight), 0, bheight - 1);
    hue_rad = buf[3 * ((size_t)cy * bwidth + cx)];
    have_hue = TRUE;
  }
  dt_iop_gui_leave_critical_section(self);

Nothing inside the section takes another lock, so this does not create a lock-order
problem. The comment at colorequal.c:3181 calling the current code "race-safe" becomes
untrue with or without this change and should be corrected or dropped.

Snapshotting the dimensions under the lock and then calling dt_preview_data_get() after
releasing it is not a fix: it removes the unsynchronized field reads but leaves the same
stale mapping.

Not proposed here: a preview_data.c accessor that takes normalized coordinates and does
the mapping under its own lock. It would fix every caller of this shape at once, but it
adds public API to a framework service, which is a decision for the maintainers rather
than part of this fix.

Priority

Medium, consequence damage, but rare.

• damage: the scroll writes an edit to history that the user did not request
  (dt_dev_add_history_item() at colorequal.c:3136), which is the cell's "wrong edit
  data saved to history".
• rare: it needs a preview resize to commit inside a window of a few instructions on
  the GTK thread.

A reader who weighs the fact that the wrong edit is immediately visible and undoable
could rate it malfunction / rare instead, which is minor. The damage row is used here
because history, and through it the XMP and the database, does receive the wrong value.

Related reports

• #22132 (colorequal-cursor-indicator-clamps-outside-the-lock) is the same unlocked
  mapping in gui_post_expose() of the same file, where the read is done by hand with no
  bounds re-check and therefore over-reads the buffer. Different function, different
  consequence, separate fix; its Additional evidence section points at this report.
• #22068 proposes dt_preview_data_get() for toneequal's readers. The mapping
  question raised here applies to any caller that computes coordinates from pd.width /
  pd.height before calling the accessor.
• #22066 (dt_preview_data_is_fresh() walking live pipe nodes) concerns the same
  service but a different function.

Environment

src/iop/colorequal.c and src/develop/preview_data.c read at commit c20f5e3566,
branch dev-doc/gui-data-sharing-pr. Static analysis only; nothing reproduced at
runtime, and no build was run for this report.

Start in src/iop/colorequal.c at scrolled() and compare its preview read with mouse_moved() around lines 2727-2738. Review the locking and buffer replacement in src/develop/preview_data.c, then confirm the cursor mapping and pixel read share the required critical section and the race-safe comment is corrected.