[PATCH] Prevent gaschange tank icons from using garbage coords.

[Dirk Hohndel]

> On Dec 2, 2015, at 9:20 PM, K. pestophagous Heller <pestophagous at gmail.com> 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 <pestophagous at gmail.com>
> ---
> 
> 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