`channelmixerrgb`: four colour-checker profiling fields are only partly covered by the lock

channelmixerrgb: colour-checker GUI state is only partly covered by the lock

Found by code analysis (Claude + Codex, cross-checked) during the gui_data threading
audit, 2026-08-19. Expanded on 2026-08-30 with the separately discovered
delta_E_label_text lifetime race, after rechecking both findings against
src/iop/channelmixerrgb.c at 037dee3956. Extended on 2026-09-01 with three more
partly-locked colour-checker fields -- profile_ready, homography and
inverse_homography -- confirmed against d5bf02a1ed. No known crash.

Severity: medium overall. The four flag and margin races have low-severity
stale-state effects. A fifth scalar, profile_ready, can silently discard a profile the
user just solved. The two homography matrices can be sampled mid-write, giving wrong patch
bounds and a bogus validation report. The eighth field is a heap pointer: its narrow but
reachable interleavings include a use-after-free while GTK copies the string and a double
free against a module reset.

Description

channelmixerrgb looks locked and mostly is. It calls
dt_iop_gui_enter_critical_section() in several places, so a scan that asks only whether
the module ever takes the lock clears it. Eight fields of gui_data are nevertheless
covered on only some access paths:

• run_profile
• run_validation
• is_blending
• safety_margin
• profile_ready
• homography
• inverse_homography
• delta_E_label_text

The gaps are spread across commit_params(), the colour-checker validation path in
process(), a GTK-side commit callback, and a GTK-side reset. A fix therefore cannot be
confined to commit_params() or to one field.

mix, xy and optimization are not part of this defect, although a scanner flags
them: their pipe-side accesses are all inside _extract_color_checker() (:1643), whose
only caller holds gui_lock across the whole call at :2162-2166, and _extract_patches()
never touches them, so the unlocked validation route does not reach them. They are recorded
as false positives in tools/dt-lockcheck/false-positives.json.

The scalar fields

run_profile

The GTK button callback writes g->run_profile under the lock at
src/iop/channelmixerrgb.c:2907-2909. commit_params() reads it unlocked at :3128.
process() also tests it before taking the lock at :2160; only the extraction and
clear at :2162-2166 are protected.

run_validation

The GTK button callback writes g->run_validation under the lock at :2920-2922.
commit_params() reads it unlocked at :3129. The complete validation path in
process() -- the test, _validate_color_checker() call, and clear -- is unlocked at
:2285-2288.

is_blending

commit_params() writes g->is_blending unlocked at :3166. _declare_cat_on_pipe()
reads it at :1187 and is called from both process() (:2133) and process_cl()
(:2307), as well as from reload_defaults() and gui_changed() on the GTK thread.
_set_trouble_messages() adds another GTK-thread read at :2028, reached from the
preview-pipe-finished callback.

safety_margin

The GTK callback writes g->safety_margin under the lock:

// src/iop/channelmixerrgb.c:2872-2874 -- GTK main thread
dt_iop_gui_enter_critical_section(self);
g->safety_margin = dt_bauhaus_slider_get(widget);
dt_iop_gui_leave_critical_section(self);

_extract_patches() reads it on the pipe at :1438. The profiling route is protected
because process() holds the lock across _extract_color_checker() at :2162-2166.
The validation route reaches the same read through _validate_color_checker() at
:1949, but its caller at :2285-2288 holds no lock.

The profiling route holds the lock across an entire extraction pass rather than
snapshotting the scalar. That is the opposite extreme from the wholly unlocked
validation route and can block the GTK thread for the duration of the pass.

profile_ready

The preview pipe sets g->profile_ready = TRUE at :1909. The profiling caller holds
self->gui_lock across that write and across all publication of g->xy and g->mix
(:2162-2166).

_commit_profile_callback() tests the flag before taking the lock, and only then takes
it to copy the payload:

// src/iop/channelmixerrgb.c:2934-2936 -- GTK main thread
if(!g->profile_ready) return;              // :2934, unlocked test

dt_iop_gui_enter_critical_section(self);   // :2936, payload copy starts here
p->x = g->xy[0];
...

The payload copy is coherent once the callback obtains the lock; the predicate deciding
whether to copy at all is not. A commit click racing publication can observe a stale
FALSE and silently do nothing, discarding a profile the user just solved.

#22080 also mentions profile_ready and cites the same :2934, but only for a GTK-only
path where checker selection returns early without clearing the flag. That stale-state bug
does not cover this pipe/GTK publication race.

The homography matrices

homography and inverse_homography fail through the same unlocked validation route
already described for safety_margin and delta_E_label_text; they are a reader-side gap,
not a new mechanism and not a "neither side locks" defect.

Every writer is locked. _update_bounding_box() rewrites the two nine-float matrices at
:2454-2455, and each of its three callers holds self->gui_lock across the call:
mouse_moved() at :2549-2558, button_released() at :2663-2669, and
_init_bounding_box() (:2458, whose own tail calls it at :2525), reached only from the
locked sections at :2618-2622, :2859-2862 and :2894-2896. Profiling is protected too,
because process() holds the lock around _extract_color_checker() at :2162-2166.

Validation is not protected. process() calls _validate_color_checker() and clears
run_validation with no lock at :2285-2288. That helper calls _extract_patches()
(:1949), which reads g->homography at :1465 and g->inverse_homography at :1487,
once per patch corner over the whole extraction loop. GTK can move the checker while those
loops run, so a single validation pass can sample with matrices assembled from different
box generations -- including a mid-write state, since get_homography() fills nine floats
in place.

The likely result is wrong patch bounds and a bogus validation report. No memory-safety
impact is claimed: both are fixed-size float[9] members, and the loop indices do not
depend on their contents.

gui_post_expose() also reads g->homography unlocked on the GTK thread (:2738-2786).
That is GTK-to-GTK against locked writers and is a lesser concern, but the same snapshot
fix covers it.

The delta_E_label_text pointer

g->delta_E_label_text is a heap gchar * written by the preview pipe and read by the
GTK thread:

• _extract_color_checker() frees and replaces it at :1912-1913. Its profiling caller
  holds gui_lock across the operation at :2162-2166.
• _validate_color_checker() frees and replaces it at :1968-1969. Its validation
  caller at :2285-2288 holds no lock.
• _preview_pipe_finished_callback() passes it to gtk_label_set_markup() while holding
  gui_lock at :3035-3037.
• reload_defaults() frees and NULLs it at :3917-3920 without gui_lock.

A lock on the reader and only one of the writers does not establish mutual exclusion.
The unlocked validation writer can free the allocation while GTK is reading it.

GTK consumes the input pointer

In GTK 3.24.52, gtk_label_set_markup() duplicates its input with
g_strdup(str ? str : "") at gtk-3.24.52/gtk/gtklabel.c:2808. GTK 4.23.3
first compares it with g_strcmp0() and then duplicates it at
gtk-4.23.3/gtk/gtklabel.c:3234-3238, reached from :3964.

Passing NULL is safe in both versions. A non-NULL pointer must remain valid while the
comparison and copy read through it. Pango subsequently parses GTK's copy, so the unsafe
read is the comparison or duplication of the caller-owned string, not later parsing of
that original allocation.

Reachable interleavings

The preview-finished signal does not serialize the GTK reader with a later pipe run.
dt_dev_process_image_job() releases pipe->mutex at
src/develop/develop.c:939 and raises the signal at :958. The signal is asynchronous
and is queued on the main context by src/control/signal.c:376-379.

A use-after-free can therefore occur as follows:

1. A preview run finishes and queues _preview_pipe_finished_callback().
2. Before that callback drains, the GTK thread handles a validation-button event, sets
   run_validation, and requests another preview run.
3. The new preview run reaches the unlocked free at channelmixerrgb.c:1968 while the
   earlier GTK callback is copying the old string under a lock the writer does not take.

An arbitrary slider, mask, or reprocess event is not sufficient by itself:
_validate_color_checker() is gated by run_validation, which the validation path
clears at :2288. An active validation request is required for the unlocked writer to
run.

There is a separate double-free window through module reset:

1. A requested validation is running on the preview pipe.
2. The user resets the module. _gui_reset_callback() and _gui_reset_clicked() call
   dt_iop_reload_defaults() at src/develop/imageop.c:2510 and :2551 without taking
   the pipe mutexes or gui_lock.
3. reload_defaults() and _validate_color_checker() can both load and free the same
   old delta_E_label_text pointer.

dt_iop_reload_defaults()'s DT_ENTER_GUI_UPDATE() is not synchronization with the
pipe; src/common/darktable.h:323-327 shows that it only increments and decrements the
GUI-reset counter.

Disputed image-switch scenario

The original label report also attributed the reload_defaults() double-free window to
an image switch. The normal image-switch route appears to exclude that interleaving: it
acquires the preview, full and preview2 pipe mutexes at
src/views/darkroom.c:1417-1451, calls dt_iop_reload_defaults() at :1528, and releases
those mutexes only at :1642-1644. On that route, a screen pipe should not be able to
execute _validate_color_checker() concurrently with the free.

Keep this scenario disputed rather than dismissed until the fix is prepared. The
implementing developer should make one final pass over every live image-switch and
reload_defaults() entry route, all three screen pipes, and queued preview-finished
callbacks. The mutex coverage above argues against the scenario; it does not establish
that some other image-switch-adjacent route cannot reach the same reset without that
coverage. Do not cite image switching as confirmed impact unless that final trace finds
such a route.

Impact

The scalar fields have formal data races. A profiling or validation run can observe
a stale flag or margin, be started or cleared twice, or use an is_blending value that
does not match the current blend state. A commit-profile click racing the pipe's
profile_ready publication can be silently rejected, discarding a solved profile with no
feedback. A validation pass racing a checker drag can run against a mixed checker
transform and report Delta E figures for patch bounds that were never on screen.

The label field raises the impact above the original report's low-severity logical bugs:
the GTK callback can read an allocation concurrently freed by the validation pipe, and a
module reset can race the same pipe-side free. The allocation is small and the writer
immediately allocates a replacement, so stale or garbled text is plausible; heap
corruption or a crash is also possible. No runtime reproduction is currently known.

Suggested fix

• Claim and clear run_profile and run_validation under the same short critical
  section in process(), so the tests and state transitions are synchronized with the
  GTK callbacks. Do not let a later clear overwrite a new request.

• Protect the shared commit_params() accesses when a GUI and initialized gui_lock
  exist. Preserve a non-GUI path: export has no gui_data, and gui_lock is initialized
  only as part of GUI setup.

• Snapshot safety_margin under the lock and pass the snapshot into extraction rather
  than holding the lock across the extraction pass.

• In _commit_profile_callback(), move the profile_ready test into the same critical
  section as the payload copy, so readiness and payload are snapshotted atomically. Do not
  test, unlock, then re-lock: they are one state.

• On the validation path, take one short critical section that snapshots the checker
  pointer, both homography matrices and the extraction scalars together, and pass that
  snapshot into _extract_patches(). Snapshotting them independently would still let a
  validation pass pair a matrix from one box generation with a checker from another.
  Holding gui_lock across the whole per-pixel extraction would be correct for exclusion,
  but it reproduces the long GTK stall this report already rejects for the profiling
  route.

• Synchronize the is_blending writer and both GTK- and pipe-side readers.

• Build each new label string in a local allocation outside the critical section. Swap
  it with g->delta_E_label_text under the lock, then free the detached old allocation.

• In _preview_pipe_finished_callback(), copy the pointer under the lock
  (gchar *text = g_strdup(g->delta_E_label_text)), release, then pass the copy to
  gtk_label_set_markup() and free it. Do not detach by NULLing the field: the
  callback is connected to DT_SIGNAL_DEVELOP_PREVIEW_PIPE_FINISHED at :4441 and so
  runs after every preview pipe, not only after a profiling or validation run, and a
  NULLed field would blank the report on the next one.

  Holding the lock across the whole gtk_label_set_markup() call would also be
  lifetime-safe once every writer takes the lock, but it blocks the preview pipe for the
  rest of that call for no benefit. GTK touches the caller's string only at the top: GTK 3
  duplicates it at gtk-3.24.52/gtk/gtklabel.c:2808, GTK 4 reads it with
  g_strcmp0() at gtk-4.23.3/gtk/gtklabel.c:3234 before duplicating at
  :3238. Everything after that works on GTK's own copy: gtk_label_recalculate()
  (gtk3 :2812) parses the copy twice (parse_uri_markup() :2669,
  pango_parse_markup() :2716), clears the layout, queues a resize, and the closing
  g_object_thaw_notify() at :2814 dispatches the queued property notifications
  synchronously. That last one is a latent reentrancy surface rather than a present
  hazard -- nothing in this tree connects a handler to any property
  gtk_label_set_markup() touches -- so it is a reason to prefer the copy, not a defect
  in the current code. Copying under the lock also keeps this bullet consistent with the
  safety_margin one above, which rejects holding the lock across a long pass.

• In reload_defaults(), detach and NULL the label pointer under the same lock before
  freeing the detached allocation. gui_cleanup() does not need that lock: framework
  teardown excludes the screen pipes, and the mutex itself is being destroyed there.

  Taking gui_lock in reload_defaults() is safe under the existing if(g) guard at
  :3903 on every normal route, and the reasoning is the ordering, not the guard itself:
  dt_iop_gui_init() initializes the mutex (src/develop/imageop.c:1441) immediately
  before calling module->gui_init(), which is the only thing that allocates a module
  instance's gui_data (IOP_GUI_ALLOC at channelmixerrgb.c:4418). For an instance in
  dev->iop, non-NULL gui_data therefore implies an initialized lock. Do not read that
  as a general invariant of the type: src/develop/imageop_gui.h:33-43
  (DT_IOP_SECTION_FOR_PARAMS_DECL) builds a compound-literal dt_iop_module_t that
  copies gui_data and leaves gui_lock all-zero, used at channelmixerrgb.c:4635 among
  others -- harmless only because nothing on that path takes the lock.

  The exception that matters here is #22062's undo/redo route: src/libs/history.c:523
  calls module->gui_init() directly and skips the wrapper, so a recreated instance
  carries allocated gui_data beside an all-zero gui_lock into every later
  reload_defaults() -- the image switch at src/views/darkroom.c:1528, or a module
  reset. That is a framework defect this fix cannot work around and should not try to:
  dt_iop_gui_enter_critical_section() locks the mutex with no readiness test
  (src/develop/imageop.h:343-353), and a module cannot test one either. Land #22062
  first, or alongside. This module is already exposed on that route today, because
  process() takes the same critical section at :2162-2166, so the change here adds one
  more site to an existing exposure rather than creating a new one -- but it does add one,
  and #22062 is what removes it.

The mutex is not recursive. Keep critical sections around state access and ownership
transfer; do not call helpers that can acquire the same lock from inside one.

Related

• #22080 is a separate allocation-size and state-pairing defect: the ΔE buffer is sized
  for the checker selected when it was allocated. Neither delta_E_in nor checker is
  among the eight fields here -- #22080 owns both, links back to this report by number, and
  shares this report's locked-profiling/unlocked-validation path, so the two fixes need to
  be coordinated. This report does not subsume it. The coordination is concrete:
  checker, the two transform matrices, the safety margin and the ΔE allocation are one
  extraction generation and have to be snapshotted as one, even though the two reports
  cover different failures.
• #22005 is a separate defect in the same functions: the colour-checker profiler inverts
  the exposure transform with the wrong quantity and formula and obtains it from the
  wrong thread. Its incidental mention of delta_E_label_text does not cover this race.
• #22060 covers colorreconstruction's frozen-grid pointer lifetime, not this field or
  call path.

Start in src/iop/channelmixerrgb.c at process(), commit_params(), _validate_color_checker(), _extract_patches(), and the GTK callbacks; trace each of the eight gui_data fields through their lock boundaries. Read the cited develop.c, signal.c, imageop.c, and darkroom.c paths before deciding how reset and queued callbacks interact. Done means all eight fields have consistent synchronization on every listed reader and writer path, with no lost requests or unsafe label lifetime, and the disputed image-switch route is resolved.