From 1193a22e15413b12d4b342d591053b6ffe46c265 Mon Sep 17 00:00:00 2001 From: Robbie Huxley Date: Mon, 3 Aug 2026 16:53:54 +1000 Subject: [PATCH] Fix heap-use-after-free in threaded area/radius propagation sweeps PlotPropagation(), PlotLOSMap() and PlotPropagationRadius() dispatch std::async(std::launch::async, ...) workers with a raw pointer into a local std::vector (ranges/r/radii), but store the returned std::future in a namespace-scope futures vector that is never read. finishProgress()/finishThreads() only poll progress counters or join an unused threads vector, so a worker can be observed as "done" the instant it increments its counter, one dereference before it actually returns. The local ranges/r/radii vector is then destructed (freeing its backing storage) while that worker still has a pending dereference of its PropagationRange/PropagationRadius pointer - AddressSanitizer confirms this as a heap-use-after-free at src/models/los.cc:175 in rangePropagation. Fix: make futures local to each function that dispatches workers, and actually await every future with .get() before the ranges/r/radii vector can be touched again or destructed. This also removes the implicit reliance on a std::future destructor blocking, which never applied here because futures lived at namespace scope and was never destroyed between calls. Also fixes an independent defect in the same PlotPropagation() cleanup step: erasing ranges by index while iterating shifts every later element down and shrinks size() by one, silently skipping every other element and calling erase(end()) on the last iteration of an even-sized vector. Replaced with ranges.clear(), since every element is discarded anyway. Verified on commit 7f6242af: 10/10 consecutive threaded area-sweep runs now exit 0 with a correct Area boundaries line and a byte-identical coverage.ppm (25622765 bytes), where the unfixed binary crashed (SIGSEGV, exit 139) with a corrupted bbox in roughly half of runs. 6/6 runs under AddressSanitizer show zero heap-use-after-free errors, where the unfixed binary reproduced the same src/models/los.cc:175 UAF reliably under ASan. --- src/models/los.cc | 44 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/src/models/los.cc b/src/models/los.cc index 92fda4d1..0fc9d71a 100644 --- a/src/models/los.cc +++ b/src/models/los.cc @@ -25,9 +25,6 @@ namespace { // Storage for processing threads std::vector threads; - // Storage for processing thread futures - std::vector> futures; - // Mutex for processed vector std::mutex maskMutex; @@ -909,6 +906,9 @@ void PlotLOSMap(struct site source, double altitude, char *plo_filename, double range_max_north[] = {max_north, max_north, min_north, max_north}; PropagationRange *r = new PropagationRange[segments]; + // Storage for this call's async worker handles (function-local: never shared across calls or functions) + std::vector> futures; + // Size our progress vector appropriately thread_progress = std::vector(segments); @@ -937,8 +937,15 @@ void PlotLOSMap(struct site source, double altitude, char *plo_filename, rangePropagation(thread_progress[i], &r[i]); } - if(use_threads) + if(use_threads) { finishThreads(); + // Actually wait for every worker to finish before r is freed below. + // finishThreads() only joins the (unused) threads vector; the real + // workers are std::async futures and must be awaited explicitly. + for (auto &f : futures) { + f.get(); + } + } delete[] r; @@ -1039,6 +1046,9 @@ void PlotPropagation(struct site source, bbox bounds, // Array to hold our edge ranges std::vector ranges; + // Storage for this call's async worker handles (function-local: never shared across calls or functions) + std::vector> futures; + // Create our longitudal (top and bottom edge) ranges for (int i = 0; i < lon_edge_segments; i++) { // Create two ranges for the top & bottom edges which will share longitude values @@ -1140,11 +1150,23 @@ void PlotPropagation(struct site source, bbox bounds, spdlog::debug("Waiting for threads to finish..."); //finishThreads(); finishProgress(); + // finishProgress() only polls progress counters; it does not wait on + // the std::async futures themselves. A worker increments its counter + // just before its final loop check and cleanup, so finishProgress() + // can return while a worker still has one more dereference of its + // PropagationRange pointer left to make. Awaiting every future here + // is the actual wait; ranges (and its backing storage) must not be + // touched until every worker has returned. + for (auto &f : futures) { + f.get(); + } } - for(size_t i = 0; i < ranges.size(); i++){ - ranges.erase(ranges.begin() + i); - } + // Erasing by index while iterating shifts every later element down and + // shrinks size() by one, so this used to skip every other element and + // call erase(end()) on the last iteration of an even-sized vector. + // All elements are being discarded anyway, so just clear the vector. + ranges.clear(); if (fd != NULL) fclose(fd); @@ -1274,6 +1296,9 @@ void PlotPropagationRadius(struct site source, double range, // Size our progress vector appropriately thread_progress = std::vector(segments); + // Storage for this call's async worker handles (function-local: never shared across calls or functions) + std::vector> futures; + // Init our vector for storing processing progress if (!has_init_processed) { init_processed(); @@ -1299,6 +1324,11 @@ void PlotPropagationRadius(struct site source, double range, { spdlog::debug("Waiting for threads to finish..."); finishThreads(); + // finishThreads() only joins the (unused) threads vector; the real + // workers are std::async futures and must be awaited explicitly. + for (auto &f : futures) { + f.get(); + } } // Clean up our radii