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
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@redhat.com>
---
src/util/viralloc.c | 29 ++++++-----------------------
1 file changed, 6 insertions(+), 23 deletions(-)
diff --git a/src/util/viralloc.c b/src/util/viralloc.c
index b74f657733..b50d989415 100644
--- a/src/util/viralloc.c
+++ b/src/util/viralloc.c
@@ -45,10 +45,7 @@ VIR_LOG_INIT("util.alloc");
int virAlloc(void *ptrptr,
size_t size)
{
- *(void **)ptrptr = calloc(1, size);
- if (*(void **)ptrptr == NULL)
- abort();
-
+ *(void **)ptrptr = g_malloc0(size);
return 0;
}
@@ -69,10 +66,7 @@ int virAllocN(void *ptrptr,
size_t size,
size_t count)
{
- *(void**)ptrptr = calloc(count, size);
- if (*(void**)ptrptr == NULL)
- abort();
-
+ *(void**)ptrptr = g_malloc0_n(count, size);
return 0;
}
@@ -94,16 +88,7 @@ int virReallocN(void *ptrptr,
size_t size,
size_t count)
{
- void *tmp;
-
- if (xalloc_oversized(count, size))
- abort();
-
- tmp = realloc(*(void**)ptrptr, size * count);
- if (!tmp && ((size * count) != 0))
- abort();
-
- *(void**)ptrptr = tmp;
+ *(void **)ptrptr = g_realloc_n(*(void**)ptrptr, size, count);
return 0;
}
@@ -341,9 +326,7 @@ int virAllocVar(void *ptrptr,
abort();
alloc_size = struct_size + (element_size * count);
- *(void **)ptrptr = calloc(1, alloc_size);
- if (*(void **)ptrptr == NULL)
- abort();
+ *(void **)ptrptr = g_malloc0(alloc_size);
return 0;
}
@@ -360,7 +343,7 @@ void virFree(void *ptrptr)
{
int save_errno = errno;
- free(*(void**)ptrptr);
+ g_free(*(void**)ptrptr);
*(void**)ptrptr = NULL;
errno = save_errno;
}
@@ -393,7 +376,7 @@ void virDispose(void *ptrptr,
if (*(void**)ptrptr && count > 0)
memset(*(void **)ptrptr, 0, count * element_size);
- free(*(void**)ptrptr);
+ g_free(*(void**)ptrptr);
*(void**)ptrptr = NULL;
if (countptr)
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn 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@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. 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) Maybe the safer approach would be to convert the code gradually and explicitly use g_alloc and g_free/g_autoptr without the wrappers? Jano -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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'.
> 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.
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.
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.