From 99ffeba5a0dce6ed7736a86b83cdb36db3b1186c Mon Sep 17 00:00:00 2001 From: Tom Poczos Date: Tue, 1 Sep 2026 21:33:15 +0200 Subject: [PATCH] preview_data: guard the pipe-nodes walk in dt_preview_data_is_fresh with busy_mutex dt_preview_data_is_fresh() (introduced in 8a26f35f92 / #21397, the shared preview-data service for interactive editing) walks dev->preview_pipe->nodes and hashes the matching piece from the GUI thread while holding only the module's own gui_lock. pipe->nodes is actually protected by pipe->busy_mutex: dt_dev_pixelpipe_cleanup_nodes() frees every piece under that lock whenever a history change rebuilds the pipe topology (DT_DEV_PIPE_REMOVE in dt_dev_pixelpipe_change), and that can run concurrently on the pipe-processing thread. A caller that walks pipe->nodes off the GUI thread without busy_mutex can therefore dereference a piece that gets freed out from under it -- reproduced as a segfault dereferencing a freed piece->module in _dev_pixelpipe_cache_basichash(), from a module's preview-pipe-finished GUI callback calling dt_preview_data_is_fresh(). colorequal.c's own interactive hue-editing mode (also from #21397) calls dt_preview_data_is_fresh() from mouse-hover/scroll handlers and is exposed to the same race in principle, just from a narrower window that makes it harder to trigger in practice. Take pipe->busy_mutex around the walk. It must be a trylock, not a blocking lock: a module's process() runs under busy_mutex for the whole pipe run (dt_dev_pixelpipe_process) and can itself call dt_preview_data_store() (colorequal.c does, twice), which takes this same module's gui_lock -- the opposite order -- so a blocking lock here would AB-BA deadlock against it. A pipe that's currently busy can't have fresh data for us anyway, so treating "busy" as "not fresh" costs nothing. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01WpGKm49b4w1L51YETXyGpU --- src/develop/preview_data.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/develop/preview_data.c b/src/develop/preview_data.c index 66ed7e76dde..b26710cce78 100644 --- a/src/develop/preview_data.c +++ b/src/develop/preview_data.c @@ -179,13 +179,27 @@ gboolean dt_preview_data_is_fresh(dt_preview_data_t *pd) if(fresh) { - const dt_develop_t *const dev = module->dev; - if(!dev || !dev->preview_pipe) + dt_dev_pixelpipe_t *const pipe = module->dev ? module->dev->preview_pipe : NULL; + if(!pipe) + fresh = FALSE; + // pipe->nodes (and the pieces it points to) are only safe to walk while + // holding busy_mutex: a concurrent history change can rebuild the whole + // list -- dt_dev_pixelpipe_cleanup_nodes() frees every piece -- while this + // runs on the GUI thread, outside that protection, causing a + // dangling-pointer segfault in the hash walk below. Must be a trylock, + // not a blocking lock: a module's process() runs under busy_mutex for + // the whole pipe run (dt_dev_pixelpipe_process) and can itself call + // dt_preview_data_store(), which takes this same module's gui_lock + // (already held above) -- the opposite order -- so a blocking lock here + // would AB-BA deadlock against it. A pipe that's currently busy can't + // have fresh data for us anyway, so treating "busy" as "not fresh" + // costs nothing. + else if(dt_pthread_mutex_trylock(&pipe->busy_mutex)) fresh = FALSE; else { dt_dev_pixelpipe_iop_t *piece = NULL; - for(GList *iter = dev->preview_pipe->nodes; iter; iter = g_list_next(iter)) + for(GList *iter = pipe->nodes; iter; iter = g_list_next(iter)) { dt_dev_pixelpipe_iop_t *const p = (dt_dev_pixelpipe_iop_t *)iter->data; if(p->module == module) @@ -201,6 +215,7 @@ gboolean dt_preview_data_is_fresh(dt_preview_data_t *pd) const dt_hash_t cur_hash = dt_dev_pixelpipe_piece_hash(piece, &piece->processed_roi_out, TRUE); fresh = (cur_hash == stored_hash); } + dt_pthread_mutex_unlock(&pipe->busy_mutex); } }