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@xxxxxxxxxx>
> > > ---
> > > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
