Fix a use-after-free on the pipeline banner message - #1151
Merged
Conversation
…while a worker frees it
darktable.main_message is the string painted over the main preview while the pipeline works.
The pipeline writes it from WORKER threads; dt_control_draw_busy_msg() reads it on the GUI
thread and hands it straight to pango_layout_set_text() (control.c:524).
The reader held no lock. Its four call sites hold thumb->lock (gui/dtgtk/thumbnail.c:745,
preview_window.c:148) or d->lock (views/slideshow.c:492, views/studio_capture.c:858), which
guard those widgets and have nothing to do with this string. So the GUI thread could be inside
pango reading the buffer while a worker executed dt_free(darktable.main_message) in
dt_set_main_message().
It is not a narrow window. _pipeline_busy() is called once per module per frame for as long as
the darkroom is rendering (pixelpipe_hb.c:975, under gui_attached), so the free rate during an
edit is the module count times the frame rate.
The lock that existed was in the wrong place to help: dt_set_main_message() took none of its
own, and the single writer took control->log_mutex at its CALL SITE (darktable.c:750). An
invariant maintained by every caller rather than by the accessor is one no future caller will
know about -- and it never covered the reader anyway.
Fixed by giving the string its own lock, inside the accessors:
- dt_set_main_message() takes it, so any writer is safe without knowing to.
- dt_get_main_message() is replaced by dt_get_main_message_copy(), which g_strdup()s under
the lock and hands ownership to the caller. There is no safe way to lend this pointer: the
next write frees it and the writer is on another thread. It had no callers, so no contract
is being broken -- the reader was going through darktable.main_message directly.
- _pipeline_busy() drops its now-redundant control->log_mutex, which must NOT come back: that
mutex guards the message LOG, dt_pthread_mutex_lock is non-recursive, and an outer
acquisition around the setter would now self-deadlock.
A private lock rather than control->log_mutex also decouples the string from dt_control_t's
lifetime, which darktable.c frees at teardown while this string outlives it.
Found while surveying src/control for its decomposition, not by a crash report -- but it is a
plausible cause of unexplained darkroom crashes inside pango or cairo, which would land nowhere
near this code.
Release, Debug and nofeatures build; ctest 8/8; headless export unaffected (it exercises the
writer on worker threads, and its teardown path).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
…_DEBUG build
A review flagged this as undefined behaviour: in _DEBUG builds dt_pthread_mutex_t carries ~1.8 kB
of instrumentation after the pthread_mutex_t, and the claim was that
`{ PTHREAD_MUTEX_INITIALIZER }' initialises only the first member and leaves the rest as garbage
that dt_pthread_mutex_lock() then reads.
Not valid, for two independent reasons either of which suffices. A brace-enclosed initialiser
with fewer initialisers than members zero-fills the remainder (C11 6.7.9p21); and the object is
`static', so it has static storage duration and lives in .bss regardless of what is written.
Measured rather than argued: a program declaring exactly this, compiled -D_DEBUG against this
header, reports sizeof 1904 (so the fat struct is genuinely in play), 0 of the 1864 trailing
bytes non-zero before first use, and completes 100000 lock/unlock cycles.
The code path is fine even on the claim's own terms: `char *name = mutex->name' takes the
buffer's ADDRESS, and lock() snprintf()s over that buffer before the strncmp() that reads it, so
the initial contents are never observed; top_wait_sum[] zeros compare fine and top_wait_name[]
all-zero is a valid empty string. The only difference from dt_pthread_mutex_init() -- which
memsets, then names the mutex after its init site -- is that `name' starts empty, and it is
overwritten on the first lock.
Static rather than initialised in dt_init() is deliberate: this string is written from worker
threads, and a lock valid from program start has no window in which it is not. The same pattern
already ships at caches/pixelpipe_cache_wait.c:51 and passed Win64 and macOS CI in #1148.
No behaviour change: comment only. Debug build with -Werror still clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
darktable.main_messageis the string painted over the main preview while the pipeline works. The pipeline writes it from worker threads;dt_control_draw_busy_msg()reads it on the GUI thread and passes it straight topango_layout_set_text()(control.c:524).The reader held no lock. Its four call sites hold
thumb->lock(thumbnail.c:745, preview_window.c:148) ord->lock(slideshow.c:492, studio_capture.c:858) — those guard the widgets, and have nothing to do with this string. So the GUI thread could be inside pango reading the buffer while a worker randt_free(darktable.main_message).It is not a narrow window:
_pipeline_busy()fires once per module per frame for as long as the darkroom is rendering (pixelpipe_hb.c:975, undergui_attached). The free rate during an edit is the module count times the frame rate.The lock that did exist was in the wrong place to help.
dt_set_main_message()took none of its own, and the single writer tookcontrol->log_mutexat its call site (darktable.c:750). An invariant maintained by every caller rather than by the accessor is one the next caller won't know about — and it never covered the reader anyway.The fix — the lock moves inside the accessors
dt_set_main_message()takes it, so a writer is safe without having to know.dt_get_main_message()becomesdt_get_main_message_copy():g_strdup()under the lock, ownership to the caller. There is no safe way to lend this pointer — the next write frees it and the writer is on another thread. It had zero callers, so no contract breaks; the reader was going throughdarktable.main_messagedirectly._pipeline_busy()drops its now-redundantcontrol->log_mutex. That one must not come back: it guards the message log,dt_pthread_mutex_lockis non-recursive, and an outer acquisition around the setter would now self-deadlock.A private lock rather than
control->log_mutexalso decouples the string fromdt_control_t's lifetime, whichdarktable.cfrees at teardown while this string outlives it.Provenance
Found while surveying
src/controlfor its decomposition — not from a crash report. But it is a plausible cause of unexplained darkroom crashes inside pango or cairo, which would land nowhere near this code. Worth a glance at Sentry for pango/cairo faults in the centre-view paint path.Verification
Release, Debug (
-Werror) and nofeatures build;ctest8/8; headless export unaffected — it exercises the writer on worker threads and the non-GUI teardown branch.grepconfirms no rawdarktable.main_messageread survives outsidedarktable.c.The GUI path itself I could not exercise here (no headless X server on this machine, and
DISPLAYis the maintainer's live desktop), so a darkroom smoke test before merge is worth it — the banner should still show module names while rendering.🤖 Generated with Claude Code