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

On Thu, Oct 10, 2019 at 6:54 PM Daniel P. Berrangé <berrange(a)redhat.com&gt; 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(a)redhat.com&gt; > Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > --- > 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=action@entry=do_abort, 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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan(a)redhat.com Phone: +861065339333