colormapping tests its preview buffer outside the critical section that protects it

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, no known crash reported. Low severity: the
consequence needs a failed memory allocation.

Description

colormapping keeps a copy of the preview image in g->buffer, written by the pixelpipe
and read on the GTK thread by process_clusters() when the preview pipe reports that it
has finished. process_clusters() checks that g->buffer is not NULL, then takes the
critical section and copies from it; the check is one line outside the section and is not
repeated inside. The pipe frees and re-allocates the buffer inside that same section, and
leaves the field NULL if the allocation fails, so the value can change between the check
and the copy. This is worth fixing for consistency rather than for the crash: the module
tests the result of dt_iop_image_alloc() in the three other places it uses it, and so
does every other caller of that function in src/iop, which makes this copy the one place
that assumes the allocation cannot fail. In practice a preview-sized allocation rarely
fails, and a system short of that much memory has other problems.

Evidence

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

The pipe side, in process(), frees the old buffer, allocates a new one and copies into
it, all under the lock. The copy is guarded, so a failed allocation leaves g->buffer
NULL and is otherwise handled:

    dt_iop_gui_enter_critical_section(self);
    if(g->buffer) dt_free_align(g->buffer);

    g->buffer = dt_iop_image_alloc(width, height, 4);
    g->width = width;
    g->height = height;
    g->ch = 4;

    if(g->buffer) dt_iop_image_copy_by_size(g->buffer, in, width, height, 4);

    dt_iop_gui_leave_critical_section(self);

process_cl() has the same shape.

The GTK side, in process_clusters(), tests the pointer before the section and uses it
inside without testing again:

  if(!g || !g->buffer) return;            // test, outside the lock
  if(!(p->flag & ACQUIRE)) return;
  ...
  dt_iop_gui_enter_critical_section(self);
  const int width = g->width;
  const int height = g->height;
  const int ch = g->ch;
  float *const restrict buffer = dt_iop_image_alloc(width, height, ch);
  if(!buffer)                             // this allocation is checked
  {
    dt_iop_gui_leave_critical_section(self);
    DT_LEAVE_GUI_UPDATE();
    return;
  }
  dt_iop_image_copy_by_size(buffer, g->buffer, width, height, ch);   // source not tested
  dt_iop_gui_leave_critical_section(self);

process_clusters() is connected to the preview-pipe-finished signal:

  DT_CONTROL_SIGNAL_HANDLE(DT_SIGNAL_DEVELOP_PREVIEW_PIPE_FINISHED, process_clusters);

dt_iop_image_copy_by_size() (src/common/imagebuf.h) passes its source pointer straight
to dt_iop_image_copy(), so a NULL source is dereferenced rather than caught.

The other fields the copy uses — g->width, g->height, g->ch — are read inside the
section together with the pointer, so they stay consistent with each other. The pointer
test is the only part that escapes.

Suggested fix

Move the NULL test inside the critical section, next to the fields it belongs with:

  dt_iop_gui_enter_critical_section(self);
  if(!g->buffer)
  {
    dt_iop_gui_leave_critical_section(self);
    DT_LEAVE_GUI_UPDATE();
    return;
  }
  const int width = g->width;
  ...

The early if(!g || !g->buffer) return; before the section can stay as a cheap first
check; it just must not be the only one.

Steps to reproduce

Derived from reading the code, not carried out. The bug needs a failing allocation, so
normal use will not trigger it:

1. Open an image in the darkroom and enable color mapping.
2. Press "acquire as source". This sets the ACQUIRE flag, so the next preview run copies
   the preview image into g->buffer.
3. Arrange for dt_iop_image_alloc() in process() to return NULL on that run — for
   example under a debugger, or with an allocation-failure injection tool.
4. When the preview pipe finishes, process_clusters() reaches the copy with g->buffer
   NULL.

Without step 3 the code takes the same path with a valid pointer and nothing visible
happens.

Expected behavior

No response

Logfile | Screenshot | Screencast

No response

Commit

No response

Where did you obtain darktable from?

darktable.org / GitHub release

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