memory: make it safer to expand arrays

* src/util/memory.h (VIR_REALLOC_N): Update docs.
(VIR_EXPAND_N, VIR_SHRINK_N): New macros.
(virAlloc, virAllocN, virReallocN, virAllocVar, virFree): Add some
gcc attributes.
* src/util/memory.c (virExpandN, virShrinkN): New functions.
(virReallocN): Update docs.
* src/libvirt_private.syms: Export new helpers.
* docs/hacking.html.in: Prefer newer interfaces over
VIR_REALLOC_N, since uninitialized memory can bite us.
* HACKING: Regenerate.

HACKING

@@ -280,9 +280,9 @@ Low level memory management
 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 memory.h
+routines, use the macros from memory.h.
 
-- e.g. to allocate a single object:
+- To allocate a single object:
 
   virDomainPtr domain;
 
@@ -293,10 +293,10 @@ routines, use the macros from memory.h
 
 
 
-- e.g. to allocate an array of objects
+- To allocate an array of objects:
 
   virDomainPtr domains;
-  int ndomains = 10;
+  size_t ndomains = 10;
 
   if (VIR_ALLOC_N(domains, ndomains) < 0) {
       virReportOOMError();
@@ -305,7 +305,7 @@ routines, use the macros from memory.h
 
 
 
-- e.g. to allocate an array of object pointers
+- To allocate an array of object pointers:
 
   virDomainPtr *domains;
   int ndomains = 10;
@@ -317,18 +317,22 @@ routines, use the macros from memory.h
 
 
 
-- e.g. to re-allocate the array of domains to be longer
+- To re-allocate the array of domains to be longer:
 
-  ndomains = 20
+  if (VIR_EXPAND_N(domains, ndomains, 10) < 0) {
-
-  if (VIR_REALLOC_N(domains, ndomains) < 0) {
       virReportOOMError();
       return NULL;
   }
 
 
 
-- e.g. to free the domain
+- To trim an array of domains to have one less element:
+
+  VIR_SHRINK_N(domains, ndomains, 1);
+
+
+
+- To free the domain:
 
   VIR_FREE(domain);
 
@@ -344,7 +348,7 @@ code base to help avoiding double-closing of files or file descriptors, which
 is particulary dangerous in a multi-threaded applications. Instead of these
 APIs, use the macros from files.h
 
-- eg opening a file from a file descriptor
+- Open a file from a file descriptor:
 
   if ((file = VIR_FDOPEN(fd, "r")) == NULL) {
       virReportSystemError(errno, "%s",
@@ -355,7 +359,7 @@ APIs, use the macros from files.h
 
 
 
-- e.g. close a file descriptor
+- Close a file descriptor:
 
   if (VIR_CLOSE(fd) < 0) {
       virReportSystemError(errno, "%s", _("failed to close file"));
@@ -363,7 +367,7 @@ APIs, use the macros from files.h
 
 
 
-- eg close a file
+- Close a file:
 
   if (VIR_FCLOSE(file) < 0) {
       virReportSystemError(errno, "%s", _("failed to close file"));
@@ -371,8 +375,8 @@ APIs, use the macros from files.h
 
 
 
-- eg close a file or file descriptor in an error path, without losing the
+- Close a file or file descriptor in an error path, without losing the previous
-previous "errno" value
+"errno" value:
 
   VIR_FORCE_CLOSE(fd);
   VIR_FORCE_FCLOSE(file);
@@ -460,7 +464,7 @@ If there is a need for complex string concatenations, avoid using the usual
 sequence of malloc/strcpy/strcat/snprintf functions and make use of the
 virBuffer API described in buf.h
 
-eg typical usage is as follows:
+Typical usage is as follows:
 
   char *
   somefunction(...)
@@ -594,7 +598,7 @@ When using goto, please use one of these standard labels if it makes sense:
       error: A path only taken upon return with an error code
     cleanup: A path taken upon return with success code + optional error
   no_memory: A path only taken upon return with an OOM error code
-      retry: If needing to jump upwards (eg retry on EINTR)
+      retry: If needing to jump upwards (e.g., retry on EINTR)
 
 
 Libvirt committer guidelines

docs/hacking.html.in

@@ -354,11 +354,12 @@
       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 memory.h
+      routines, use the macros from memory.h.
     </p>
 
     <ul>
-      <li><p>e.g. to allocate  a single object:</p>
+      <li><p>To allocate a single object:</p>
+
 <pre>
   virDomainPtr domain;
 
@@ -369,10 +370,10 @@
 </pre>
       </li>
 
-      <li><p>e.g. to allocate an array of objects</p>
+      <li><p>To allocate an array of objects:</p>
 <pre>
   virDomainPtr domains;
-  int ndomains = 10;
+  size_t ndomains = 10;
 
   if (VIR_ALLOC_N(domains, ndomains) &lt; 0) {
       virReportOOMError();
@@ -381,7 +382,7 @@
 </pre>
       </li>
 
-      <li><p>e.g. to allocate an array of object pointers</p>
+      <li><p>To allocate an array of object pointers:</p>
 <pre>
   virDomainPtr *domains;
   int ndomains = 10;
@@ -393,18 +394,22 @@
 </pre>
       </li>
 
-      <li><p>e.g. to re-allocate the array of domains to be longer</p>
+      <li><p>To re-allocate the array of domains to be longer:</p>
 <pre>
-  ndomains = 20
+  if (VIR_EXPAND_N(domains, ndomains, 10) &lt; 0) {
-
-  if (VIR_REALLOC_N(domains, ndomains) &lt; 0) {
       virReportOOMError();
       return NULL;
   }
 </pre>
       </li>
 
-      <li><p>e.g. to free the domain</p>
+      <li><p>To trim an array of domains to have one less element:</p>
+
+<pre>
+  VIR_SHRINK_N(domains, ndomains, 1);
+</pre></li>
+
+      <li><p>To free the domain:</p>
 <pre>
   VIR_FREE(domain);
 </pre>
@@ -421,7 +426,7 @@
     </p>
 
    <ul>
-      <li><p>eg opening a file from a file descriptor</p>
+      <li><p>Open a file from a file descriptor:</p>
 
 <pre>
   if ((file = VIR_FDOPEN(fd, "r")) == NULL) {
@@ -432,14 +437,14 @@
   /* fd is now invalid; only access the file using file variable */
 </pre></li>
 
-      <li><p>e.g. close a file descriptor</p>
+      <li><p>Close a file descriptor:</p>
 <pre>
   if (VIR_CLOSE(fd) &lt; 0) {
       virReportSystemError(errno, "%s", _("failed to close file"));
   }
 </pre></li>
 
-      <li><p>eg close a file</p>
+      <li><p>Close a file:</p>
 
 <pre>
   if (VIR_FCLOSE(file) &lt; 0) {
@@ -447,8 +452,8 @@
   }
 </pre></li>
 
-      <li><p>eg close a file or file descriptor in an error path, without losing
+      <li><p>Close a file or file descriptor in an error path, without losing
-             the previous <code>errno</code> value</p>
+             the previous <code>errno</code> value:</p>
 
 <pre>
   VIR_FORCE_CLOSE(fd);
@@ -554,7 +559,7 @@
       make use of the virBuffer API described in buf.h
     </p>
 
-    <p>eg typical usage is as follows:</p>
+    <p>Typical usage is as follows:</p>
 
 <pre>
   char *
@@ -722,7 +727,7 @@
       error: A path only taken upon return with an error code
     cleanup: A path taken upon return with success code + optional error
   no_memory: A path only taken upon return with an OOM error code
-      retry: If needing to jump upwards (eg retry on EINTR)
+      retry: If needing to jump upwards (e.g., retry on EINTR)
 </pre>
 
 

src/libvirt_private.syms

@@ -503,8 +503,10 @@ virLogUnlock;
 # memory.h
 virAlloc;
 virAllocN;
+virExpandN;
 virFree;
 virReallocN;
+virShrinkN;
 
 
 # network.h

src/util/memory.c

@@ -1,6 +1,7 @@
 /*
  * memory.c: safer memory allocation
  *
+ * Copyright (C) 2010 Red Hat, Inc.
  * Copyright (C) 2008 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -24,6 +25,7 @@
 #include <stddef.h>
 
 #include "memory.h"
+#include "ignore-value.h"
 
 
 #if TEST_OOM
@@ -141,7 +143,7 @@ int virAllocN(void *ptrptr, size_t size, size_t count)
  * 'count' elements, each 'size' bytes in length. Update 'ptrptr'
  * with the address of the newly allocated memory. On failure,
  * 'ptrptr' is not changed and still points to the original memory
- * block. The newly allocated memory is filled with zeros.
+ * block. Any newly allocated memory in 'ptrptr' is uninitialized.
  *
  * Returns -1 on failure to allocate, zero on success
  */
@@ -164,6 +166,61 @@ int virReallocN(void *ptrptr, size_t size, size_t count)
     return 0;
 }
 
+/**
+ * virExpandN:
+ * @ptrptr: pointer to pointer for address of allocated memory
+ * @size: number of bytes per element
+ * @countptr: pointer to number of elements in array
+ * @add: number of elements to add
+ *
+ * Resize the block of memory in 'ptrptr' to be an array of
+ * '*countptr' + 'add' elements, each 'size' bytes in length.
+ * Update 'ptrptr' and 'countptr'  with the details of the newly
+ * allocated memory. On failure, 'ptrptr' and 'countptr' are not
+ * changed. Any newly allocated memory in 'ptrptr' is zero-filled.
+ *
+ * Returns -1 on failure to allocate, zero on success
+ */
+int virExpandN(void *ptrptr, size_t size, size_t *countptr, size_t add)
+{
+    int ret;
+
+    if (*countptr + add < *countptr) {
+        errno = ENOMEM;
+        return -1;
+    }
+    ret = virReallocN(ptrptr, size, *countptr + add);
+    if (ret == 0) {
+        memset(*(char **)ptrptr + (size * *countptr), 0, size * add);
+        *countptr += add;
+    }
+    return ret;
+}
+
+/**
+ * virShrinkN:
+ * @ptrptr: pointer to pointer for address of allocated memory
+ * @size: number of bytes per element
+ * @countptr: pointer to number of elements in array
+ * @remove: number of elements to remove
+ *
+ * Resize the block of memory in 'ptrptr' to be an array of
+ * '*countptr' - 'remove' elements, each 'size' bytes in length.
+ * Update 'ptrptr' and 'countptr'  with the details of the newly
+ * allocated memory. If 'remove' is larger than 'countptr', free
+ * the entire array.
+ */
+void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t remove)
+{
+    if (remove < *countptr)
+        ignore_value(virReallocN(ptrptr, size, *countptr -= remove));
+    else {
+        virFree(ptrptr);
+        *countptr = 0;
+    }
+}
+
+
 /**
  * Vir_Alloc_Var:
  * @ptrptr: pointer to hold address of allocated memory

src/util/memory.h

@@ -46,14 +46,21 @@
 
 
 /* Don't call these directly - use the macros below */
-int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK;
+int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK
-int virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
+    ATTRIBUTE_NONNULL(1);
-int virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
+int virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK
+    ATTRIBUTE_NONNULL(1);
+int virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK
+    ATTRIBUTE_NONNULL(1);
+int virExpandN(void *ptrptr, size_t size, size_t *count, size_t add)
+    ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
+void virShrinkN(void *ptrptr, size_t size, size_t *count, size_t remove)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
 int virAllocVar(void *ptrptr,
                 size_t struct_size,
                 size_t element_size,
-                size_t count) ATTRIBUTE_RETURN_CHECK;
+                size_t count) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1);
-void virFree(void *ptrptr);
+void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
 
 /**
  * VIR_ALLOC:
@@ -87,12 +94,44 @@ void virFree(void *ptrptr);
  *
  * Re-allocate an array of 'count' elements, each sizeof(*ptr)
  * bytes long and store the address of allocated memory in
- * 'ptr'. Fill the newly allocated memory with zeros
+ * 'ptr'. If 'ptr' grew, the added memory is uninitialized.
  *
  * Returns -1 on failure, 0 on success
  */
 # define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count))
 
+/**
+ * VIR_EXPAND_N:
+ * @ptr: pointer to hold address of allocated memory
+ * @count: variable tracking number of elements currently allocated
+ * @add: number of elements to add
+ *
+ * Re-allocate an array of 'count' elements, each sizeof(*ptr)
+ * bytes long, to be 'count' + 'add' elements long, then store the
+ * address of allocated memory in 'ptr' and the new size in 'count'.
+ * The new elements are filled with zero.
+ *
+ * Returns -1 on failure, 0 on success
+ */
+# define VIR_EXPAND_N(ptr, count, add) \
+    virExpandN(&(ptr), sizeof(*(ptr)), &(count), add)
+
+/**
+ * VIR_SHRINK_N:
+ * @ptr: pointer to hold address of allocated memory
+ * @count: variable tracking number of elements currently allocated
+ * @remove: number of elements to remove
+ *
+ * Re-allocate an array of 'count' elements, each sizeof(*ptr)
+ * bytes long, to be 'count' - 'remove' elements long, then store the
+ * address of allocated memory in 'ptr' and the new size in 'count'.
+ * If 'count' <= 'remove', the entire array is freed.
+ *
+ * No return value.
+ */
+# define VIR_SHRINK_N(ptr, count, remove) \
+    virShrinkN(&(ptr), sizeof(*(ptr)), &(count), remove)
+
 /*
  * VIR_ALLOC_VAR_OVERSIZED:
  * @M: size of base structure