> On Dec 2, 2015, at 9:20 PM, K. pestophagous Heller <[email protected]> > wrote: > > The DiveEventItem(s) need not store a pointer to memory
"need not" is odd. "should not" maybe? > 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. Some of this explanation could even migrate up to the commit message. It's always nice to have more context there. > (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 Hehe > 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 :) No, your analysis seems solid. > 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. No, what you are doing sounds like the right thing to do. Having a copy of the event seems the safest course of action. > 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. Wise. It's a pain to mess with 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. That's the beauty and the curse of it. It makes things presumably easier when writing code - but then it comes back to bite you. > 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." Yes - that's definitely something we need. Tomaz thought he had an idea how to do that - but sadly he's very busy at work right now. > 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. Good work. > 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? Yes, I'd like that better > + int size = sizeof(*ev) + strlen(ev->name) + 1; > + internalEvent = (struct event*) malloc(size); > + memcpy(internalEvent, ev, size); > + internalEvent->next = NULL; that way this code would be in dive.c and we'd just call that helper. > 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); The rest looks good to me. Would you re-edit this and re-submit? Thanks /D _______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
