273337 - memory from gtk widgets should be allocated using gmalloc

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect)

Description

[Todd A. Fisher]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; chrome://navigator/locale/navigator.properties; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; chrome://navigator/locale/navigator.properties; rv:1.7.5) Gecko/20041107 Firefox/1.0

many functions in gtkmozembed return memory that is allocated with the standard
c functions.  This is ok but could be a serious problem if a programmer decided
to use alternate gmalloc functions and then accidentally called g_free on the
memory returned from a gtkmozembed function.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Todd A. Fisher]

Attachment: Replaces strdup calls with g_strdup

Comment 2

[Marco Pesenti Gritti]

As far as I know, it's ok to free memory allocated by strdup and malloc using g_free

Comment 3

[Todd A. Fisher]

I do not believe that is the case.  A user can use the following api to override
g_malloc and g_free. If i'm not mistaken this function does not override malloc
and free, making it unsafe to mix the two.

see: from http://developer.gnome.org/doc/API/2.0/glib/glib-Memory-Allocation.html
void        g_mem_set_vtable                (GMemVTable *vtable);
gboolean    g_mem_is_system_malloc          (void);

also, looking at glibs memory function implementation g_malloc for instance:
uses the function defined in vtable so unless that function is compatible with
the standard libraries free then it is absolutely not safe.

Comment 4

[Brian Ryner (not reading)]

Comment on attachment 167994 [details] [diff] [review]
Replaces strdup calls with g_strdup

good to know... we're probably violating this in other places as well.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Fix checked in for 1.8b.

Todd, in general you want to request review for your patches (see
http://www.mozilla.org/hacking/life-cycle.html and
http://www.mozilla.org/owners.html) and when they have reviews it's a good idea
to ping someone to check them in.  Keeps them from getting lost.  ;)

Comment 6

[Christopher Blizzard (:blizzard)]

This is the same as an API change.  I would have rathered that this not have
been checked in.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Chris, it's your module, so it's your call.  If we clearly document that the
return values of certain functions must be deallocated with a certain allocator,
then we can revert this patch, of course...

Comment 8

[Christian Persch (GNOME) (away; not receiving bug mail)]

I've looked at epiphany, galeon, yelp and devhelp (all gecko embedders I could
find in GNOME), and all assume that you can free the return values with g_free.
That suggests that this is a widespread assumption.

Comment 9

[Christopher Blizzard (:blizzard)]

It's a mistake, sure.  But fixing it at this point means that everyone has to
update their clients.  The cost/benefit ratio here seems low to me.