<div dir="ltr">Not reproduced when build source before `make clean`. Please ignore that issue<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 15, 2019 at 10:20 AM Han Han <<a href="mailto:hhan@redhat.com">hhan@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 10, 2019 at 6:54 PM Daniel P. Berrangé <<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Convert the VIR_ALLOC family of APIs with use of the g_malloc family of<br>
APIs. Use of VIR_ALLOC related functions should be incrementally phased<br>
out over time, allowing return value checks to be dropped. Use of<br>
VIR_FREE should be replaced with auto-cleanup whenever possible.<br>
<br>
We previously used the 'calloc-posix' gnulib module because mingw does<br>
not set errno to ENOMEM on failure.<br>
<br>
Reviewed-by: Ján Tomko <<a href="mailto:jtomko@redhat.com" target="_blank">jtomko@redhat.com</a>><br>
Signed-off-by: Daniel P. Berrangé <<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>><br>
---<br>
bootstrap.conf | 1 -<br>
docs/<a href="http://hacking.html.in" rel="noreferrer" target="_blank">hacking.html.in</a> | 106 +++++--------------------------------------<br>
src/util/viralloc.c | 29 +++---------<br>
src/util/viralloc.h | 9 ++++<br>
4 files changed, 27 insertions(+), 118 deletions(-)<br>
<br>
diff --git a/bootstrap.conf b/bootstrap.conf<br>
index 358d783a6b..7d73584809 100644<br>
--- a/bootstrap.conf<br>
+++ b/bootstrap.conf<br>
@@ -26,7 +26,6 @@ byteswap<br>
c-ctype<br>
c-strcase<br>
c-strcasestr<br>
-calloc-posix<br>
canonicalize-lgpl<br>
chown<br>
clock-time<br>
diff --git a/docs/<a href="http://hacking.html.in" rel="noreferrer" target="_blank">hacking.html.in</a> b/docs/<a href="http://hacking.html.in" rel="noreferrer" target="_blank">hacking.html.in</a><br>
index 2e064ced5e..8072796312 100644<br>
--- a/docs/<a href="http://hacking.html.in" rel="noreferrer" target="_blank">hacking.html.in</a><br>
+++ b/docs/<a href="http://hacking.html.in" rel="noreferrer" target="_blank">hacking.html.in</a><br>
@@ -1008,102 +1008,20 @@ BAD:<br>
</p><br>
<br>
<dl><br>
+ <dt>VIR_ALLOC, VIR_REALLOC, VIR_RESIZE_N, VIR_EXPAND_N,<br>
+ VIR_SHRINK_N, VIR_FREE, VIR_APPEND_ELEMENT, VIR_INSERT_ELEMENT,<br>
+ VIR_DELETE_ELEMENT</dt><br>
+ <dd>Prefer the GLib APIs g_new0/g_renew/g_free in most cases.<br>
+ There should rarely be a need to use g_malloc/g_realloc.<br>
+ Instead of using plain C arrays, it is preferrable to use<br>
+ one of the GLib types, GArray, GPtrArray or GByteArray. These<br>
+ all use a struct to track the array memory and size together<br>
+ and efficiently resize. <strong>NEVER MIX</strong> use of the<br>
+ classic libvirt memory allocation APIs and GLib APIs within<br>
+ a single method. Keep the style consistent, converting existing<br>
+ code to GLib style in a separate, prior commit.</dd><br>
</dl><br>
<br>
- <h2><a id="memalloc">Low level memory management</a></h2><br>
-<br>
- <p><br>
- Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt<br>
- codebase, because they encourage a number of serious coding bugs and do<br>
- not enable compile time verification of checks for NULL. Instead of these<br>
- routines, use the macros from viralloc.h.<br>
- </p><br>
-<br>
- <ul><br>
- <li><p>To allocate a single object:</p><br>
-<br>
-<pre><br>
- virDomainPtr domain;<br>
-<br>
- if (VIR_ALLOC(domain) < 0)<br>
- return NULL;<br>
-</pre><br>
- </li><br>
-<br>
- <li><p>To allocate an array of objects:</p><br>
-<pre><br>
- virDomainPtr domains;<br>
- size_t ndomains = 10;<br>
-<br>
- if (VIR_ALLOC_N(domains, ndomains) < 0)<br>
- return NULL;<br>
-</pre><br>
- </li><br>
-<br>
- <li><p>To allocate an array of object pointers:</p><br>
-<pre><br>
- virDomainPtr *domains;<br>
- size_t ndomains = 10;<br>
-<br>
- if (VIR_ALLOC_N(domains, ndomains) < 0)<br>
- return NULL;<br>
-</pre><br>
- </li><br>
-<br>
- <li><p>To re-allocate the array of domains to be 1 element<br>
- longer (however, note that repeatedly expanding an array by 1<br>
- scales quadratically, so this is recommended only for smaller<br>
- arrays):</p><br>
-<pre><br>
- virDomainPtr domains;<br>
- size_t ndomains = 0;<br>
-<br>
- if (VIR_EXPAND_N(domains, ndomains, 1) < 0)<br>
- return NULL;<br>
- domains[ndomains - 1] = domain;<br>
-</pre></li><br>
-<br>
- <li><p>To ensure an array has room to hold at least one more<br>
- element (this approach scales better, but requires tracking<br>
- allocation separately from usage)</p><br>
-<br>
-<pre><br>
- virDomainPtr domains;<br>
- size_t ndomains = 0;<br>
- size_t ndomains_max = 0;<br>
-<br>
- if (VIR_RESIZE_N(domains, ndomains_max, ndomains, 1) < 0)<br>
- return NULL;<br>
- domains[ndomains++] = domain;<br>
-</pre><br>
- </li><br>
-<br>
- <li><p>To trim an array of domains from its allocated size down<br>
- to the actual used size:</p><br>
-<br>
-<pre><br>
- virDomainPtr domains;<br>
- size_t ndomains = x;<br>
- size_t ndomains_max = y;<br>
-<br>
- VIR_SHRINK_N(domains, ndomains_max, ndomains_max - ndomains);<br>
-</pre></li><br>
-<br>
- <li><p>To free an array of domains:</p><br>
-<pre><br>
- virDomainPtr domains;<br>
- size_t ndomains = x;<br>
- size_t ndomains_max = y;<br>
- size_t i;<br>
-<br>
- for (i = 0; i < ndomains; i++)<br>
- VIR_FREE(domains[i]);<br>
- VIR_FREE(domains);<br>
- ndomains_max = ndomains = 0;<br>
-</pre><br>
- </li><br>
- </ul><br>
-<br>
<h2><a id="file_handling">File handling</a></h2><br>
<br>
<p><br>
diff --git a/src/util/viralloc.c b/src/util/viralloc.c<br>
index 10a8d0fb73..b8ca850764 100644<br>
--- a/src/util/viralloc.c<br>
+++ b/src/util/viralloc.c<br>
@@ -45,10 +45,7 @@ VIR_LOG_INIT("util.alloc");<br>
int virAlloc(void *ptrptr,<br>
size_t size)<br>
{<br>
- *(void **)ptrptr = calloc(1, size);<br>
- if (*(void **)ptrptr == NULL)<br>
- abort();<br>
-<br>
+ *(void **)ptrptr = g_malloc0(size);<br>
return 0;<br>
}<br>
<br>
@@ -69,10 +66,7 @@ int virAllocN(void *ptrptr,<br>
size_t size,<br>
size_t count)<br>
{<br>
- *(void**)ptrptr = calloc(count, size);<br>
- if (*(void**)ptrptr == NULL)<br>
- abort();<br>
-<br>
+ *(void**)ptrptr = g_malloc0_n(count, size);<br>
return 0;<br>
}<br>
<br>
@@ -94,16 +88,7 @@ int virReallocN(void *ptrptr,<br>
size_t size,<br>
size_t count)<br>
{<br>
- void *tmp;<br>
-<br>
- if (xalloc_oversized(count, size))<br>
- abort();<br>
-<br>
- tmp = realloc(*(void**)ptrptr, size * count);<br>
- if (!tmp && ((size * count) != 0))<br>
- abort();<br>
-<br>
- *(void**)ptrptr = tmp;<br>
+ *(void **)ptrptr = g_realloc_n(*(void**)ptrptr, size, count);<br>
return 0;<br>
}<br>
<br>
@@ -343,9 +328,7 @@ int virAllocVar(void *ptrptr,<br>
abort();<br>
<br>
alloc_size = struct_size + (element_size * count);<br>
- *(void **)ptrptr = calloc(1, alloc_size);<br>
- if (*(void **)ptrptr == NULL)<br>
- abort();<br>
+ *(void **)ptrptr = g_malloc0(alloc_size);<br>
return 0;<br>
}<br>
<br>
@@ -362,7 +345,7 @@ void virFree(void *ptrptr)<br>
{<br>
int save_errno = errno;<br>
<br>
- free(*(void**)ptrptr);<br>
+ g_free(*(void**)ptrptr);<br></blockquote><div>Hello, Daniel, a SIGABRT was found here:</div><div>Version:<br></div><div>libvirt v5.8.0-134-g9d03e9adf1</div><div>glib2-devel-2.56.4-7.el8.x86_64</div><div><br></div><div>Build:</div><div># 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<br>--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, <a href="http://lab.rhel8.me" target="_blank">lab.rhel8.me</a>' --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</div><div><br></div><div>Test steps:</div><div># LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')" src/.libs/virtlogd<br>free(): invalid pointer<br>[1] 22488 abort (core dumped) LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')" src/.libs/virtlogd</div><div>(gdb) bt<br>#0 0x00007fc3be4c68df in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50<br>#1 0x00007fc3be4b0cf5 in __GI_abort () at abort.c:79<br>#2 0x00007fc3be509c17 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7fc3be61670c "%s\n") at ../sysdeps/posix/libc_fatal.c:181<br>#3 0x00007fc3be51053c in malloc_printerr (str=str@entry=0x7fc3be6148bb "free(): invalid pointer") at malloc.c:5356<br>#4 0x00007fc3be511c64 in _int_free (av=<optimized out>, p=<optimized out>, have_lock=<optimized out>) at malloc.c:4185<br>#5 0x00007fc3bf3182e2 in g_free (mem=0x7ffe56ef7e58) at gmem.c:194<br>#6 0x00007fc3c261f63b in virFree (ptrptr=ptrptr@entry=0x7ffe56ef7d88) at util/viralloc.c:348<br>#7 0x00007fc3c26a1270 in virSystemdActivationFree (act=<optimized out>, act@entry=0x7ffe56ef7e58) at util/virsystemd.c:1056<br>#8 0x000055767eed7f4c in main (argc=<optimized out>, argv=0x7ffe56ef84c8) at logging/log_daemon.c:1119</div><div><br></div><div>See the full backtrace in attachment<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
*(void**)ptrptr = NULL;<br>
errno = save_errno;<br>
}<br>
@@ -395,7 +378,7 @@ void virDispose(void *ptrptr,<br>
if (*(void**)ptrptr && count > 0)<br>
memset(*(void **)ptrptr, 0, count * element_size);<br>
<br>
- free(*(void**)ptrptr);<br>
+ g_free(*(void**)ptrptr);<br>
*(void**)ptrptr = NULL;<br>
<br>
if (countptr)<br>
diff --git a/src/util/viralloc.h b/src/util/viralloc.h<br>
index 3e72e40bc9..517f9aada6 100644<br>
--- a/src/util/viralloc.h<br>
+++ b/src/util/viralloc.h<br>
@@ -24,6 +24,15 @@<br>
<br>
#include "internal.h"<br>
<br>
+/**<br>
+ * DEPRECATION WARNING<br>
+ *<br>
+ * APIs in this file should only be used when modifying existing code.<br>
+ * Consider converting existing code to use the new APIs when touching<br>
+ * it. All new code must use the GLib memory allocation APIs and/or<br>
+ * GLib array data types. See the hacking file for more guidance.<br>
+ */<br>
+<br>
/* Return 1 if an array of N objects, each of size S, cannot exist due<br>
to size arithmetic overflow. S must be positive and N must be<br>
nonnegative. This is a macro, not an inline function, so that it<br>
-- <br>
2.21.0<br>
<br>
--<br>
libvir-list mailing list<br>
<a href="mailto:libvir-list@redhat.com" target="_blank">libvir-list@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/libvir-list" rel="noreferrer" target="_blank">https://www.redhat.com/mailman/listinfo/libvir-list</a></blockquote></div><br clear="all"><br>-- <br><div dir="ltr"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr">Best regards,</div><div dir="ltr">-----------------------------------<br></div><div dir="ltr">Han Han<br>Quality Engineer<br>Redhat.<br><br>Email: <a href="mailto:hhan@redhat.com" target="_blank">hhan@redhat.com</a><br>Phone: +861065339333<br></div></div></div></div></div></div></div>
</blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr">Best regards,</div><div dir="ltr">-----------------------------------<br></div><div dir="ltr">Han Han<br>Quality Engineer<br>Redhat.<br><br>Email: <a href="mailto:hhan@redhat.com" target="_blank">hhan@redhat.com</a><br>Phone: +861065339333<br></div></div></div></div></div></div>