Re: GTK, Purify, some patches, etc.

[Tim Janik <timj gtk org>]

lars,

please take a look at the portions marked with LARS:


On Tue, 16 Mar 1999, Bruce Mitchener, Jr. wrote:

> Hello.
> 
> I'm currently running Purify against Mozilla and have found some memory
> errors in GTK.  Those errors I will report after some further investigation.
> I've gone further and am running Purify against testgtk and have a couple of
> small patches.  There are additional problems that I haven't yet had the
> opportunity to look into, but will try and send out descriptions of the
> problems.  Some of them may be specific to Solaris.
> 
> Index: gtkitemfactory.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkitemfactory.c,v
> retrieving revision 1.23
> diff -u -r1.23 gtkitemfactory.c
> --- gtkitemfactory.c 1999/03/15 00:03:22 1.23
> +++ gtkitemfactory.c 1999/03/17 05:46:59
> @@ -684,6 +684,7 @@
>    for (slist = ifactory->widgets_by_action; slist; slist = slist->next)
>      g_free (slist->data);
>    g_slist_free (ifactory->widgets_by_action);
> +  g_free(ifactory->path);
  ^^^^^^^^^

this is bogus (i've had such a report before), ifactory->path is freed in
gtk_item_factory_finalize() and shouldn't in gtk_item_factory_destroy().
if you encounter this as a leak you most probably don't get the item factory
finalized appropriatedly which means it is still around with a ref_count >= 1
and you've got leaks much bigger than ifactory->path.
despite that, object fields that are free()d in a _destroy handler should get
reset to NULL afterwards to avoid further referencing of freed memory.

>    ifactory->widgets_by_action = NULL;
> 
>    parent_class->destroy (object);
> 
> Index: gtktipsquery.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtktipsquery.c,v
> retrieving revision 1.16
> diff -u -r1.16 gtktipsquery.c
> --- gtktipsquery.c 1999/02/24 07:36:38 1.16
> +++ gtktipsquery.c 1999/03/17 05:46:40
> @@ -257,6 +257,9 @@
> 
>    gtk_tips_query_set_caller (tips_query, NULL);
> 
> +  g_free(tips_query->label_inactive);
> +  g_free(tips_query->label_no_tip);
> +

accompanied by before mentioned NULL setting this one is a real memory leak
fix, thanx.

>    if (GTK_OBJECT_CLASS (parent_class)->destroy)
>      (* GTK_OBJECT_CLASS (parent_class)->destroy) (object);
>  }
> 
> 
> In gdk_xpm_destroy_notify(), it has a local variable 'color' of type
> GdkColor.  It passes this to gdk_colormap_free_colors() which passes it to
> g_hash_table_remove() and then to gdk_color_equal().  In gdk_color_equal(),
> it attempts to read the fields 'red', 'green' and 'blue' on the color, but
> they were never initialized and Purify gives an unitialized memory read
> error in gdk_color_equal().

should be fixed in CVS as well.

> When running testgtk, grabbing a target in the 'Selection Test' and dragging
> it yields an uninitialized memory error in gtk_list_motion_notify().  This
> appears to be the reading of event->window in that code.  Should it be
> initialized to event->window = list->window along with the other
> initializations for 'event' in gtk_list_vertical_timeout()?

hm, with the current setup the test for event->window == list->window will
definitely fail, i thusly initialized the event->window with NULL and
appropriatedly filled in the other portions of the even, such as type and
send_event. the same applies for gtk_list_horizontal_timeout() btw.

LARS: Lars, could you furtherly look into this?


> That same test also appears to leak 4 bytes of memory.  That memory was
> allocated in XGetWindowProperty() as called by gdk_selection_property_get()
> (which was called by gtk_selection_notify()). Actually, this was a 1 byte
> leak, which happened 4 times during my testing of that test.  Looking again,
> it leaks 1 byte on each click of the 'Get Targets' button.

uhm, here a char** pointer is filled and the code to free it actually says
  if (t)
    {
      t = NULL;
      XFree (t);
    }
which should read:
  if (t)
    {
      XFree (t);
      t = NULL;
    }


> gdk_init() on my system ends up calling gdk_im_open().  However, gdk_exit()
> doesn't call gdk_im_close().  Should it?

uhm, no, if we wanted to free all allocated memory in gdk_exit(), we'd
have a lot to do and practically no benefit as all program resources will
be freed anyways by a call to exit() or _exit() (module special things
like shared memory segemtns that we take care of appropriatedly of course).
besides, i doubt that gdk_exit() is used in more then 1% of actuall gdk/gtk
applications out there.

> When doing the 'Layout' test, it appeared to leak from
> gtk_layout_set_adjustments(), where it called and allocated
> gtk_adjustment_new().

yeah, gtklayout lacks a _finalize implementation which should
decrement the adjustment's ref counts, this is can easily be discovered
with GTK_DEBUG=objects or --gtk-debug=objects as well.

> In the 'clist' test, when called by 'create_clist()' and
> 'insert_row_clist()' from testgtk, there uninitialized memory read errors
> from the 'requisition' variable in gtk_clist_set_cell_style() where it has a
> value read from it and passed to column_auto_resize().

hm eventually we should always fill &requisition in this place...

LARS: ?

> the font selection test should g_free() the result of the call to
> gtk_font_selection_dialog_get_font_name().  Currently, it leaks (just to
> provide a good example).  gtk_clist_set_shift() has the same uninitialized
> memory read as gtk_clist_set_cell_style() as described above, when called by
> gtk_font_selection_init().

jup.

> testgtk doesn't initialize the variable 'transparent' for the ctree test, so
> uninitialized memory read errors occur in gdk_gc_set_foreground() where this
> value is read.  What is a valid value for transparent here?

{ 0 }

> The 'color selection' test had the most problems.  When bringing it up and
> then hitting 'cancel', some very bad things happened. a 'gtk_style' has what
> seems to be a refcounting issue.  It is freed multiple times and a lot of
> 'free memory read', 'free memory write' and 'freeing unallocated memory'
> errors occurred in 'gtk_style_unref()'.

uhm, these can't hardly be tracked without further information, in any case
i've added extra assertments to gtk_style_[un]ref () for the ref_count being
greater than 0. but even with that, i'm not able to trigger any abnormal
style [un]referencing, and the color selection itself doesn't muck around with
styles internally.


> That's all of the errors, excepting some that appear to be OS-specific that
> I noticed via Purifying the 'testgtk' application.

i guess you could save yourself some work by not tracking down memory leaks
that are related to stale objects, e.g. configure glib and gtk with
--enable-debug=yes (only required on stable branches) and put something like
export GTK_DEBUG=objects GDK_DEBUG=misc
into your ~/.profile.

> The more serious of the OS-specific errors is the apparent
> multiple-destruction of input contexts used for any text entry widgets.  I'm
> looking to confirm whether this is a Solaris 2.6 bug or a GTK bug yet.

ok, thanx. i guess owen will be furtherly interested in those, i know he
loves IC issues on solaris ;)

> 
>  - Bruce
> 

---
ciaoTJ