[libvirt] [PATCH 7/7] util: use glib allocation functions

[Daniel P. Berrangé]

On Mon, Sep 02, 2019 at 04:00:36PM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 02, 2019 at 04:52:47PM +0200, Ján Tomko wrote:
> > On Thu, Aug 29, 2019 at 07:02:50PM +0100, Daniel P. Berrangé wrote:
> > > GLib requires that any memory allocated with g_new/g_malloc/etc
> > > is free'd by g_free. This means mixing virAlloc with g_free,
> > > or g_new with virFree will violate API rules. The easy way to
> > > dea with this is to simply make our allocation functions wrappers
> > 
> > s/dea/deal/
> > 
> > > to the glib functions.
> > > 
> > > Use of our allocation functions can be phased out gradually
> > > over time. When doing such conversions, the return values checks
> > > can be dropped at the same time.
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > > ---
> > > src/util/viralloc.c | 29 ++++++-----------------------
> > > 1 file changed, 6 insertions(+), 23 deletions(-)
> > > 
> > 
> > I'm afraid this is incomplete.
> > 
> > The obvious mismatch being VIR_STRDUP calling strdup while
> > VIR_FREE would call g_free now.
> 
> Opps, yes.
> 
> > Same with (v)asprintf. We also use VIR_FREE to free memory
> > allocated by external libraries (e.g. virXMLPropString; while
> > there is a xmlFree function which is just an alias for free,
> > we never use it in libvirt code)
> 
> Hmm, not using xmlFree is arguably a bug in libvirt code.
> 
> Most other libs quite probably mandate plain 'free'.

The other really big thing is that some of our public APIs
return an allocated 'char *'  that we document the application
must call 'free()' on. We cannot change our public API contract
in this respect.

> 
> > Maybe the safer approach would be to convert the code gradually
> > and explicitly use g_alloc and g_free/g_autoptr without the wrappers?
> 
> I'm not convince that would be especially safe - the caller of any
> API which returns an allocated string now checks to look at the impl
> to see how it was allocated - with deep call chains this gets quite
> error prone I think.
> 
> What's interesting is that the glib docs says
> 
>  * It's important to match g_malloc() (and wrappers such as g_new()) with
>  * g_free(), g_slice_alloc() (and wrappers such as g_slice_new()) with
>  * g_slice_free(), plain malloc() with free(), and (if you're using C++)
>  * new with delete and new[] with delete[]. Otherwise bad things can happen,
>  * since these allocators may use different memory pools (and new/delete call
>  * constructors and destructors).
> 
> but the actual code just does
> 
> gpointer
> g_malloc (gsize n_bytes)
> {
>   if (G_LIKELY (n_bytes))
>     {
>       gpointer mem;
> 
>       mem = malloc (n_bytes);
>       TRACE (GLIB_MEM_ALLOC((void*) mem, (unsigned int) n_bytes, 0, 0));
>       if (mem)
>         return mem;
> 
>       g_error ("%s: failed to allocate %"G_GSIZE_FORMAT" bytes",
>                G_STRLOC, n_bytes);
>     }
> 
>   TRACE(GLIB_MEM_ALLOC((void*) NULL, (int) n_bytes, 0, 0));
> 
>   return NULL;
> }
> 
> void
> g_free (gpointer mem)
> {
>   free (mem);
>   TRACE(GLIB_MEM_FREE((void*) mem));
> }
> 
> IOW, it is entirely safe to mix free + g_free today - looks like the warning
> is just talking about a hypothetical risk that doesn't actually exist right
> now, at least for free vs g_free.

It appears this was a change in GLib 2.46.

Before this time they allowed for a custom malloc impl to be registered.
In 2.46 this feature was dropped and the system malloc hardcoded:

  https://gitlab.gnome.org/GNOME/glib/commit/3be6ed6

The docs were not fully updated though.

So the only thing that 'g_free' does differently is to emit systemtap
trace events.


> The g_slice warning is definitely relevant though since that uses a pool
> allocator
> 
> So I'm inclined to fix the mistakes in this patch, and then just deal with
> using plain 'free' for any cases we deal with external libraries on best
> effort, in the belief that it doesnt' actually matter for glib anyway.

I think I'm going to propose a docs update to glib to make it explicit
that you are permitted to use g_new + free() and see what their maintainers
say about that - whether they'll guarantee this forever more.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|