The DiveEventItem(s) need not store a pointer to memory that they do not own. This way, no matter how the path of execution arrives into slot recalculatePos, we never need fear that the DiveEventItem will dereference a garbage pointer to a struct event.
Fixes #968 Signed-off-by: K. Heller <[email protected]> --- So here is one fix for trac ticket #968. Certainly there could be other fixes. What follows is my rationale for doing it this way. First thing to know is that the reason the tank icons "walk" to incorrect spots on the profile is that the DiveEventItem holds a pointer to a struct event -- even after the struct event at that address has been freed. When 'internalEvent' is a pointer to freed memory, internalEvent->time.seconds can have all kinds of crazy values, which get used in DiveEventItem::recalculatePos to place the tank at weird x coordinates. (if anyone needs convincing, you can try a 'diagnostic' patch instead of the fix patch. one such diagnostic patch is shown: https://github.com/pestophagous/subsurface/commit/8625ff058fc73a2d3612655455464a9f3708615b ) As you might guess, the following function is partly implicated in the code path to the bug: ProfileWidget2::plotDive plotDive gets called at inopportune times. Also, there are several branches near the top of plotDive that cause us to exit plotDive early (before refreshing the gaschange events). This function also matters: create_dive_from_plan in subsurface-core/planner.c because the pointers-to-struct-events get malloc'ed and freed in create_dive_from_plan. So... ...debate at will :) Could there be some other fix involving tighter coordination between create_dive_from_plan and ProfileWidget2::plotDive? Maybe, but attempting that doesn't sound palatable to me. Could there be a fix where we remove or adjust the ways to jump out of plotDive early? Maybe, but plotDive is so huge and also so central that I don't want to touch it. DiveEventItem::recalculatePos is a slot. As many have said, one faces an uphill battle (a losing battle) to try to reason cleanly about ordering and dependency graphs and such between all the various signals and slots. Recently (here: http://lists.subsurface-divelog.org/pipermail/subsurface/2015-December/023509.html ), Dirk mentioned that reshuffling where the call site to plotDive goes might just be hiding a problem, and "we need a way to know when the profile is fully updated and ready to be rendered." I wholeheartedly agree. Any slot needs to be ready with its own internal "defenses" and sanity-checking so that it only executes its slot duties when it is safe. This patch for DiveEventItem is in keeping with that philosophy. DiveEventItem now won't have to blindly trust about the lifetime of a pointer it never properly owned. Now DiveEventItem can "defend itself" to never use a garbage pointer. profile-widget/diveeventitem.cpp | 13 ++++++++++++- profile-widget/diveeventitem.h | 1 + 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/profile-widget/diveeventitem.cpp b/profile-widget/diveeventitem.cpp index 0bbc842..52e4995 100644 --- a/profile-widget/diveeventitem.cpp +++ b/profile-widget/diveeventitem.cpp @@ -19,6 +19,10 @@ DiveEventItem::DiveEventItem(QObject *parent) : DivePixmapItem(parent), setFlag(ItemIgnoresTransformations); } +DiveEventItem::~DiveEventItem() +{ + free(internalEvent); +} void DiveEventItem::setHorizontalAxis(DiveCartesianAxis *axis) { @@ -48,7 +52,14 @@ void DiveEventItem::setEvent(struct event *ev) { if (!ev) return; - internalEvent = ev; + + free(internalEvent); + // logic for copying ev partially copied from copy_events. + // maybe dive.c should have a 'copy_event' used here and by copy_events? + int size = sizeof(*ev) + strlen(ev->name) + 1; + internalEvent = (struct event*) malloc(size); + memcpy(internalEvent, ev, size); + internalEvent->next = NULL; setupPixmap(); setupToolTipString(); recalculatePos(true); diff --git a/profile-widget/diveeventitem.h b/profile-widget/diveeventitem.h index f358fee..9d6ad5d 100644 --- a/profile-widget/diveeventitem.h +++ b/profile-widget/diveeventitem.h @@ -11,6 +11,7 @@ class DiveEventItem : public DivePixmapItem { Q_OBJECT public: DiveEventItem(QObject *parent = 0); + virtual ~DiveEventItem(); void setEvent(struct event *ev); struct event *getEvent(); void eventVisibilityChanged(const QString &eventName, bool visible); -- 2.5.0 _______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
