[Gnucash-changes] Eliminate a double free of memory.

[Derek Atkins]

Quoting Chris Shoemaker <c.shoemaker at cox.net>:

> On Thu, Jun 02, 2005 at 10:15:18PM -0400, Chris Shoemaker wrote:
> > On Thu, Jun 02, 2005 at 06:55:18PM -0400, David Hampton wrote:
> > > On Thu, 2005-06-02 at 17:33 -0400, Derek Atkins wrote:
> > > It might be possible to add a call to g_signal_connect_after(cw->dialog,
> > > "destroy", gtk_widget_destroyed, &cw->dialog) when the dialog is created
> > > to resolve this case.   If the window destruction completes before the
> > > CM close function is called, then this new callback will have zeroed the
> > > cw->dialog pointer.  
> > > 
> > > Of course, since the gnc_customer_window_close_handler function frees
> > > cw, it would have to remove this callback so the original invocation
> > > order we were discussing won't write to freed memory.  I don't know if
> > > this will work though as I don't know if glib handles deleting callback
> > > functions from within a callback.  It appears that glib builds an
> > > ordered list of all handlers before calling the first handler, so the
> > > above solution will not work for all cases.
> > > 
> > > I suppose this could be flipped and the gnc_customer_window_destroy_cb
> > > callback could be connected "after" the gtk_widget_destroyed handler.
> > > In this case if the dialog has been closed via the window manager close
> > > button cw->dialog will always be NULL regardless of whether the CM
> > > callback runs immediately or at some later time.  If the dialog has been
> > > closed some other way, then cw->dialog will be non-NULL and this
> > > function can call gtk_widget_destroy.  I think this works. 
> > 
> > Maybe there's an even easier way: sink and ref the dialog when it gets
> > created, and then unref it only in the CM close handler, right before
> > the gtk_destroy_widget() call.  Then you know the dialog won't be
> > freed until you're done with it.
> 
> After thinking about this overnight, I think taking a ref is probably
> the right thing to do.  After all, cw->dialog _is_ essentially now an
> uncounted ref.  And ref counting is supposed to prevent the exact kind
> of surprise that started this discussion:  use after free.

I really don't think there's a bug here...  There was a use-after-free but it
was from the context, not the widget.  Hampton fixed that.  I really don't
think we need to re-architect the rest of the code.  It works just fine, and
all the rest of gnucash uses this same model:

CM close callback tells the widget to destroy itself
Gtk Destroy callback frees the context and clears the references.

So I don't think we need to do anything.  What we've got works just fine (modulo
the bug that hampton fixed already).

> -chris

-derek

-- 
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       warlord at MIT.EDU                        PGP key available