Re: [evolution-patches] Freeing a xmlGetProp() return value with g_free()

[Jeffrey Stedfast <fejj novell com>]

indeed, this is how we solved it in other places in evo too. somehow
this instance must have been overlooked

Jeff

On Tue, 2005-12-27 at 12:53 +0200, Tor Lillqvist wrote:
> evo/e-util/e-menu.c/emph_construct_menu() assigns tmp =
> xmlGetProp("file"), then  assigns ui->filename = tmp.
> 
> In emph_free_ui() it then calls g_free(ui->filename).
> 
> This is wrong, isn't it? The value returned by xmlGetProp() should be
> freed with xmlFree(). It just happens to work most of the time to use
> g_free(), as xmlGetProp() normally returns a malloc()ed result, and
> g_free() normally just calls free(). But this is not necessarily always
> the case.
> 
> (To catch things like this, it would be so much better if for instance
> the libxml2 functions didn't return the exact pointers that malloc() has
> returned, but purposedly allocated, say, four extra bytes and returned a
> correspondingly offset pointer. (xmlFree() would then of course have to
> correspondingly de-offset what it passes to free().) Then mixups like
> this would be more likely to cause serious problems and be noticed
> earlier.)
> 
> I guess it's cleanest here to use g_strdup(tmp) in the assignment to
> ui->filename, and call xmlFree(tmp) immediately afterwards, and leave
> the g_free() in emph_free_ui() as is.
> 
> I noticed this while I was adding Win32 installation relocatability for
> the UI file names.
> 
> --tml
> 
> 
> plain text document attachment (1.txt), ""
> Index: e-util/e-menu.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/e-util/e-menu.c,v
> retrieving revision 1.9
> diff -p -u -2 -r1.9 e-menu.c
> --- e-util/e-menu.c	26 Nov 2005 03:51:16 -0000	1.9
> +++ e-util/e-menu.c	27 Dec 2005 10:34:43 -0000
> @@ -30,11 +30,12 @@
>  #include <glib.h>
>  
> -#include "e-menu.h"
> -
> -#include <e-util/e-icon-factory.h>
> -
>  #include <libgnome/gnome-i18n.h>
>  #include <bonobo/bonobo-ui-util.h>
>  
> +#include <libedataserver/e-util.h>
> +
> +#include "e-menu.h"
> +#include "e-icon-factory.h"
> +
>  #define d(x)
>  
> @@ -785,5 +786,14 @@ emph_construct_menu(EPluginHook *eph, xm
>  				EMenuUIFile *ui = g_malloc0(sizeof(*ui));
>  
> -				ui->filename = tmp;
> +				ui->filename = g_strdup(tmp);
> +				xmlFree(tmp);
> +#ifdef G_OS_WIN32
> +				{
> +					char *mapped_location = e_util_replace_prefix (e_util_get_prefix (),
> +										       ui->filename);
> +					g_free (ui->filename);
> +					ui->filename = mapped_location;
> +				}
> +#endif
>  				ui->appdir = g_strdup(g_get_tmp_dir());
>  				ui->appname = g_strdup("Evolution");
> _______________________________________________
> Evolution-patches mailing list
> Evolution-patches gnome org
> http://mail.gnome.org/mailman/listinfo/evolution-patches
-- 
Jeffrey Stedfast
Evolution Hacker - Novell, Inc.
fejj ximian com  - www.novell.com