darktable-org / darktable-org/darktable
`colorreconstruction`: the frozen bilateral grid is freed while another pipe reads it
Nobody has claimed this yet.
- 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) isdt_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
- Thaw under the lock. Move
b = _thaw(can)inside the critical section at 634-636.
_thaw()allocates and copies a whole grid, so this holdsgui_lockacross an
allocation and a largememcpy, andgui_update()on the GTK thread would block
behind it — visible to the user. - Reference-count the frozen grid, or hand ownership over: take the pointer out
under the lock and NULLg->canin 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. - 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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