darktable-org / darktable-org/darktable

`colorreconstruction`: the frozen bilateral grid is freed while another pipe reads it

Open
#22,060 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

scope: threading
Dominant language
C
Stars
13.1k
Forks
1.4k
Avg merge
22h 14m
Merged PRs (30d)
198

Description

Summary

The full pipe copies the pointer g->can out of gui_data under gui_lock, releases
the lock, and only then dereferences it. Two other threads may free that same object
under the same lock in the meantime. The lock protects the pointer load and not the
pointee — the exact shape dev-doc/GUI_Threading.md calls out under Short Is Not The
Same As Correct
.

The window

Reader, process(), on a full pipe (process_cl() is the same at 1031-1039):

  if(!dt_dev_sync_pixelpipe_hash(...&self->gui_lock, &g->hash))   /* 632 */
    dt_control_log(_("inconsistent output"));

  dt_iop_gui_enter_critical_section(self);
  can = g->can;                       /* 634 — pointer copied out    */
  dt_iop_gui_leave_critical_section(self);
}

if(can)
  b = dt_iop_colorreconstruct_bilateral_thaw(can);   /* 641 — dereferenced unlocked */

dt_iop_colorreconstruct_bilateral_thaw() (338-...) reads bf->size_x … bf->sigma_r
and copies bf->buf.

Writers, both of which free the object the reader is holding:

  • The preview pipe, at the foot of the same process() (655-662):
    dt_iop_colorreconstruct_bilateral_dump(g->can); g->can = freeze(b); inside a
    critical section. _dump() (247-252) is dt_free_align(bf->buf); free(bf);.
  • The GTK thread, gui_update() (1182-1187): the same _dump(g->can) followed by
    g->can = NULL, inside a critical section.

Both take gui_lock, so they are correctly serialised against the load at 634 — and
not at all against the use at 641. The reader has released the lock by then.

gui_cleanup() (1255-1259) dumps it once more without the lock, which is fine on its
own: the framework holds the three screen-pipe mutexes across module GUI teardown, so no
pipe is running there.

Why the hash wait does not close it

dt_dev_sync_pixelpipe_hash() only establishes that the preview pipe has finished a
run whose upstream state matches. Nothing stops it starting the next one — a slider
move, a zoom, a new history item — immediately afterwards, and that run reaches 659 and
frees the grid the full pipe is still thawing. gui_update() needs even less: any
external params change while the full pipe is between 634 and 641.

Contrast with the other three users of this idiom

levels, globaltonemap and hazeremoval publish plain scalars through gui_data with
the same hash handshake, and copy the values themselves out under the lock. For them the
span in which the value must stay valid ends at the load, so the short critical section
is exactly right. colorreconstruction is the one that hands over a heap object, and it
kept the same locking.

Possible fixes, in increasing cost

  1. Thaw under the lock. Move b = _thaw(can) inside the critical section at 634-636.
    _thaw() allocates and copies a whole grid, so this holds gui_lock across an
    allocation and a large memcpy, and gui_update() on the GTK thread would block
    behind it — visible to the user.
  2. Reference-count the frozen grid, or hand ownership over: take the pointer out
    under the lock and NULL g->can in the same section, so the reader owns it and dumps
    it when done; the publisher then always allocates a fresh one. Cheapest correct fix,
    but it costs the cache hit for the next full-pipe run.
  3. Keep a generation counter beside the pointer and re-check it after the thaw,
    discarding the result on a mismatch. Correct without a long lock, but it can waste a
    thaw.

Option 2 looks like the best trade unless the re-freeze cost matters.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read dev-doc/GUI_Threading.md, then locate process(), process_cl(), gui_update(), gui_cleanup(), and the bilateral thaw/dump functions. Trace the frozen grid's ownership from the protected pointer load through thawing and freeing, and choose a lifetime-safe approach from the options described. Done means the full pipe cannot dereference a grid after another thread frees it.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
desktop, operating-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
50/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.