colorequal: the cursor indicator clamps against preview dimensions read outside the lock

Found by code analysis, 2026-09-03; no known crash reported.

Severity: medium. Heap over-read while drawing the darkroom overlay: garbage in the
cursor indicator at best, a crash if the memory past the allocation is not mapped.

Summary

gui_post_expose() in src/iop/colorequal.c draws a small indicator showing the input
and output color under the cursor. It reads the pixel from g->pd, the module's shared
preview buffer, which pipe worker threads refill and resize. The pixel read is inside
the module GUI lock, but the two CLAMP calls that compute the pixel coordinates are
outside it. Between the clamp and the read, a preview pipe run can free the buffer,
allocate a smaller one and store the new dimensions, all under the same lock. The read
then combines a row index clamped to the old height with the current width, and
indexes past the end of the new, smaller buffer.

Nothing here is a use-after-free: the pointer is re-read under the lock, so it is always
the live buffer. The defect is that the index does not belong to it.

The over-read is bounded by the size difference. Shrinking the preview from old_h to
new_h rows at unchanged width puts the worst index roughly (old_h - new_h) * new_w
pixels — three times that many floats — past the end.

Evidence

src/iop/colorequal.c, gui_post_expose() (function starts at :2828):

2882:  if(g->pd.buf && g->pd.width > 0 && g->pd.height > 0 && g->gamut_LUT)   // unlocked
2883:  {
2884:    const int p_cx = CLAMP((int)(g->cursor_pos_x * g->pd.width),  0, (int)g->pd.width  - 1);
2885:    const int p_cy = CLAMP((int)(g->cursor_pos_y * g->pd.height), 0, (int)g->pd.height - 1);
2886:
2887:    // Read the 3 HSB components under one lock, like mouse_moved does.
2888:    float hue_in = 0.f, sat_in = 0.f, bright_in = 0.f;
2889:    gboolean have_hsb = FALSE;
2890:    dt_iop_gui_enter_critical_section(self);
2891:    const float *buf = g->pd.buf;
2892:    if(buf)
2893:    {
2894:      const size_t idx = (size_t)p_cy * g->pd.width + p_cx;   // stale p_cy, current width
2895:      hue_in    = buf[3 * idx + 0];
2896:      sat_in    = buf[3 * idx + 1];
2897:      bright_in = buf[3 * idx + 2];

The if(buf) at :2892 re-establishes that the pointer is live, and nothing more. The
dimensions p_cx and p_cy were clamped against are whatever the pipe had published
when line :2884 ran.

The writer is dt_preview_data_store(), src/develop/preview_data.c:50. 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:

62:  dt_iop_gui_enter_critical_section((dt_iop_module_t *)pd->module);
...
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() (:1138) and process_cl() (:1653), both on
pipe worker threads, while gui_post_expose() runs on the GTK thread. g->pd.components
is 3 (:3741), so each pixel is the three HSB floats read at :2895-2897.

Trigger. Any preview-ROI change while the cursor indicator is on screen: resizing the
darkroom window, dragging a side panel, zooming, or switching image.

Why this is not a judgement call

The same file performs the same read correctly twice, and the buggy site's own comment
names the pattern it then does not follow.

• mouse_moved() :2727-2738 does the identical lookup with the clamp inside the
  critical section:

  2727:  dt_iop_gui_enter_critical_section(self);
  2728:  const float *buf    = g->pd.buf;
  2729:  const int    bwidth  = g->pd.width;
  2730:  const int    bheight = g->pd.height;
  2731:  if(buf != NULL && bwidth > 0 && bheight > 0)
  2732:  {
  2733:    const int cx = CLAMP((int)(pzx * bwidth),  0, bwidth  - 1);
  2734:    const int cy = CLAMP((int)(pzy * bheight), 0, bheight - 1);
  2735:    hue_rad = buf[3 * ((size_t)cy * bwidth + cx)];
  

• scrolled() :3191 goes through dt_preview_data_get()
  (src/develop/preview_data.c:142-167), whose bounds check and read are both under the
  lock taken at :150, with the reason stated in the source at :154-155: "The bounds
  check and the buffer read must both happen under the GUI lock: the pipe thread may
  resize pd->buf between the two."

• gui_post_expose()'s own comment at :2887 says "Read the 3 HSB components under one
  lock, like mouse_moved does."

Suggested fix

Either move the two CLAMP lines into the critical section that is already there,
reading g->pd.width / g->pd.height into locals beside buf as mouse_moved() does,
or replace :2884-2897 with three dt_preview_data_get() calls, one per component. The
second is shorter and reuses the helper written for exactly this, at the cost of taking
the lock three times.

Note that dt_preview_data_get() takes the module GUI lock itself and the lock is not
recursive, so the calls must be outside any critical section — see #22068, where the
same helper is proposed for a caller that already holds it.

Related reports

• #22064 and #22066 are the other colorequal / preview_data reports:
  mask_mode toggles shared without locking, and dt_preview_data_is_fresh() walking
  live pipe nodes. Neither covers this read.
• #21916 (zonesystem) is the same shape in another module: a size read outside the
  lock used against a buffer allocated under it.

Lower severity, same field, worth mentioning in passing rather than filing:
dt_preview_data_is_fresh() tests pd->buf outside the lock at preview_data.c:171,
and _area_scrolled_callback() tests g->pd.buf == NULL outside it at
colorequal.c:3428. Neither dereferences the pointer it tests, so neither can fault.

Environment

Verified line by line against 3c73bf2aaa (branch dt-lockcheck; src/ is identical to
4d9e40f30e, the base the validation used).
Static analysis only — nothing reproduced at runtime.

Start in src/iop/colorequal.c at gui_post_expose() and compare its preview lookup with mouse_moved(); then read dt_preview_data_store() in src/develop/preview_data.c. Done means the coordinate bounds and buffer read use one consistent, lock-protected set of preview dimensions, without holding the lock around dt_preview_data_get() calls.