alloc: make VIR_APPEND_ELEMENT safer

VIR_APPEND_ELEMENT(array, size, elem) was not safe if the expression
for 'size' had side effects.  While no one in the current code base
was trying to pass side effects, we might as well be robust and
explicitly document our intentions.

* src/util/viralloc.c (virInsertElementsN): Add special case.
* src/util/viralloc.h (VIR_APPEND_ELEMENT): Use it.
(VIR_ALLOC, VIR_ALLOC_N, VIR_REALLOC_N, VIR_EXPAND_N)
(VIR_RESIZE_N, VIR_SHRINK_N, VIR_INSERT_ELEMENT)
(VIR_DELETE_ELEMENT, VIR_ALLOC_VAR, VIR_FREE): Document
which macros are safe in the presence of side effects.
* docs/hacking.html.in: Document this.
* HACKING: Regenerate.

Signed-off-by: Eric Blake <eblake@redhat.com>

diff --git a/HACKING b/HACKING
index b70b89d016d6f15326ac9832a2cdab43b611b85f..b4dcd3da6227223fefc3d687b994939a8161019a 100644 (file)
--- a/HACKING
+++ b/HACKING
@@ -418,6 +418,11 @@ But if negating a complex condition is too ugly, then at least add braces:
 
 Preprocessor
 ============
+Macros defined with an ALL_CAPS name should generally be assumed to be unsafe
+with regards to arguments with side-effects (that is, MAX(a++, b--) might
+increment a or decrement b too many or too few times). Exceptions to this rule
+are explicitly documented for macros in viralloc.h.
+
 For variadic macros, stick with C99 syntax:
 
   #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__)
@@ -501,7 +506,7 @@ 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 viralloc.h.
 
 - To allocate a single object:
 

diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index b15d18711c21983669fdfadea854e9db3d3e6af3..8708cb3a72f3fcbc2fabc15eae0777fa649541e0 100644 (file)
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -519,6 +519,13 @@
 
     <h2><a name="preprocessor">Preprocessor</a></h2>
 
+    <p>Macros defined with an ALL_CAPS name should generally be
+      assumed to be unsafe with regards to arguments with side-effects
+      (that is, MAX(a++, b--) might increment a or decrement b too
+      many or too few times).  Exceptions to this rule are explicitly
+      documented for macros in viralloc.h.
+    </p>
+
     <p>
       For variadic macros, stick with C99 syntax:
     </p>
@@ -616,7 +623,7 @@
       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 viralloc.h.
     </p>
 
     <ul>

diff --git a/src/util/viralloc.c b/src/util/viralloc.c
index 8f219bf1b5b56242932c6f823668d59fad38b9c4..8d6a7e6d32c21f9ace1e9a5cba78c3893fbe7b21 100644 (file)
--- a/src/util/viralloc.c
+++ b/src/util/viralloc.c
@@ -281,7 +281,7 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove)
  * virInsertElementsN:
  * @ptrptr:   pointer to hold address of allocated memory
  * @size:     the size of one element in bytes
- * @at:       index within array where new elements should be added
+ * @at:       index within array where new elements should be added, -1 for end
  * @countptr: variable tracking number of elements currently allocated
  * @add:      number of elements to add
  * @newelems: pointer to array of one or more new elements to move into
@@ -300,7 +300,8 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove)
  * items from newelems into ptrptr[at], then store the address of
  * allocated memory in *ptrptr and the new size in *countptr.  If
  * newelems is NULL, the new elements at ptrptr[at] are instead filled
- * with zero.
+ * with zero.  at must be between [0,*countptr], except that -1 is
+ * treated the same as *countptr for convenience.
  *
  * Returns -1 on failure, 0 on success
  */
@@ -310,7 +311,9 @@ virInsertElementsN(void *ptrptr, size_t size, size_t at,
                    size_t add, void *newelems,
                    bool clearOriginal, bool inPlace)
 {
-    if (at > *countptr) {
+    if (at == -1) {
+        at = *countptr;
+    } else if (at > *countptr) {
         VIR_WARN("out of bounds index - count %zu at %zu add %zu",
                  *countptr, at, add);
         return -1;

diff --git a/src/util/viralloc.h b/src/util/viralloc.h
index 7be7f82d57b2c2ce662f980739c8f6fb216ed4f2..35d3a37f742519b4a8e2a7e214c6dfb1e1e2700f 100644 (file)
--- a/src/util/viralloc.h
+++ b/src/util/viralloc.h
@@ -80,6 +80,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
  * the address of allocated memory in 'ptr'. Fill the
  * newly allocated memory with zeros.
  *
+ * This macro is safe to use on arguments with side effects.
+ *
  * Returns -1 on failure, 0 on success
  */
 # define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)))
@@ -93,6 +95,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
  * bytes long and store the address of allocated memory in
  * 'ptr'. Fill the newly allocated memory with zeros.
  *
+ * This macro is safe to use on arguments with side effects.
+ *
  * Returns -1 on failure, 0 on success
  */
 # define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count))
@@ -106,6 +110,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
  * bytes long and store the address of allocated memory in
  * 'ptr'. If 'ptr' grew, the added memory is uninitialized.
  *
+ * This macro is safe to use on arguments with side effects.
+ *
  * Returns -1 on failure, 0 on success
  */
 # define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count))
@@ -121,6 +127,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
  * address of allocated memory in 'ptr' and the new size in 'count'.
  * The new elements are filled with zero.
  *
+ * This macro is safe to use on arguments with side effects.
+ *
  * Returns -1 on failure, 0 on success
  */
 # define VIR_EXPAND_N(ptr, count, add) \
@@ -144,6 +152,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
  * the address of allocated memory in 'ptr' and the new size in 'alloc'.
  * The new elements are filled with zero.
  *
+ * This macro is safe to use on arguments with side effects.
+ *
  * Returns -1 on failure, 0 on success
  */
 # define VIR_RESIZE_N(ptr, alloc, count, add) \
@@ -160,6 +170,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
  * address of allocated memory in 'ptr' and the new size in 'count'.
  * If 'count' <= 'remove', the entire array is freed.
  *
+ * This macro is safe to use on arguments with side effects.
+ *
  * No return value.
  */
 # define VIR_SHRINK_N(ptr, count, remove) \
@@ -236,9 +248,9 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
  * replacing it with VIR_TYPECHECK(ptr, &newelem) so that we can be
  * assured ptr and &newelem are of compatible types.
  *
- * Returns -1 on failure, 0 on success
- *
+ * These macros are safe to use on arguments with side effects.
  *
+ * Returns -1 on failure, 0 on success
  */
 # define VIR_INSERT_ELEMENT(ptr, at, count, newelem) \
     virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count),    \
@@ -284,21 +296,21 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
  * replacing it with VIR_TYPECHECK(ptr, &newelem) so that we can be
  * assured ptr and &newelem are of compatible types.
  *
- * Returns -1 on failure, 0 on success
- *
+ * These macros are safe to use on arguments with side effects.
  *
+ * Returns -1 on failure, 0 on success
  */
 # define VIR_APPEND_ELEMENT(ptr, count, newelem) \
-    virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count),  \
+    virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count),  \
                        VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false)
 # define VIR_APPEND_ELEMENT_COPY(ptr, count, newelem) \
-    virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count),  \
+    virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count),  \
                        VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false)
 # define VIR_APPEND_ELEMENT_INPLACE(ptr, count, newelem) \
-    virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count),  \
+    virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count),  \
                        VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, true)
 # define VIR_APPEND_ELEMENT_COPY_INPLACE(ptr, count, newelem) \
-    virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count),  \
+    virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count),  \
                        VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, true)
 
 /**
@@ -315,6 +327,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
  * VIR_DELETE_ELEMENT_INPLACE is identical, but assumes any
  *   necessary memory re-allocation will be done later.
  *
+ * These macros are safe to use on arguments with side effects.
+ *
  * Returns -1 on failure, 0 on success
  */
 # define VIR_DELETE_ELEMENT(ptr, at, count) \
@@ -348,7 +362,9 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
  * that will be returned and allocate a suitable buffer to contain the
  * returned data.  C99 refers to these variable length objects as
  * structs containing flexible array members.
-
+ *
+ * This macro is safe to use on arguments with side effects.
+ *
  * Returns -1 on failure, 0 on success
  */
 # define VIR_ALLOC_VAR(ptr, type, count) \
@@ -360,6 +376,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
  *
  * Free the memory stored in 'ptr' and update to point
  * to NULL.
+ *
+ * This macro is safe to use on arguments with side effects.
  */
 # if !STATIC_ANALYSIS
 /* The ternary ensures that ptr is a pointer and not an integer type,