[libvirt] [PATCH v3 03/19] util: use glib memory allocation functions
Han Han
hhan at redhat.com
Tue Oct 15 02:20:16 UTC 2019
On Thu, Oct 10, 2019 at 6:54 PM Daniel P. Berrangé <berrange at redhat.com>
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 at redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
> 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) < 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) < 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) < 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) < 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) < 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 < 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 at 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 at entry=do_abort,
fmt=fmt at entry=0x7fc3be61670c "%s\n") at ../sysdeps/posix/libc_fatal.c:181
#3 0x00007fc3be51053c in malloc_printerr (str=str at 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 at entry=0x7ffe56ef7d88) at
util/viralloc.c:348
#7 0x00007fc3c26a1270 in virSystemdActivationFree (act=<optimized out>,
act at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan at redhat.com
Phone: +861065339333
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191015/551f669d/attachment-0001.htm>
-------------- next part --------------
Thread 1 (Thread 0x7fc3c2dabb80 (LWP 22488)):
#0 0x00007fc3be4c68df in __GI_raise (sig=sig at 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 at entry=do_abort, fmt=fmt at 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 at 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 at entry=0x7ffe56ef7d88) at util/viralloc.c:348
save_errno = 2
#7 0x00007fc3c26a1270 in virSystemdActivationFree (act=<optimized out>, act at 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"
More information about the libvir-list
mailing list