From 769403a4b2c3dd8e8cc94d7f1b5dad6b66d10caf Mon Sep 17 00:00:00 2001 From: Berthold Stoeger Date: Thu, 17 Oct 2019 22:56:40 +0200 Subject: Planner: copy deco state before passing it to worker thread The planner has a computeVariations() function that can be run in a worker thread. The code was not thread safe: a deco_state object allocated on the stack of the caller was passed down to the worker thread. It's well possible that the object would go out of scope before the thread run. Therefore, when running in the background, copy the object first and free it in the worker thread. Side note: Qt makes proper memory management again as difficult as possible: You can't pass a std::unique_ptr<> to QtConcurrent::run, because move-only objects are not supported. Not very friendly! Signed-off-by: Berthold Stoeger --- qt-models/diveplannermodel.cpp | 15 ++++++++++++--- qt-models/diveplannermodel.h | 3 ++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/qt-models/diveplannermodel.cpp b/qt-models/diveplannermodel.cpp index b77534819..b8f8d54a4 100644 --- a/qt-models/diveplannermodel.cpp +++ b/qt-models/diveplannermodel.cpp @@ -919,7 +919,10 @@ void DivePlannerPointsModel::createTemporaryPlan() cloneDiveplan(&diveplan, plan_copy); unlock_planner(); #ifdef VARIATIONS_IN_BACKGROUND - QtConcurrent::run(this, &DivePlannerPointsModel::computeVariations, plan_copy, &plan_deco_state); + // Since we're calling computeVariations asynchronously and plan_deco_state is allocated + // on the stack, it must be copied and freed by the worker-thread. + struct deco_state *plan_deco_state_copy = new deco_state(plan_deco_state); + QtConcurrent::run(this, &DivePlannerPointsModel::computeVariationsFreeDeco, plan_copy, plan_deco_state_copy); #else computeVariations(plan_copy, &plan_deco_state); #endif @@ -1004,7 +1007,13 @@ int DivePlannerPointsModel::analyzeVariations(struct decostop *min, struct decos return (leftsum + rightsum) / 2; } -void DivePlannerPointsModel::computeVariations(struct diveplan *original_plan, struct deco_state *previos_ds) +void DivePlannerPointsModel::computeVariationsFreeDeco(struct diveplan *original_plan, struct deco_state *previous_ds) +{ + computeVariations(original_plan, previous_ds); + delete previous_ds; +} + +void DivePlannerPointsModel::computeVariations(struct diveplan *original_plan, const struct deco_state *previous_ds) { // bool oldRecalc = setRecalc(false); @@ -1014,7 +1023,7 @@ void DivePlannerPointsModel::computeVariations(struct diveplan *original_plan, s struct deco_state *cache = NULL, *save = NULL; struct diveplan plan_copy; struct divedatapoint *last_segment; - struct deco_state ds = *previos_ds; + struct deco_state ds = *previous_ds; if (!original_plan) return; diff --git a/qt-models/diveplannermodel.h b/qt-models/diveplannermodel.h index 8fe6e20d6..2a2aaa64d 100644 --- a/qt-models/diveplannermodel.h +++ b/qt-models/diveplannermodel.h @@ -117,7 +117,8 @@ private: void createPlan(bool replanCopy); struct diveplan diveplan; struct divedatapoint *cloneDiveplan(struct diveplan *plan_src, struct diveplan *plan_copy); - void computeVariations(struct diveplan *diveplan, struct deco_state *ds); + void computeVariations(struct diveplan *diveplan, const struct deco_state *ds); + void computeVariationsFreeDeco(struct diveplan *diveplan, struct deco_state *ds); int analyzeVariations(struct decostop *min, struct decostop *mid, struct decostop *max, const char *unit); Mode mode; bool recalc; -- cgit v1.2.3-70-g09d2