ashift reads its buffer geometry on the GTK thread without the lock the pipe writes it under

Is there an existing issue for this?

• I checked and did not find my issue in the already reported ones

Describe the bug

Found by Claude + Codex via code analysis.

Description

ashift keeps a copy of the preview image in g->buf, together with the size and offset
of that buffer (buf_width, buf_height, buf_x_off, buf_y_off) and a flag saying
whether the image is rotated by 90 degrees (isflipped). The pixelpipe replaces the
buffer and updates all of these inside a critical section, and so does the GTK-side reset
in reload_defaults(). Several other GTK-thread readers take no lock at all: they read
two or four of the fields one after another, so a preview run landing between the reads
gives them a mixture of old and new values. Nothing here is memory-unsafe — the unlocked
readers only test whether g->buf is non-NULL and read the numbers, and the two places
that actually copy pixels out of the buffer do hold the lock. The effect is incoherent
geometry: the crop outline drawn over the image can be misplaced for a frame, and, less
often, an automatic crop computed from mismatched width and height can be written into the
module parameters, where it stays until the user changes it.

Evidence

All code is in src/iop/ashift.c.

The pipe writes the buffer and its geometry together, under the lock, in process():

    dt_iop_gui_enter_critical_section(self);
    g->isflipped = isflipped;
    ...
    if(g->buf /* && hash != g->buf_hash */)
    {
      dt_iop_image_copy_by_size(g->buf, ivoid, roi_in->width, roi_in->height, ch);
      g->buf_width = roi_in->width;
      g->buf_height = roi_in->height;
      g->buf_x_off = roi_in->x;
      g->buf_y_off = roi_in->y;
      ...
    }
    dt_iop_gui_leave_critical_section(self);

process_cl() has the same shape.

GTK-thread readers with no lock:

static void do_crop(const dt_iop_module_t *self, dt_iop_ashift_params_t *p)
{
  ...
  if(g->buf_width == 0 || g->buf_height == 0) return;
  ...
  cropfit.width = g->buf_width;
  cropfit.height = g->buf_height;

static void crop_adjust(const dt_iop_module_t *self, ...)
{
  const float wd = g->buf_width;
  const float ht = g->buf_height;

void gui_post_expose(dt_iop_module_t *self, ...)
{
  if(g->buf && self->enabled && dt_iop_has_focus(self))
  {
    const float iwd = g->buf_width;
    const float iht = g->buf_height;
    const float ixo = g->buf_x_off;
    const float iyo = g->buf_y_off;

gui_changed() reads g->buf_height and g->buf_width the same way.

isflipped is read under the lock in one place, _event_draw():

  dt_iop_gui_enter_critical_section(self);
  const int isflipped = g->isflipped;
  dt_iop_gui_leave_critical_section(self);

and without it in nmsfit(), model_probe(), _update_colors() and _get_points(),
for example:

  const gboolean isflipped = g->isflipped;

For contrast, the places that read the pixels do it correctly — _get_structure() copies
the buffer and its geometry inside one section:

  dt_iop_gui_enter_critical_section(self);
  if(g->buf != NULL)
  {
    width = g->buf_width;
    height = g->buf_height;
    x_off = g->buf_x_off;
    y_off = g->buf_y_off;
    ...
      dt_iop_image_copy_by_size(buffer, g->buf, width, height, 4);
  }
  dt_iop_gui_leave_critical_section(self);

One detail worth noting: isflipped uses -1 to mean "not known yet"
(reload_defaults() and gui_init() set it). _event_draw() tests for -1 explicitly,
but the unlocked readers use the field directly in conditions such as
g->isflipped ? ... : ..., where -1 counts as true. Whether those readers can actually
be reached in the -1 state has not been traced.

Suggested fix

Give the GTK-side readers the same treatment the correct call sites already use: take the
critical section, copy the fields into local variables, release it, and work from the
locals. For example in gui_post_expose():

  dt_iop_gui_enter_critical_section(self);
  const gboolean have_buf = g->buf != NULL;
  const float iwd = g->buf_width;
  const float iht = g->buf_height;
  const float ixo = g->buf_x_off;
  const float iyo = g->buf_y_off;
  dt_iop_gui_leave_critical_section(self);

  if(have_buf && self->enabled && dt_iop_has_focus(self))
  {
    ...

The same applies to do_crop(), crop_adjust() and gui_changed() for the geometry, and
to the four unlocked isflipped readers. The sections stay short, and no work is done
while holding the lock.

Steps to reproduce

No known report; derived from reading the code, not carried out. The window is a few instructions wide, so
the visible symptom may not appear at all; a build with ThreadSanitizer is the dependable
way to see the problem, because it reports the unsynchronised access itself.

1. Build with ThreadSanitizer, if possible.
2. Open an image in the darkroom and give perspective correction the focus, so that
   gui_post_expose() draws the crop outline.
3. Set automatic cropping to something other than "off".
4. Drag the window edge, or the divider of the side panel, continuously, and move the
   mouse over the centre view at the same time. This makes the preview pipe run over and
   over with different buffer sizes while the widget is being redrawn.
5. ThreadSanitizer reports the race on buf_width and the fields next to it. Without it,
   watch for the crop outline sitting in the wrong place for one frame.

Note what does not help: a slower preview pipe, from a large image or from disabling
OpenCL, reaches the write less often per second, so it lowers the chance rather than
raising it. A run that writes the same size as before also produces no visible mismatch,
which is why the dimensions have to keep changing.

Expected behavior

No response

Logfile | Screenshot | Screencast

No response

Commit

No response

Where did you obtain darktable from?

self compiled

darktable version

6783f7c4

What OS are you using?

Linux

What is the version of your OS?

Ubuntu 26.04

Describe your system

No response

Are you using OpenCL GPU in darktable?

None

If yes, what is the GPU card and driver?

No response

Please provide additional context if applicable. You can attach files too, but might need to rename to .txt or .zip

No response