Skip to content

fix(flatpak): replace detached threads with promise-tracked workers#243

Open
aki1770-del wants to merge 2 commits into
toyota-connected:v2.0from
aki1770-del:fix/flatpak-thread-lifetime-safety
Open

fix(flatpak): replace detached threads with promise-tracked workers#243
aki1770-del wants to merge 2 commits into
toyota-connected:v2.0from
aki1770-del:fix/flatpak-thread-lifetime-safety

Conversation

@aki1770-del

Copy link
Copy Markdown

Problem

Five std::thread().detach() sites in FlatpakShim have no lifetime
tracking. If FlatpakShim is destroyed while a flatpak transaction is
in flight, the destructor returns immediately — leaving strand_,
event sinks, and the operation tracker accessible to the still-running
thread. A use-after-free in the Flatpak plugin host process crashes
the IVI HMI process entirely.

Additionally, ApplicationInstall contains two sleep_for(seconds(2))
calls used as a proxy for filesystem sync — a TOCTOU race that fails
on slow systems and wastes time on fast ones.

Fixes #231

Changes

Thread lifetime tracking

Added TrackWorker(std::future<void>) to FlatpakShim, implementing
the std::future/std::promise pattern recommended in #231:

  • TrackWorker stores a future per spawned thread, pruning completed
    futures on each call via a non-blocking wait_for(seconds(0)) probe
  • The destructor waits on all pending futures before releasing members
  • WorkerGuard RAII struct (anonymous namespace in .cc) ensures
    promise->set_value() is called on every exit path, including the
    six early-return branches in ApplicationInstall
Site Function Thread name
1 ApplicationInstall flatpak-install
2 ApplicationUninstall flatpak-uninstall
3 ApplicationUpdate flatpak-update
4 install_runtime flatpak-runtime
5 install_extensions flatpak-ext

TOCTOU sleep replacement (ApplicationInstall only)

Both sleep_for(seconds(2)) + one-shot check blocks are replaced
with bounded poll loops (10 × 500 ms, 5 s max) that exit as soon as
flatpak_installation_get_installed_ref returns a valid ref.

A GFileMonitor implementation was evaluated per #231's recommendation.
It requires additional async integration with the existing strand executor;
bounded polling provides deterministic behavior consistent with the strand
model and is reviewable in this PR's scope. GFile monitor integration can
follow as a separate PR if preferred.

Fix Pattern

// Before — no tracking, destructor races running thread
std::thread([self, ...] {
  // ... transaction body with multiple return paths ...
}).detach();

// After — destructor waits for all workers
auto worker_promise = std::make_shared<std::promise<void>>();
TrackWorker(worker_promise->get_future());
std::thread([self, ..., p = std::move(worker_promise)]() mutable {
  WorkerGuard guard{std::move(p)};  // resolves on any exit path
  // ... transaction body unchanged ...
}).detach();
// Before — fixed 2 s sleep, TOCTOU race
std::this_thread::sleep_for(std::chrono::seconds(2));
// ... one-shot flatpak_installation_get_installed_ref check ...

// After — bounded poll, exits on first successful check
for (int retry = 0; retry < kFsyncMaxRetries && !found; ++retry) {
  std::this_thread::sleep_for(kFsyncRetryDelay);
  // ... same check, breaks on success ...
}

Verification

  • All five thread bodies are otherwise unchanged: GObject cleanup,
    asio::post callbacks, and error returns are preserved exactly
  • WorkerGuard calls set_value() after the thread body exits —
    wait() in the destructor correctly indicates thread completion
  • TrackWorker's wait_for(seconds(0)) probe never blocks the
    calling thread
  • TOCTOU replacement: worst-case wait unchanged (≤ 5 s); typical
    case exits earlier; existing check logic is structurally identical

Part of the security audit series: #227 #228 #229 #232 #233 #234 #235 #236

Signed-off-by: aki1770-del aki1770@gmail.com

Five std::thread().detach() sites in FlatpakShim had no lifetime
tracking. If FlatpakShim is destroyed while a transaction is in
flight, the destructor returns without waiting for running threads,
leaving strand_, event sinks, and the operation tracker accessible
to the still-running thread.

Adds TrackWorker(std::future<void>) and WorkerGuard RAII struct.
The destructor waits on all pending futures before releasing members.
WorkerGuard ensures promise resolution on every exit path, including
the multiple early-returns in ApplicationInstall.

Per toyota-connected#231, the two hardcoded sleep_for(seconds(2)) TOCTOU races in
ApplicationInstall are also replaced. A GFile monitor was evaluated
but requires async integration with the existing strand executor
that is out of scope here; bounded polling (10 x 500ms, 5s max)
provides deterministic behavior consistent with the strand model.
GFile monitor integration can follow as a separate PR.

Fixes toyota-connected#231

Signed-off-by: Akihiro Komada <aki1770@gmail.com>
Fix 4 Chromium-style violations flagged by CI format job:
- Single-statement if bodies moved to indented next line (3 sites)
- Long string literal split across lines (1 site)

Co-Authored-By: Claude and aki1770-del <aki1770@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Detached Thread Without Lifetime Safety Guarantees

1 participant