Re: [PATCH v3 03/19] util: use glib memory allocation functions

[Han Han <hhan@xxxxxxxxxx>]

On Thu, Oct 10, 2019 at 6:54 PM Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote:

Convert the VIR_ALLOC family of APIs with use of the g_malloc family of
APIs. Use of VIR_ALLOC related functions should be incrementally phased
out over time, allowing return value checks to be dropped. Use of
VIR_FREE should be replaced with auto-cleanup whenever possible.

We previously used the 'calloc-posix' gnulib module because mingw does
not set errno to ENOMEM on failure.

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>
Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
---
 bootstrap.conf       |   1 -
 docs/hacking.html.in | 106 +++++--------------------------------------
 src/util/viralloc.c  |  29 +++---------
 src/util/viralloc.h  |   9 ++++
 4 files changed, 27 insertions(+), 118 deletions(-)

diff --git a/bootstrap.conf b/bootstrap.conf
index 358d783a6b..7d73584809 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -26,7 +26,6 @@ byteswap
 c-ctype
 c-strcase
 c-strcasestr
-calloc-posix
 canonicalize-lgpl
 chown
 clock-time
diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index 2e064ced5e..8072796312 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -1008,102 +1008,20 @@ BAD:
     </p>

     <dl>
+      <dt>VIR_ALLOC, VIR_REALLOC, VIR_RESIZE_N, VIR_EXPAND_N,
+        VIR_SHRINK_N, VIR_FREE, VIR_APPEND_ELEMENT, VIR_INSERT_ELEMENT,
+        VIR_DELETE_ELEMENT</dt>
+      <dd>Prefer the GLib APIs g_new0/g_renew/g_free in most cases.
+        There should rarely be a need to use g_malloc/g_realloc.
+        Instead of using plain C arrays, it is preferrable to use
+        one of the GLib types, GArray, GPtrArray or GByteArray. These
+        all use a struct to track the array memory and size together
+        and efficiently resize. <strong>NEVER MIX</strong> use of the
+        classic libvirt memory allocation APIs and GLib APIs within
+        a single method. Keep the style consistent, converting existing
+        code to GLib style in a separate, prior commit.</dd>
     </dl>

-    <h2><a id="memalloc">Low level memory management</a></h2>
-
-    <p>
-      Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt
-      codebase, because they encourage a number of serious coding bugs and do
-      not enable compile time verification of checks for NULL. Instead of these
-      routines, use the macros from viralloc.h.
-    </p>
-
-    <ul>
-      <li><p>To allocate a single object:</p>
-
-<pre>
-  virDomainPtr domain;
-
-  if (VIR_ALLOC(domain) &lt; 0)
-      return NULL;
-</pre>
-      </li>
-
-      <li><p>To allocate an array of objects:</p>
-<pre>
-  virDomainPtr domains;
-  size_t ndomains = 10;
-
-  if (VIR_ALLOC_N(domains, ndomains) &lt; 0)
-      return NULL;
-</pre>
-      </li>
-
-      <li><p>To allocate an array of object pointers:</p>
-<pre>
-  virDomainPtr *domains;
-  size_t ndomains = 10;
-
-  if (VIR_ALLOC_N(domains, ndomains) &lt; 0)
-      return NULL;
-</pre>
-      </li>
-
-      <li><p>To re-allocate the array of domains to be 1 element
-      longer (however, note that repeatedly expanding an array by 1
-      scales quadratically, so this is recommended only for smaller
-      arrays):</p>
-<pre>
-  virDomainPtr domains;
-  size_t ndomains = 0;
-
-  if (VIR_EXPAND_N(domains, ndomains, 1) &lt; 0)
-      return NULL;
-  domains[ndomains - 1] = domain;
-</pre></li>
-
-      <li><p>To ensure an array has room to hold at least one more
-      element (this approach scales better, but requires tracking
-      allocation separately from usage)</p>
-
-<pre>
-  virDomainPtr domains;
-  size_t ndomains = 0;
-  size_t ndomains_max = 0;
-
-  if (VIR_RESIZE_N(domains, ndomains_max, ndomains, 1) &lt; 0)
-      return NULL;
-  domains[ndomains++] = domain;
-</pre>
-      </li>
-
-      <li><p>To trim an array of domains from its allocated size down
-      to the actual used size:</p>
-
-<pre>
-  virDomainPtr domains;
-  size_t ndomains = x;
-  size_t ndomains_max = y;
-
-  VIR_SHRINK_N(domains, ndomains_max, ndomains_max - ndomains);
-</pre></li>
-
-      <li><p>To free an array of domains:</p>
-<pre>
-  virDomainPtr domains;
-  size_t ndomains = x;
-  size_t ndomains_max = y;
-  size_t i;
-
-  for (i = 0; i &lt; ndomains; i++)
-      VIR_FREE(domains[i]);
-  VIR_FREE(domains);
-  ndomains_max = ndomains = 0;
-</pre>
-      </li>
-    </ul>
-
     <h2><a id="file_handling">File handling</a></h2>

     <p>
diff --git a/src/util/viralloc.c b/src/util/viralloc.c
index 10a8d0fb73..b8ca850764 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;
 }

@@ -343,9 +328,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;
 }

@@ -362,7 +345,7 @@ void virFree(void *ptrptr)
 {
     int save_errno = errno;

-    free(*(void**)ptrptr);
+    g_free(*(void**)ptrptr);

Hello, Daniel, a SIGABRT was found here:

Version:

libvirt v5.8.0-134-g9d03e9adf1

glib2-devel-2.56.4-7.el8.x86_64

Build:

# CC=/usr/lib64/ccache/cc ./configure --without-libssh --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu --program-prefix= --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64
--libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib --mandir=/usr/share/man --infodir=/usr/share/info --with-qemu --without-openvz --without-lxc --without-vbox --without-libxl --with-sasl --with-polkit --with-libvirtd --without-phyp --with-esx --without-hyperv --without-vmware --without-xenapi --without-vz --without-bhyve --with-interface --with-network --with-storage-fs --with-storage-lvm --with-storage-iscsi --with-storage-iscsi-direct --with-storage-scsi --with-storage-disk --with-storage-mpath --with-storage-rbd --without-storage-sheepdog --with-storage-gluster --without-storage-zfs --without-storage-vstorage --with-numactl --with-numad --with-capng --without-fuse --with-netcf --with-selinux --with-selinux-mount=/sys/fs/selinux --without-apparmor --without-hal --with-udev --with-yajl --with-sanlock --with-libpcap --with-macvtap --with-audit --with-dtrace --with-driver-modules --with-firewalld --with-firewalld-zone --without-wireshark-dissector --without-pm-utils --with-nss-plugin '--with-packager=Unknown, 2019-08-19-12:13:01, lab.rhel8.me' --with-packager-version=1.el8 --with-qemu-user=qemu --with-qemu-group=qemu --with-tls-priority=@LIBVIRT,SYSTEM --enable-werror --enable-expensive-tests --with-init-script=systemd --without-login-shell && make -j8

Test steps:

# LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')" src/.libs/virtlogd
free(): invalid pointer
[1]    22488 abort (core dumped)  LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')" src/.libs/virtlogd

(gdb) bt
#0  0x00007fc3be4c68df in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007fc3be4b0cf5 in __GI_abort () at abort.c:79
#2  0x00007fc3be509c17 in __libc_message (action="" fmt=fmt@entry=0x7fc3be61670c "%s\n") at ../sysdeps/posix/libc_fatal.c:181
#3  0x00007fc3be51053c in malloc_printerr (str=str@entry=0x7fc3be6148bb "free(): invalid pointer") at malloc.c:5356
#4  0x00007fc3be511c64 in _int_free (av=<optimized out>, p=<optimized out>, have_lock=<optimized out>) at malloc.c:4185
#5  0x00007fc3bf3182e2 in g_free (mem=0x7ffe56ef7e58) at gmem.c:194
#6  0x00007fc3c261f63b in virFree (ptrptr=ptrptr@entry=0x7ffe56ef7d88) at util/viralloc.c:348
#7  0x00007fc3c26a1270 in virSystemdActivationFree (act=<optimized out>, act@entry=0x7ffe56ef7e58) at util/virsystemd.c:1056
#8  0x000055767eed7f4c in main (argc=<optimized out>, argv=0x7ffe56ef84c8) at logging/log_daemon.c:1119

See the full backtrace in attachment

     *(void**)ptrptr = NULL;
     errno = save_errno;
 }
@@ -395,7 +378,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)
diff --git a/src/util/viralloc.h b/src/util/viralloc.h
index 3e72e40bc9..517f9aada6 100644
--- a/src/util/viralloc.h
+++ b/src/util/viralloc.h
@@ -24,6 +24,15 @@

 #include "internal.h"

+/**
+ * DEPRECATION WARNING
+ *
+ * APIs in this file should only be used when modifying existing code.
+ * Consider converting existing code to use the new APIs when touching
+ * it. All new code must use the GLib memory allocation APIs and/or
+ * GLib array data types. See the hacking file for more guidance.
+ */
+
 /* Return 1 if an array of N objects, each of size S, cannot exist due
    to size arithmetic overflow.  S must be positive and N must be
    nonnegative.  This is a macro, not an inline function, so that it
--
2.21.0

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

--

Best regards,

-----------------------------------

Han Han
Quality Engineer
Redhat.

Email: hhan@xxxxxxxxxx
Phone: +861065339333

Thread 1 (Thread 0x7fc3c2dabb80 (LWP 22488)):
#0  0x00007fc3be4c68df in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
        set = 
            {__val = {0, 0, 12884902125, 0, 0, 0, 140478764464584, 140478756550203, 140478762420208, 2, 140478759080664, 140478756836535, 140478759081712, 12850540288, 140478759079669, 0}}
        pid = <optimized out>
        tid = <optimized out>
#1  0x00007fc3be4b0cf5 in __GI_abort () at abort.c:79
        save_stage = 1
        act = 
          {__sigaction_handler = {sa_handler = 0x7fc3c28980d4, sa_sigaction = 0x7fc3c28980d4}, sa_mask = {__val = {140478756836535, 140478759142104, 10048469808, 140478759141522, 0, 140728898421085, 15612540617273618176, 140478759141632, 93967452941024, 3, 0, 0, 0, 0, 140730356956096, 140730356956352}}, sa_flags = 4096, sa_restorer = 0x7ffe56ef7bc0}
        sigs = {__val = {32, 0 <repeats 15 times>}}
#2  0x00007fc3be509c17 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7fc3be61670c "%s\n") at ../sysdeps/posix/libc_fatal.c:181
        ap = {{gp_offset = 24, fp_offset = 32707, overflow_arg_area = 0x7ffe56ef7cd0, reg_save_area = 0x7ffe56ef7c60}}
        fd = <optimized out>
        list = <optimized out>
        nlist = <optimized out>
        cp = <optimized out>
        written = <optimized out>
#3  0x00007fc3be51053c in malloc_printerr (str=str@entry=0x7fc3be6148bb "free(): invalid pointer") at malloc.c:5356
#4  0x00007fc3be511c64 in _int_free (av=<optimized out>, p=<optimized out>, have_lock=<optimized out>) at malloc.c:4185
        size = 0
        fb = <optimized out>
        nextchunk = <optimized out>
        nextsize = <optimized out>
        nextinuse = <optimized out>
        prevsize = <optimized out>
        bck = <optimized out>
        fwd = <optimized out>
        __PRETTY_FUNCTION__ = "_int_free"
#5  0x00007fc3bf3182e2 in g_free (mem=0x7ffe56ef7e58) at gmem.c:194
#6  0x00007fc3c261f63b in virFree (ptrptr=ptrptr@entry=0x7ffe56ef7d88) at util/viralloc.c:348
        save_errno = 2
#7  0x00007fc3c26a1270 in virSystemdActivationFree (act=<optimized out>, act@entry=0x7ffe56ef7e58) at util/virsystemd.c:1056
#8  0x000055767eed7f4c in main (argc=<optimized out>, argv=0x7ffe56ef84c8) at logging/log_daemon.c:1119
        act = 0x0
        actmap = 
            {{name = 0x55767eeddfcd "virtlogd.socket", family = 1, port = 0, path = 0x557680a73760 "/var/run/libvirt/virtlogd-sock"}, {name = 0x55767eeddfdd "virtlogd-admin.socket", family = 1, port = 0, path = 0x557680a73790 "/var/run/libvirt/virtlogd-admin-sock"}}
        logSrv = 0x557680a72cf0
        adminSrv = 0x557680a72f00
        logProgram = 0x0
        adminProgram = 0x0
        remote_config_file = 0x557680a73680 "/etc/libvirt/virtlogd.conf"
        statuswrite = -1
        ret = 1
        verbose = 0
        godaemon = 0
        run_dir = 0x557680a5c440 "/var/run/libvirt"
        pid_file = 0x557680a5b980 "/var/run/virtlogd.pid"
        pid_file_fd = 3
        sock_file = 0x557680a73760 "/var/run/libvirt/virtlogd-sock"
        admin_sock_file = 0x557680a73790 "/var/run/libvirt/virtlogd-admin-sock"
        timeout = -1
        state_file = 0x557680a737c0 "/var/run/virtlogd-restart-exec.json"
        implicit_conf = <optimized out>
        old_umask = <optimized out>
        privileged = <optimized out>
        config = <optimized out>
        rv = 0
        opts = {{name = 0x55767eedddfd "verbose", has_arg = 0, flag = 0x7ffe56ef7e0c, val = 118}, {name = 0x55767eede0e0 "daemon", has_arg = 0, flag = 0x7ffe56ef7e10, val = 100}, {name = 0x55767eedde05 "config", has_arg = 1, flag = 0x0, val = 102}, {name = 0x55767eedde5a "timeout", has_arg = 1, flag = 0x0, val = 116}, {name = 0x55767eedde0c "pid-file", has_arg = 1, flag = 0x0, val = 112}, {name = 0x55767eedde15 "version", has_arg = 0, flag = 0x0, val = 86}, {name = 0x55767eedde1d "help", has_arg = 0, flag = 0x0, val = 104}, {name = 0x0, has_arg = 0, flag = 0x0, val = 0}}
        __func__ = "main"

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list