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

classic Classic list List threaded Threaded
43 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Chris Shoemaker
On Thu, Jun 02, 2005 at 11:10:48AM -0400, David Hampton wrote:

> --- src/business/business-gnome/dialog-customer.c
> +++ src/business/business-gnome/dialog-customer.c
> @@ -383,7 +383,8 @@
>    CustomerWindow *cw = user_data;
>  
>    gtk_widget_destroy (cw->dialog);
> -  cw->dialog = NULL;
> +  // cw has already been freed by this point.
> +  // cw->dialog = NULL;
>  }

This patch looks wrong.  1) This certainly doesn't fix a double-free
bug because it doesn't remove a free.  2) gtk_widget_destroy() may
free cw->dialog, in which case it's wise to assign NULL.
gtk_widget_destroy should never free cw.  If it does, then something
is seriously wrong somewhere else, but not here.

-chris
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Derek Atkins
Chris Shoemaker <[hidden email]> writes:

> On Thu, Jun 02, 2005 at 11:10:48AM -0400, David Hampton wrote:
>> --- src/business/business-gnome/dialog-customer.c
>> +++ src/business/business-gnome/dialog-customer.c
>> @@ -383,7 +383,8 @@
>>    CustomerWindow *cw = user_data;
>>  
>>    gtk_widget_destroy (cw->dialog);
>> -  cw->dialog = NULL;
>> +  // cw has already been freed by this point.
>> +  // cw->dialog = NULL;
>>  }
>
> This patch looks wrong.  1) This certainly doesn't fix a double-free
> bug because it doesn't remove a free.  2) gtk_widget_destroy() may
> free cw->dialog, in which case it's wise to assign NULL.
> gtk_widget_destroy should never free cw.  If it does, then something
> is seriously wrong somewhere else, but not here.

It's certainly possible that gtk_widget_destroy() indirectly causes
the cw object to get destroyed.  But you're correct, this isn't
fixing a double-free.  It might fix a SEGV or memory corruption.

-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
       [hidden email]                        PGP key available
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

David Hampton-2
In reply to this post by Chris Shoemaker
On Thu, 2005-06-02 at 13:00 -0400, Chris Shoemaker wrote:

> On Thu, Jun 02, 2005 at 11:10:48AM -0400, David Hampton wrote:
> > --- src/business/business-gnome/dialog-customer.c
> > +++ src/business/business-gnome/dialog-customer.c
> > @@ -383,7 +383,8 @@
> >    CustomerWindow *cw = user_data;
> >  
> >    gtk_widget_destroy (cw->dialog);
> > -  cw->dialog = NULL;
> > +  // cw has already been freed by this point.
> > +  // cw->dialog = NULL;
> >  }
>
> This patch looks wrong.  1) This certainly doesn't fix a double-free
> bug because it doesn't remove a free.

You're right; my comment is wrong.  This change prevents writing to
already freed memory.  The problem was this particular write trashes the
memory free list such that libc rolls over and dies.

> 2) gtk_widget_destroy() may
> free cw->dialog, in which case it's wise to assign NULL.
> gtk_widget_destroy should never free cw.  If it does, then something
> is seriously wrong somewhere else, but not here.

Calling gtk_widget_destroy on this dialog eventually causes the
gnc_customer_window_destroy_cb function to be called.  That function is
where the free of the cw data structure occurs.

The pattern of having a destroy callback function that frees the data
structure associated with a window is common in the gnucash code.  It
allows one function to handle the cleanup of a dialog and any associated
data structures, no matter how the dialog was closed.  This is
particularly important because closing a window via the title bar close
button translates directly to a call to gtk_widget_destroy on the window
widget.  There's no other way for the code to know the window was closed
this way, other than to attach a callback to the widget destruction.

David


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Derek Atkins
Quoting David Hampton <[hidden email]>:

> The pattern of having a destroy callback function that frees the data
> structure associated with a window is common in the gnucash code.  It
> allows one function to handle the cleanup of a dialog and any associated
> data structures, no matter how the dialog was closed.  This is
> particularly important because closing a window via the title bar close
> button translates directly to a call to gtk_widget_destroy on the window
> widget.  There's no other way for the code to know the window was closed
> this way, other than to attach a callback to the widget destruction.

Speaking of which...  Using g2 cvs from yesterday, I clicked on the titlebar
close button and the window closed but the app didn't exit....  So I think
we're still missing some events.  There's a similar problem in one of the
business dialogs (the one that pops up when there are outstanding bills to be
paid) -- it doesn't close properly.

> David

-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
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Chris Shoemaker
In reply to this post by David Hampton-2
On Thu, Jun 02, 2005 at 01:26:59PM -0400, David Hampton wrote:

> On Thu, 2005-06-02 at 13:00 -0400, Chris Shoemaker wrote:
> > On Thu, Jun 02, 2005 at 11:10:48AM -0400, David Hampton wrote:
> > > --- src/business/business-gnome/dialog-customer.c
> > > +++ src/business/business-gnome/dialog-customer.c
> > > @@ -383,7 +383,8 @@
> > >    CustomerWindow *cw = user_data;
> > >  
> > >    gtk_widget_destroy (cw->dialog);
> > > -  cw->dialog = NULL;
> > > +  // cw has already been freed by this point.
> > > +  // cw->dialog = NULL;
> > >  }
> >
> Calling gtk_widget_destroy on this dialog eventually causes the
> gnc_customer_window_destroy_cb function to be called.  That function is
> where the free of the cw data structure occurs.
>
> The pattern of having a destroy callback function that frees the data
> structure associated with a window is common in the gnucash code.  It
> allows one function to handle the cleanup of a dialog and any associated
> data structures, no matter how the dialog was closed.  This is
> particularly important because closing a window via the title bar close
> button translates directly to a call to gtk_widget_destroy on the window
> widget.  There's no other way for the code to know the window was closed
> this way, other than to attach a callback to the widget destruction.

Thanks for explaining.  I haven't looked at the details, but shouldn't
destroy signal handler just generate the right CM event, and then the
CM close handler for the cw structure actually frees cw.  I thought
that was the intended use of the CM.

-chris
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Derek Atkins
Quoting Chris Shoemaker <[hidden email]>:

> Thanks for explaining.  I haven't looked at the details, but shouldn't
> destroy signal handler just generate the right CM event, and then the
> CM close handler for the cw structure actually frees cw.  I thought
> that was the intended use of the CM.

Yes, but all of that can happen before the gtk_widget_destroy() function
returns.

> -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
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Stewart Wright
In reply to this post by Derek Atkins
> Speaking of which...  Using g2 cvs from yesterday, I clicked on the titlebar
> close button and the window closed but the app didn't exit....  So I think
> we're still missing some events.  There's a similar problem in one of the
> business dialogs (the one that pops up when there are outstanding bills to be
> paid) -- it doesn't close properly.

Oh, the irony of being able to do this to you Derek...

Bug #301856


:-)


Cheers,

S.


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel

attachment0 (282 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Chris Shoemaker
In reply to this post by Derek Atkins
On Thu, Jun 02, 2005 at 01:44:29PM -0400, Derek Atkins wrote:

> Quoting Chris Shoemaker <[hidden email]>:
>
> > Thanks for explaining.  I haven't looked at the details, but shouldn't
> > destroy signal handler just generate the right CM event, and then the
> > CM close handler for the cw structure actually frees cw.  I thought
> > that was the intended use of the CM.
>
> Yes, but all of that can happen before the gtk_widget_destroy() function
> returns.
>

hmmm, I must be missing something.  Only a month without hacking GC
and I'm confused already.  Time to crack the source open again.  I've
been meaning to brush the dust off my budget patches anyway...

-chris
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Derek Atkins
Quoting Chris Shoemaker <[hidden email]>:

> On Thu, Jun 02, 2005 at 01:44:29PM -0400, Derek Atkins wrote:
> > Quoting Chris Shoemaker <[hidden email]>:
> >
> > > Thanks for explaining.  I haven't looked at the details, but shouldn't
> > > destroy signal handler just generate the right CM event, and then the
> > > CM close handler for the cw structure actually frees cw.  I thought
> > > that was the intended use of the CM.
> >
> > Yes, but all of that can happen before the gtk_widget_destroy() function
> > returns.
> >
>
> hmmm, I must be missing something.  Only a month without hacking GC
> and I'm confused already.  Time to crack the source open again.  I've
> been meaning to brush the dust off my budget patches anyway...

gtk_widget_destroy() "emits" the signal which calls the signal handler.  The
signal handler sets emits the CM event which calls the dialog CM destroy
handler which destroys the dialog context.   These are all function calls,
unless the CM has events turned off in which case the event will be cached for
later.

The events are not asynchronous.  Or at least, they don't HAVE to be..  They CAN
be synchronous and cause real-time function calls.  This means the CM event
handler can be called in the middle of the gtk_widget_destroy(), so the dialog
context can be destroyed before the gtk_widget_destroy() returns.

Does that make more sense?

> -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
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Chris Shoemaker
On Thu, Jun 02, 2005 at 04:01:27PM -0400, Derek Atkins wrote:

> Quoting Chris Shoemaker <[hidden email]>:
>
> > On Thu, Jun 02, 2005 at 01:44:29PM -0400, Derek Atkins wrote:
> > > Quoting Chris Shoemaker <[hidden email]>:
> > >
> > > > Thanks for explaining.  I haven't looked at the details, but shouldn't
> > > > destroy signal handler just generate the right CM event, and then the
> > > > CM close handler for the cw structure actually frees cw.  I thought
> > > > that was the intended use of the CM.
> > >
> > > Yes, but all of that can happen before the gtk_widget_destroy() function
> > > returns.
> > >
> >
> > hmmm, I must be missing something.  Only a month without hacking GC
> > and I'm confused already.  Time to crack the source open again.  I've
> > been meaning to brush the dust off my budget patches anyway...
>
> gtk_widget_destroy() "emits" the signal which calls the signal handler.  The
> signal handler sets emits the CM event which calls the dialog CM destroy
> handler which destroys the dialog context.  

Yes.  That's how it should work, but that's not what the code does at all.

1) The gtk_destroy signal handler (gnc_customer_window_destroy_cb) does
NOT emit the CM event.  
(It should be calling gnc_close_gui_component(cw->component_id);)

2) The CM destroy handler (gnc_customer_window_close_handler) does NOT
destroy the dialog context.  Instead, that's actually done inside the
gtk_destroy signal handler.


The biggest hint that this is an abuse of the CM api is the fact that
the close handler is not explicitly unregistering the component with
the CM, instead relying on it happening as a side-effect of the
gtk_destroy signal.  If the close handler wanted to do anything with
the component after it destroyed the dialog (like, say, read the
non-gui state) it would be screwed because the whole component has
been freed!

I think all the confusion goes away when you do something like this:
[warning: not even compile tested]

// This is the gtk_destroy signal handler
void
gnc_customer_window_destroy_cb (GtkWidget *widget, gpointer data)
{
  CustomerWindow *cw = data;

  gnc_close_gui_component(cw->component_id);

}

// This is the CM close handler
static void
gnc_customer_window_close_handler (gpointer user_data)
{
  CustomerWindow *cw = user_data;
  GncCustomer *customer = cw_get_customer (cw);

  gnc_suspend_gui_refresh ();

  if (cw->dialog_type == NEW_CUSTOMER && customer != NULL) {
    gncCustomerBeginEdit (customer);
    gncCustomerDestroy (customer);
    cw->customer_guid = *xaccGUIDNULL ();
  }

  gnc_unregister_gui_component (cw->component_id);

  gtk_widget_destroy (cw->dialog);
  cw->dialog = NULL;
  g_free (cw);

  gnc_resume_gui_refresh ();
}

Makes a heck of a lot more sense to me.  David?

-chris
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

David Hampton-2
On Thu, 2005-06-02 at 16:59 -0400, Chris Shoemaker wrote:

> 1) The gtk_destroy signal handler (gnc_customer_window_destroy_cb) does
> NOT emit the CM event.  
> (It should be calling gnc_close_gui_component(cw->component_id);)
>
> 2) The CM destroy handler (gnc_customer_window_close_handler) does NOT
> destroy the dialog context.  Instead, that's actually done inside the
> gtk_destroy signal handler.
>
>
> The biggest hint that this is an abuse of the CM api is the fact that
> the close handler is not explicitly unregistering the component with
> the CM, instead relying on it happening as a side-effect of the
> gtk_destroy signal.  If the close handler wanted to do anything with
> the component after it destroyed the dialog (like, say, read the
> non-gui state) it would be screwed because the whole component has
> been freed!
>
> I think all the confusion goes away when you do something like this:
> [warning: not even compile tested]
>
> // This is the gtk_destroy signal handler
> void
> gnc_customer_window_destroy_cb (GtkWidget *widget, gpointer data)
> {
>   CustomerWindow *cw = data;
>
>   gnc_close_gui_component(cw->component_id);
>
> }
>
> // This is the CM close handler
> static void
> gnc_customer_window_close_handler (gpointer user_data)
> {
>   CustomerWindow *cw = user_data;
>   GncCustomer *customer = cw_get_customer (cw);
>
>   gnc_suspend_gui_refresh ();
>
>   if (cw->dialog_type == NEW_CUSTOMER && customer != NULL) {
>     gncCustomerBeginEdit (customer);
>     gncCustomerDestroy (customer);
>     cw->customer_guid = *xaccGUIDNULL ();
>   }
>
>   gnc_unregister_gui_component (cw->component_id);
>
>   gtk_widget_destroy (cw->dialog);
>   cw->dialog = NULL;
>   g_free (cw);
>
>   gnc_resume_gui_refresh ();
> }
>
> Makes a heck of a lot more sense to me.  David?

I think this will work because gtk_widget_destroy correctly handles
recursive invocation.  If the widget is already in the process of being
destroyed then the call to gtk_widget_destroy from
gnc_customer_window_close_handler is a noop, otherwise it will actually
destroy the dialog.

David


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Derek Atkins
Quoting David Hampton <[hidden email]>:

>
> I think this will work because gtk_widget_destroy correctly handles
> recursive invocation.  If the widget is already in the process of being
> destroyed then the call to gtk_widget_destroy from
> gnc_customer_window_close_handler is a noop, otherwise it will actually
> destroy the dialog.

What if the gtk_widget_destroy() finishes and returns before the CM callback
happens?  For example, if the CM events are suspended, the CM system will cache
the event and then return, causing gtk_widget_destroy() to finish.  Then later
when the events are resumed it will call the CM callback which will call
gtk_widget_destroy() on the already-freed widget.  Wouldn't that be a problem?

> David

-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
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

David Hampton-2
On Thu, 2005-06-02 at 17:33 -0400, Derek Atkins wrote:

> Quoting David Hampton <[hidden email]>:
>
> >
> > I think this will work because gtk_widget_destroy correctly handles
> > recursive invocation.  If the widget is already in the process of being
> > destroyed then the call to gtk_widget_destroy from
> > gnc_customer_window_close_handler is a noop, otherwise it will actually
> > destroy the dialog.
>
> What if the gtk_widget_destroy() finishes and returns before the CM callback
> happens?  For example, if the CM events are suspended, the CM system will cache
> the event and then return, causing gtk_widget_destroy() to finish.  Then later
> when the events are resumed it will call the CM callback which will call
> gtk_widget_destroy() on the already-freed widget.  Wouldn't that be a problem?

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.

David


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Derek Atkins
I should point out that the Account Window seems to have the exact same code
path as the customer dialog.  If there really is a missing code-path then we
probably need to rototill the code to get it all working "right".

-derek

Quoting David Hampton <[hidden email]>:

> On Thu, 2005-06-02 at 17:33 -0400, Derek Atkins wrote:
> > Quoting David Hampton <[hidden email]>:
> >
> > >
> > > I think this will work because gtk_widget_destroy correctly handles
> > > recursive invocation.  If the widget is already in the process of being
> > > destroyed then the call to gtk_widget_destroy from
> > > gnc_customer_window_close_handler is a noop, otherwise it will actually
> > > destroy the dialog.
> >
> > What if the gtk_widget_destroy() finishes and returns before the CM
> callback
> > happens?  For example, if the CM events are suspended, the CM system will
> cache
> > the event and then return, causing gtk_widget_destroy() to finish.  Then
> later
> > when the events are resumed it will call the CM callback which will call
> > gtk_widget_destroy() on the already-freed widget.  Wouldn't that be a
> problem?
>
> 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.
>
> David
>
>
>


--
       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
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Chris Shoemaker
In reply to this post by David Hampton-2
On Thu, Jun 02, 2005 at 05:29:06PM -0400, David Hampton wrote:

> On Thu, 2005-06-02 at 16:59 -0400, Chris Shoemaker wrote:
> > 1) The gtk_destroy signal handler (gnc_customer_window_destroy_cb) does
> > NOT emit the CM event.  
> > (It should be calling gnc_close_gui_component(cw->component_id);)
> >
> > 2) The CM destroy handler (gnc_customer_window_close_handler) does NOT
> > destroy the dialog context.  Instead, that's actually done inside the
> > gtk_destroy signal handler.
> >
> >
> > The biggest hint that this is an abuse of the CM api is the fact that
> > the close handler is not explicitly unregistering the component with
> > the CM, instead relying on it happening as a side-effect of the
> > gtk_destroy signal.  If the close handler wanted to do anything with
> > the component after it destroyed the dialog (like, say, read the
> > non-gui state) it would be screwed because the whole component has
> > been freed!
> >
> > I think all the confusion goes away when you do something like this:
> > [warning: not even compile tested]
> >
> > // This is the gtk_destroy signal handler
> > void
> > gnc_customer_window_destroy_cb (GtkWidget *widget, gpointer data)
> > {
> >   CustomerWindow *cw = data;
> >
> >   gnc_close_gui_component(cw->component_id);
> >
> > }
> >
> > // This is the CM close handler
> > static void
> > gnc_customer_window_close_handler (gpointer user_data)
> > {
> >   CustomerWindow *cw = user_data;
> >   GncCustomer *customer = cw_get_customer (cw);
> >
> >   gnc_suspend_gui_refresh ();
> >
> >   if (cw->dialog_type == NEW_CUSTOMER && customer != NULL) {
> >     gncCustomerBeginEdit (customer);
> >     gncCustomerDestroy (customer);
> >     cw->customer_guid = *xaccGUIDNULL ();
> >   }
> >
> >   gnc_unregister_gui_component (cw->component_id);
> >
> >   gtk_widget_destroy (cw->dialog);
> >   cw->dialog = NULL;
> >   g_free (cw);
> >
> >   gnc_resume_gui_refresh ();
> > }
> >
> > Makes a heck of a lot more sense to me.  David?
>
> I think this will work because gtk_widget_destroy correctly handles
> recursive invocation.  If the widget is already in the process of being
> destroyed then the call to gtk_widget_destroy from
> gnc_customer_window_close_handler is a noop, otherwise it will actually
> destroy the dialog.

That makes sense since "gtk_widget_destroy" is supposed to mean "break
all your references".  If they're already broken, re-calling does
nothing.  

I'm not sure if the destroy-signal will get re-delivered in the
recursive case, but I would hope that it wouldn't matter since I would
expect the CM to ignore any events after the un-registration, whether
the events are cached and then discarded because they arrived after
the unregistration, or were discarded immediately because the
component had already been unregistered.

-chris
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Derek Atkins
Quoting Chris Shoemaker <[hidden email]>:

> On Thu, Jun 02, 2005 at 05:29:06PM -0400, David Hampton wrote:
> > On Thu, 2005-06-02 at 16:59 -0400, Chris Shoemaker wrote:
> > > 1) The gtk_destroy signal handler (gnc_customer_window_destroy_cb) does
> > > NOT emit the CM event.  
> > > (It should be calling gnc_close_gui_component(cw->component_id);)
> > >
> > > 2) The CM destroy handler (gnc_customer_window_close_handler) does NOT
> > > destroy the dialog context.  Instead, that's actually done inside the
> > > gtk_destroy signal handler.
> > >
> > >
> > > The biggest hint that this is an abuse of the CM api is the fact that
> > > the close handler is not explicitly unregistering the component with
> > > the CM, instead relying on it happening as a side-effect of the
> > > gtk_destroy signal.  If the close handler wanted to do anything with
> > > the component after it destroyed the dialog (like, say, read the
> > > non-gui state) it would be screwed because the whole component has
> > > been freed!
> > >
> > > I think all the confusion goes away when you do something like this:
> > > [warning: not even compile tested]
> > >
> > > // This is the gtk_destroy signal handler
> > > void
> > > gnc_customer_window_destroy_cb (GtkWidget *widget, gpointer data)
> > > {
> > >   CustomerWindow *cw = data;
> > >
> > >   gnc_close_gui_component(cw->component_id);
> > >
> > > }
> > >
> > > // This is the CM close handler
> > > static void
> > > gnc_customer_window_close_handler (gpointer user_data)
> > > {
> > >   CustomerWindow *cw = user_data;
> > >   GncCustomer *customer = cw_get_customer (cw);
> > >
> > >   gnc_suspend_gui_refresh ();
> > >
> > >   if (cw->dialog_type == NEW_CUSTOMER && customer != NULL) {
> > >     gncCustomerBeginEdit (customer);
> > >     gncCustomerDestroy (customer);
> > >     cw->customer_guid = *xaccGUIDNULL ();
> > >   }
> > >
> > >   gnc_unregister_gui_component (cw->component_id);
> > >
> > >   gtk_widget_destroy (cw->dialog);
> > >   cw->dialog = NULL;
> > >   g_free (cw);
> > >
> > >   gnc_resume_gui_refresh ();
> > > }
> > >
> > > Makes a heck of a lot more sense to me.  David?
> >
> > I think this will work because gtk_widget_destroy correctly handles
> > recursive invocation.  If the widget is already in the process of being
> > destroyed then the call to gtk_widget_destroy from
> > gnc_customer_window_close_handler is a noop, otherwise it will actually
> > destroy the dialog.
>
> That makes sense since "gtk_widget_destroy" is supposed to mean "break
> all your references".  If they're already broken, re-calling does
> nothing.  
>
> I'm not sure if the destroy-signal will get re-delivered in the
> recursive case, but I would hope that it wouldn't matter since I would
> expect the CM to ignore any events after the un-registration, whether
> the events are cached and then discarded because they arrived after
> the unregistration, or were discarded immediately because the
> component had already been unregistered.

It all depends which API gets called first.  If the user clicks the window
manager close button, it winds up calling gtk_widget_destroy() first, which
partially destroys the widget and calls the callback, which would call the CM,
which calls the CM callback, which would re-call the gtk_widget_destroy().

Basically, we need to handle re-entrancy in BOTH apis.  They are necessarily
tied together, which is why I don't consider it bad that we're assuming a
side-effect.

> -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
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Chris Shoemaker
In reply to this post by David Hampton-2
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:
> > Quoting David Hampton <[hidden email]>:
> >
> > >
> > > I think this will work because gtk_widget_destroy correctly handles
> > > recursive invocation.  If the widget is already in the process of being
> > > destroyed then the call to gtk_widget_destroy from
> > > gnc_customer_window_close_handler is a noop, otherwise it will actually
> > > destroy the dialog.
> >
> > What if the gtk_widget_destroy() finishes and returns before the CM callback
> > happens?  For example, if the CM events are suspended, the CM system will cache
> > the event and then return, causing gtk_widget_destroy() to finish.  Then later
> > when the events are resumed it will call the CM callback which will call
> > gtk_widget_destroy() on the already-freed widget.  Wouldn't that be a problem?
>
> 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.

-chris
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Chris Shoemaker
In reply to this post by Derek Atkins
On Thu, Jun 02, 2005 at 10:11:11PM -0400, Derek Atkins wrote:

> Quoting Chris Shoemaker <[hidden email]>:
>
> > On Thu, Jun 02, 2005 at 05:29:06PM -0400, David Hampton wrote:
> > > On Thu, 2005-06-02 at 16:59 -0400, Chris Shoemaker wrote:
> > > > 1) The gtk_destroy signal handler (gnc_customer_window_destroy_cb) does
> > > > NOT emit the CM event.  
> > > > (It should be calling gnc_close_gui_component(cw->component_id);)
> > > >
> > > > 2) The CM destroy handler (gnc_customer_window_close_handler) does NOT
> > > > destroy the dialog context.  Instead, that's actually done inside the
> > > > gtk_destroy signal handler.
> > > >
> > > >
> > > > The biggest hint that this is an abuse of the CM api is the fact that
> > > > the close handler is not explicitly unregistering the component with
> > > > the CM, instead relying on it happening as a side-effect of the
> > > > gtk_destroy signal.  If the close handler wanted to do anything with
> > > > the component after it destroyed the dialog (like, say, read the
> > > > non-gui state) it would be screwed because the whole component has
> > > > been freed!
> > > >
> > > > I think all the confusion goes away when you do something like this:
> > > > [warning: not even compile tested]
> > > >
> > > > // This is the gtk_destroy signal handler
> > > > void
> > > > gnc_customer_window_destroy_cb (GtkWidget *widget, gpointer data)
> > > > {
> > > >   CustomerWindow *cw = data;
> > > >
> > > >   gnc_close_gui_component(cw->component_id);
> > > >
> > > > }
> > > >
> > > > // This is the CM close handler
> > > > static void
> > > > gnc_customer_window_close_handler (gpointer user_data)
> > > > {
> > > >   CustomerWindow *cw = user_data;
> > > >   GncCustomer *customer = cw_get_customer (cw);
> > > >
> > > >   gnc_suspend_gui_refresh ();
> > > >
> > > >   if (cw->dialog_type == NEW_CUSTOMER && customer != NULL) {
> > > >     gncCustomerBeginEdit (customer);
> > > >     gncCustomerDestroy (customer);
> > > >     cw->customer_guid = *xaccGUIDNULL ();
> > > >   }
> > > >
> > > >   gnc_unregister_gui_component (cw->component_id);
> > > >
> > > >   gtk_widget_destroy (cw->dialog);
> > > >   cw->dialog = NULL;
> > > >   g_free (cw);
> > > >
> > > >   gnc_resume_gui_refresh ();
> > > > }
> > > >
> > > > Makes a heck of a lot more sense to me.  David?
> > >
> > > I think this will work because gtk_widget_destroy correctly handles
> > > recursive invocation.  If the widget is already in the process of being
> > > destroyed then the call to gtk_widget_destroy from
> > > gnc_customer_window_close_handler is a noop, otherwise it will actually
> > > destroy the dialog.
> >
> > That makes sense since "gtk_widget_destroy" is supposed to mean "break
> > all your references".  If they're already broken, re-calling does
> > nothing.  
> >
> > I'm not sure if the destroy-signal will get re-delivered in the
> > recursive case, but I would hope that it wouldn't matter since I would
> > expect the CM to ignore any events after the un-registration, whether
> > the events are cached and then discarded because they arrived after
> > the unregistration, or were discarded immediately because the
> > component had already been unregistered.
>
> It all depends which API gets called first.  If the user clicks the window
> manager close button, it winds up calling gtk_widget_destroy() first, which
> partially destroys the widget and calls the callback, which would call the CM,
> which calls the CM callback, which would re-call the gtk_widget_destroy().
                                                       ^^^^^^^^^^^^^^^^^^

Um, yeah, but my paragraph above was pondering what happens after all
that.  David said that gtk_widget_destroy would block the reentance,
but I didn't know if that also means block the re-delivery of the
signal after the last step in the sequence you just described.

>
> Basically, we need to handle re-entrancy in BOTH apis.  They are necessarily
> tied together, which is why I don't consider it bad that we're assuming a
> side-effect.

Well, hopefully we can leave worrying about gtk_destroy re-entrency to
the gtk developers.  What we need to do is use the signal as it's
meant to be used.  The "destroy" signal handler should break external
references.

IMO, making the GTK destroy signal mean "free my component struct",
while making the ComponentManager's close signal mean "drop the
dialog's references" _is_ bad.  I recognize that it probably doesn't
bother you, but that's just because you're so intelligent.  Average
developers like me really benefit from semantic consistency.  :)

-chris

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Chris Shoemaker
In reply to this post by Chris Shoemaker
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.

-chris
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

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

Derek Atkins
Quoting Chris Shoemaker <[hidden email]>:

> 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
       [hidden email]                        PGP key available

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
123