Some Coverity memory leak patches

[PATCH 9/9] util: Avoid using wrong free function

Posted by John Ferlan 4 years, 5 months ago

Since 1e2ae2e31, changes to use the automagic free logic didn't take
into account that one path uses posix_memalign and the other uses
VIR_ALLOC_N - the former requires using VIR_FREE() and not g_free()
to free the memory.

Found by Coverity.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/util/iohelper.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/util/iohelper.c b/src/util/iohelper.c
index 342bae229b..64b7a13f61 100644
--- a/src/util/iohelper.c
+++ b/src/util/iohelper.c
@@ -45,7 +45,11 @@
 static int
 runIO(const char *path, int fd, int oflags)
 {
+#if HAVE_POSIX_MEMALIGN
+    void *base = NULL; /* Location to be freed */
+#else
     g_autofree void *base = NULL; /* Location to be freed */
+#endif
     char *buf = NULL; /* Aligned location within base */
     size_t buflen = 1024*1024;
     intptr_t alignMask = 64*1024 - 1;
@@ -168,6 +172,9 @@ runIO(const char *path, int fd, int oflags)
     ret = 0;
 
  cleanup:
+#if HAVE_POSIX_MEMALIGN
+    VIR_FREE(base);
+#endif
     if (VIR_CLOSE(fd) < 0 &&
         ret == 0) {
         virReportSystemError(errno, _("Unable to close %s"), path);
-- 
2.25.4

Re: [PATCH 9/9] util: Avoid using wrong free function

Posted by Peter Krempa 4 years, 5 months ago

On Tue, Jun 16, 2020 at 08:07:10 -0400, John Ferlan wrote:
> Since 1e2ae2e31, changes to use the automagic free logic didn't take
> into account that one path uses posix_memalign and the other uses
> VIR_ALLOC_N - the former requires using VIR_FREE() and not g_free()
> to free the memory.

I don't really think this makes sense. VIR_FREE is now using
g_clear_pointer(&PTR, g_free) internally, so g_free will be used
regardless.

Is the problem really in g_free or rather some kind of problem with the
__attribute__(cleanup)) feature?

Additionally this fix looks weird. I'd prefer if autofree is dropped
here and VIR_FREE is used directly in both cases if this in fact fixes
the problem as there is no reason to keep g_autofree just in one case.

Re: [PATCH 9/9] util: Avoid using wrong free function

Posted by Daniel P. Berrangé 4 years, 5 months ago

On Tue, Jun 16, 2020 at 08:07:10AM -0400, John Ferlan wrote:
> Since 1e2ae2e31, changes to use the automagic free logic didn't take
> into account that one path uses posix_memalign and the other uses
> VIR_ALLOC_N - the former requires using VIR_FREE() and not g_free()
> to free the memory.

VIR_FREE calls g_free, so they're semantically identical


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 :|

Re: [PATCH 9/9] util: Avoid using wrong free function

Posted by John Ferlan 4 years, 5 months ago

On 6/16/20 8:16 AM, Daniel P. Berrangé wrote:
> On Tue, Jun 16, 2020 at 08:07:10AM -0400, John Ferlan wrote:
>> Since 1e2ae2e31, changes to use the automagic free logic didn't take
>> into account that one path uses posix_memalign and the other uses
>> VIR_ALLOC_N - the former requires using VIR_FREE() and not g_free()
>> to free the memory.
> 
> VIR_FREE calls g_free, so they're semantically identical
> 

Hmm... this one was something that had been on my list of false
positives (~60 patches) and wait for enough other changes before posting
list since March, but I see that reverting it doesn't cause the error
any more. So sure dropping is understandable...

Tks -

John