basicadj publishes an auto-exposure region and request without the GUI lock

Found by tools/dt-lockcheck/dt-lockcheck.py (basicadj:box_cood and
basicadj:call_auto_exposure) and confirmed by source analysis at darktable
d5bf02a1ed. No runtime reproduction was attempted. No known crash.

Severity: low. Logical races on a request token and a bounding box. Nothing here is a
pointer and no index leaves its array.

Summary

basicadj asks the preview pipe to calculate automatic exposure through a shared
four-state token, g->call_auto_exposure, and a shared four-coordinate selection,
g->box_cood. Most token transitions use self->gui_lock, but the region-release
callback writes the complete rectangle and sets the request token without that lock. The
preview pipe claims the token under the lock and then reads the rectangle after releasing
it.

A pipe run can therefore consume a partly updated rectangle. A second region request can
also overwrite the -1 (pipe owns the request) or 2 (result ready) state of the first
request, after which either the new request or the completed result is silently lost.

Evidence

All cited code is in src/iop/basicadj.c.

The ordinary auto button shows the intended publication protocol. It takes the GUI lock,
requires the state to be idle, writes the rectangle, and publishes state 1 in the same
critical section:

// :232-238 -- GTK main thread, _auto_levels_callback()
dt_iop_gui_enter_critical_section(self);
if(g->call_auto_exposure == 0)
{
  g->box_cood[0] = g->box_cood[1] = g->box_cood[2] = g->box_cood[3] = 0.f;
  g->call_auto_exposure = 1;
}
dt_iop_gui_leave_critical_section(self);

The selected-region route does not. button_released() runs on the GTK main thread and
writes g->box_cood[0..3], back-transforms and normalizes those same shared values, then
sets the request token, all with no critical section (:360-386):

// :371-384 -- GTK main thread, button_released()
g->box_cood[0] = g->posx_from;
g->box_cood[1] = g->posy_from;
g->box_cood[2] = g->posx_to;
g->box_cood[3] = g->posy_to;
dt_dev_distort_backtransform(darktable.develop, g->box_cood, 2);
g->box_cood[0] /= darktable.develop->preview_pipe->iwidth;
g->box_cood[1] /= darktable.develop->preview_pipe->iheight;
g->box_cood[2] /= darktable.develop->preview_pipe->iwidth;
g->box_cood[3] /= darktable.develop->preview_pipe->iheight;

g->button_down = 0;
g->call_auto_exposure = 1;

Note that the shared array is used as the scratch space for the transform: the four values
are visible to the pipe in their untransformed, then transformed, then normalized forms.

Both pipe implementations claim the request correctly. The CPU path changes 1 -> -1
under the lock at :1419-1423; the OpenCL path does the same at :1301-1305. Both then
call _get_selected_area() after releasing the lock, and that helper loads the four
shared coordinates at :1223:

// :1223 -- pixelpipe worker, _get_selected_area(), no lock held
dt_boundingbox_t box_cood = { g->box_cood[0], g->box_cood[1],
                              g->box_cood[2], g->box_cood[3] };

The pipe publishes state 2 under the lock (process() at :1433-1435, process_cl()
at :1334-1336). _develop_ui_pipe_finished_callback() claims state 2 and changes it
to -1 under the same lock before consuming the result (:279-291). The unlocked GTK
write at :384 can land on any of these states.

dt_dev_reprocess_all() at :386 queues work; it does not wait for an already-running
preview pipe and is not mutual exclusion for the shared fields.

change_image() also resets both fields without the lock at :600-603. That is not
part of this defect: image switching is externally serialized against the pipe workers —
_dev_load_requested_image() holds the preview, full and preview2 pipe mutexes across the
change_image() call (src/views/darkroom.c:1417-1451, :1595, :1641-1644). The same
argument was established for retouch while filing #22081.

Reachable effects

• If the pipe claims state 1 from an earlier request while button_released() is still
  transforming the four shared coordinates, _get_selected_area() can copy a mixture of
  raw, back-transformed and normalized values. Automatic exposure is then calculated over
  a region the user did not select.
• If a second release writes state 1 while the first calculation owns state -1, the
  first calculation later publishes 2 over the second request. The newer request is
  lost.
• If the release overwrites state 2 before the pipe-finished callback consumes it, the
  completed result is discarded and replaced by a new request.

These are logical races, not a demonstrated memory-safety defect. The coordinates remain
inside a fixed-size array.

g->params is not part of this defect, despite the scanner also naming it. The token's
locked ownership transitions order and exclude its pipe writer and GTK reader, and only
one preview pipe runs at a time, so two pipe writers cannot overlap. That finding has been
recorded as a false positive, matching the accepted rgblevels:params entry for the same
code.

Steps to reproduce

Derived from source, not carried out:

1. Run a ThreadSanitizer build, open a large image in darkroom, and enable
   basic adjustments.
2. Select automatic exposure by region and release a rectangle.
3. Before the preview finishes, draw and release one or more different rectangles.
4. Check for a ThreadSanitizer report on call_auto_exposure or box_cood; without
   instrumentation, watch for a lost request or exposure calculated from neither complete
   selection.

Suggested fix

Compute, back-transform and normalize the selected rectangle in local storage. Then take
self->gui_lock and publish the four locals together with the request transition. The
minimal safe policy is to mirror _auto_levels_callback() and publish only from state
0; if the intended UX is instead "latest selection wins", represent a pending generation
explicitly rather than overwriting an owned token.

When the pipe changes 1 -> -1, copy box_cood into a local snapshot in that same
critical section and make _get_selected_area() consume the snapshot. Do not hold the GUI
lock across distortion or histogram work.

Related

• #21919 is the same copied request/rectangle defect in rgblevels, down to the shared
  array being used as transform scratch space. Its fix does not change basicadj.c, so
  this remains a separate issue — but the two should be fixed the same way, and whoever
  takes one should take the other.
• #22081 established the external serialization of change_image() cited above.

Start in src/iop/basicadj.c, reading button_released(), _get_selected_area(), the CPU and OpenCL request transitions, and _develop_ui_pipe_finished_callback(). Build with ThreadSanitizer and follow the described repeated-region-selection scenario; done means publishing the transformed rectangle under the GUI lock, consuming a pipe-local snapshot, and preserving request ownership without holding the lock during processing.