channelmixerrg: the deltaE feedback buffer keeps the size of the previous colour checker

channelmixerrgb: the ΔE feedback buffer keeps the size of the previous colour checker

Found by code analysis during the gui_data threading audit, 2026-08-30, and verified
against the tree before filing. No known crash — the overflow is small and on the heap,
and its first third lands in padding dt_alloc_align_float() added when it rounded the
request up. The remaining two thirds do not: they are past the end of the block. See
Impact for the exact extent.

The primary defect is not a threading defect. It surfaced in a lock-check round
because the field is also shared between threads without locking, but the out-of-bounds
access described first happens on a single thread and no amount of locking fixes it: even
perfectly serialized code reuses a 24-entry allocation after the checker changes to 48
patches.

Two threading defects come with it, and this report owns both:
the buffer's unsynchronised sharing
and the g->checker pointer it is paired with —
whose racing overrun is six times larger than the sequential one this report is named
after.

This report therefore remains separate from
#22058, which shares the
locked-profiling/unlocked-validation path and should be fixed in coordination with it.
Closing #22058 without invalidating or resizing this buffer would leave the heap overflow
intact.

Summary

g->delta_E_in is allocated once, sized from whichever colour checker was selected at
that moment, and never resized. The user can then select a checker with twice as many
patches. Every consumer loops to the current checker's patch count over the old
buffer, so the module reads and writes past the end of the allocation.

The allocation happens once

// src/iop/channelmixerrgb.c:1441-1442, in _extract_patches()
if(g->delta_E_in == NULL)
  g->delta_E_in = dt_alloc_align_float(g->checker->patches);

The guard is == NULL, not a size comparison, and nothing anywhere records the patch
count the buffer was sized for. The buffer is freed in exactly one place, gui_cleanup()
(src/iop/channelmixerrgb.c:4766-4769), and set to NULL once at :4433. Nothing else
in the file frees or re-nulls it — grep -n delta_E_in src/iop/channelmixerrgb.c returns
eleven lines and none of them is a free outside gui_cleanup().

g->delta_E_in is the only patch-count-sized buffer in the module that outlives the
call that allocates it. The other three sites — :1652, :1773-1774 and :1947 — are
function locals, freed at :1937, :1860-1861 and :1979 respectively, so a checker
switch cannot leave any of them stale. That is why this report is about one field. It is
not why the other three are safe: see
the checker pointer, where they fail differently
and worse.

The checker changes underneath it

// src/iop/channelmixerrgb.c:2854, in _checker_changed_callback()
g->checker = dt_get_color_checker(i);

and again in gui_update() at :3818. Neither touches delta_E_in. The critical
section that follows at :2859-2862 clears g->profile_ready and re-inits the bounding
box; it does not resize the buffer.

Patch counts differ by a factor of two. src/common/colorchecker.h declares four
checkers with .patches = 24 (:76, :115, :158, :200) and three with
.patches = 48 (:242, :308, :374). dt_get_color_checker() (colorchecker.h:428)
returns a pointer to a static object, so the checker itself is not freed — only the count
the consumers use changes.

Both consumers use the current count

• Write. _compute_patches_delta_E() fills delta_E[k] for k < checker->patches
  (src/iop/channelmixerrgb.c:1300). Called at :1663, :1769, :1881 and :1954,
  always with g->checker and g->delta_E_in as a pair.
• Read. gui_post_expose() loops for(size_t k = 0; k < g->checker->patches; k++)
  (:2768) and dereferences g->delta_E_in[k] at :2799 and :2805 to draw the ΔE
  diagonals over each patch.

Reproduction

1. Open an image in darkroom, enable color calibration, select a 24-patch checker
   (e.g. the default Xrite ColorChecker 24).
2. Run the colour-checker profiling once — this reaches :1441 and allocates 24 floats.
3. Switch the checker combobox to a 48-patch one (e.g. a SpyderCheckr 48).

gui_post_expose() now reads elements 24..47 of a 24-element buffer on the next redraw,
and the next profiling or validation run writes them. dt_alloc_align_float(24) does not
round up to 48 floats.

Of those 24 elements, 8 land inside the allocator's own rounding and 16 do not.
dt_alloc_aligned() rounds the request to DT_CACHELINE_BYTES
(src/common/darktable.c:2495-2496, dt_round_size() at :2517-2521), so the 96 bytes
asked for become a 128-byte block — 32 floats, while 48 floats need 192 bytes.
Elements 24..31 therefore sit in padding; elements 32..47 are 16 floats / 64 bytes past
the allocation. That holds on every platform: DT_CACHELINE_BYTES is 64 or 128
(src/common/dttypes.h:44,48) and both round 96 to 128, and every branch of
dt_alloc_aligned() hands back exactly aligned_size usable bytes — including the
_DEBUG branch (:2503-2509), whose extra alignment bytes sit before the returned
pointer.

The redraw needs no pipe run of its own: _checker_changed_callback() queues one at
:2864 immediately after swapping the checker, so the next expose performs the
out-of-bounds read.

Impact

Heap out-of-bounds read and write, 64 bytes past the allocation. Undefined behaviour;
in practice most likely silent, because the underlying malloc chunk usually has slack
beyond what darktable asked for, which is why this has not been reported as a crash — it
is the kind of defect ASAN finds and a user does not. The read feeds Cairo drawing
decisions (> 2.3f / > 4.6f thresholds), so garbage values produce spurious or missing
ΔE diagonals in the overlay.

This overrun is hard-bounded at 64 bytes: src/common/colorchecker.h declares seven
checkers and only two patch counts, 24 (:76, :115, :158, :200) and 48 (:242,
:308, :374), and dt_get_color_checker() (:428-457) can return nothing else. The
bound does not extend to the whole report — the racing overrun described under
the checker pointer is 384 bytes.

Severity is judged on the class, not the observed symptom: a write through a heap pointer
past the end of its allocation is the most serious thing this audit round turned up.

Suggested fix

Record the allocated patch count beside the pointer and reallocate at
src/iop/channelmixerrgb.c:1441 when g->checker->patches differs from it. The == NULL
guard is not enough on its own: nothing today stores the size the buffer was made for, so
a fix has to add that, or allocate a flat 48 floats and stop caring.

This fix is wrong on its own. It must land together with the synchronisation fix
below. Reallocating at :1441 means freeing on the pixelpipe thread, while
gui_post_expose() reads the same pointer on the GTK thread with no critical section
anywhere in its body — so a sizing fix shipped alone converts a bounded 64-byte overrun
into a use-after-free. There is none in the tree today: g->delta_E_in is freed in
exactly one place, gui_cleanup().

While touching :1442, note that the allocation is unchecked and the pipe writes through
the result at :1663, :1769, :1881 and :1954 with no NULL test, though
gui_post_expose() does test at :2796.

Rejected: free and NULL next to each g->checker write

The obvious alternative — free and NULL g->delta_E_in at :2854 and :3818 so the
existing == NULL guard reallocates at the right size — is a use-after-free, not
merely a maintenance risk.

:2854 is in _checker_changed_callback(), on the GTK thread, and the function's only
critical section starts at :2859. A free placed next to that write runs while the pipe
can be inside _compute_patches_delta_E() holding the same pointer.

Moving the free inside :2859-2862 does not rescue it, for two independent reasons:

• The validation path takes no critical section at all (:2285-2288), so it races the
  free regardless. Only the profiling path is covered, by :2162-2166.
• An early return at :2857 sits between the checker write and the critical section,
  so on that path the section — and any free inside it — never runs at all, while
  g->checker has already changed. That path also skips g->profile_ready = FALSE,
  leaving a profile solved against the old chart committable through :2934.

The sibling site :3818 (in gui_update()) is inside a critical section
(:3814-3827), so the module is inconsistent with itself here; that does not make the
approach safe, because :2854 is the site a user actually reaches.

Second defect — the same buffer is shared with no synchronisation

Separate, and worth fixing in the same change.

gui_post_expose() takes no critical section anywhere: there is no
dt_iop_gui_enter_critical_section() between its opening at
src/iop/channelmixerrgb.c:2676 and its closing brace at :2832. So the GTK thread reads
delta_E_in while the preview pipe refills it from _compute_patches_delta_E(). The
pipe-side coverage is uneven in the way
#22058 describes: the profiling
path holds the section across _extract_color_checker() (:2162-2166), the validation
path holds nothing (:2285-2288 — the test at :2285, the _validate_color_checker()
call at :2287, the clear at :2288). Because the reader never locks, neither matters —
the field is unsynchronised in every combination.

The pointer is also published before the memory is initialised: :1442 assigns the
result of dt_alloc_align_float(), and the first _compute_patches_delta_E() call is at
:1663. A redraw landing between the two reads uninitialised heap through a non-NULL
pointer that passes the if(g->delta_E_in) test at :2796.

Suggested fix for this half: the validity-flag pattern from dev-doc/GUI_Threading.md —
clear a flag under the lock before the fill, commit contents and flag under the lock
after, and have gui_post_expose() test the flag and use the buffer inside one section;
or copy the array out for the overlay.

g->checker must be fixed in the same change — see the next section, which this report
owns.

The checker pointer is shared too

This report covers g->checker as well as g->delta_E_in. They cannot be separated:
every consumer pairs the buffer with the count, and a fix that synchronises one without
the other still lets a reader pair one with the other's patch count.
#22058 lists five gui_data
fields and checker is not among them; it hands the field here explicitly.

g->checker is written on the GTK thread at :2854 outside any critical section (and at
:3818 inside one) and read from roughly 33 pipe-side sites. Nothing serialises the
first write: DT_GUARD_GUI_UPDATE() at :2849 is an early-out on darktable.gui->reset
(src/common/darktable.h:315-316), not a lock, and the pipe-side section at :2162-2166
is held by one party only.

The consequence is worse than the stale-buffer defect this report is named after, because
the count is re-read after the buffer that depends on it has been sized:

// src/iop/channelmixerrgb.c:1652, in _extract_color_checker()
float *const restrict patches = dt_alloc_align_float(g->checker->patches * 4);
...
// :1445, in _extract_patches(), bounding the writes into that same buffer
for(size_t k = 0; k < g->checker->patches; k++)

A swap from 24 to 48 patches landing between those two reads makes a 384-byte allocation
take 768 bytes of writes — 384 bytes past the end, six times the delta_E_in overrun,
inside a single run, and immune to any amount of correct sizing at allocation time. The
window is not tight: a full-image dt_simd_memcpy() sits between the two reads at
:1654, and the checker combobox is never desensitised while a pipe runs.

The same shape applies to the other patch-count-sized locals — Y and A at
:1773-1774, whose count is re-read at :1776 and again at :1855 when
pseudo_solve_gaussian() is told the row count, and validation's patches at :1947,
whose path holds no lock at all.

A fix therefore has to publish the checker and everything sized from it as one unit:
capture g->checker once per run into a local under the critical section and use only
that local, rather than re-reading the field.

Related

• #22058 — channelmixerrgb's
  colour-checker GUI state is only partly covered by gui_lock. The races here share its
  profiling/validation asymmetry and require a coordinated synchronization fix. This
  report is not a duplicate: neither delta_E_in nor checker is among #22058's five
  fields — #22058 hands both to this report — and the primary allocation-size defect
  occurs sequentially and survives every locking change. Between them the two reports
  cover every shared gui_data field of this module that the audit found.
• #22005 — a different defect in
  the same function (_extract_patches() inverting the exposure transform wrongly).
• #22060 —
  colorreconstruction's frozen grid. The other pointer-lifetime defect of this audit, but
  a use-after-free rather than a sizing bug.

Start in src/iop/channelmixerrgb.c at _extract_patches(), _checker_changed_callback(), gui_post_expose(), and the profiling and validation callers; read dev-doc/GUI_Threading.md for the validity-flag pattern. Compare the buffer lifetime and checker publication with src/common/colorchecker.h. Done means checker changes no longer cause out-of-bounds access, and the buffer and checker are synchronized across pipe and GTK access.