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(a)redhat.com&gt; --- HACKING | 7 ++++++- docs/hacking.html.in | 9 ++++++++- src/util/viralloc.c | 9 ++++++--- src/util/viralloc.h | 36 +++++++++++++++++++++++++++--------- 4 files changed, 47 insertions(+), 14 deletions(-) diff --git a/HACKING b/HACKING index b70b89d..b4dcd3d 100644 --- 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 b15d187..8708cb3 100644 --- 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 8f219bf..8d6a7e6 100644 --- 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 7be7f82..35d3a37 100644 --- 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, -- 1.8.1.4

The surest way to avoid regressions is to test documented behavior :) * tests/virstringtest.c (testStrdup): New test case. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tests/virstringtest.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/tests/virstringtest.c b/tests/virstringtest.c index db1e8a9..159566b 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2012 Red Hat, Inc. + * Copyright (C) 2012-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -107,6 +107,51 @@ cleanup: return ret; } +static int +testStrdup(const void *data ATTRIBUTE_UNUSED) +{ + char *array[] = { NULL, NULL }; + size_t i = 0; + int ret = -1; + int value; + + value = VIR_STRDUP(array[i++], "hello"); + if (value != 1) { + fprintf(stderr, "unexpected strdup result %d, expected 1\n", value); + goto cleanup; + } + if (i != 1) { + fprintf(stderr, "unexpected side effects i=%zu, expected 1\n", i); + goto cleanup; + } + if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { + fprintf(stderr, "incorrect array contents '%s' '%s'\n", + NULLSTR(array[0]), NULLSTR(array[1])); + goto cleanup; + } + + value = VIR_STRNDUP(array[i++], NULL, 5); + if (value != 0) { + fprintf(stderr, "unexpected strdup result %d, expected 0\n", value); + goto cleanup; + } + if (i != 2) { + fprintf(stderr, "unexpected side effects i=%zu, expected 2\n", i); + goto cleanup; + } + if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { + fprintf(stderr, "incorrect array contents '%s' '%s'\n", + NULLSTR(array[0]), NULLSTR(array[1])); + goto cleanup; + } + + ret = 0; +cleanup: + for (i = 0; i < ARRAY_CARDINALITY(array); i++) + VIR_FREE(array[i]); + return ret; +} + static int mymain(void) @@ -153,6 +198,8 @@ mymain(void) const char *tokens7[] = { "The", "quick", "brown", "fox", "", NULL }; TEST_SPLIT("The quick brown fox ", " ", 0, tokens7); + if (virtTestRun("strdup", 1, testStrdup, NULL) < 0) + ret = -1; return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.8.1.4

On 05/14/2013 06:50 AM, Ján Tomko wrote: Ooh, good idea. I'm squashing this in (it still passes, and now exercises more side effects), then pushing. diff --git i/tests/virstringtest.c w/tests/virstringtest.c index 359cb9a..da06c0f 100644 --- i/tests/virstringtest.c +++ w/tests/virstringtest.c @@ -107,15 +107,41 @@ cleanup: return ret; } +static bool fail; + +static const char * +testStrdupLookup1(size_t i) +{ + switch (i) { + case 0: + return "hello"; + case 1: + return NULL; + default: + fail = true; + return "oops"; + } +} + +static size_t +testStrdupLookup2(size_t i) +{ + if (i) + fail = true; + return 5; +} + static int testStrdup(const void *data ATTRIBUTE_UNUSED) { char *array[] = { NULL, NULL }; size_t i = 0; + size_t j = 0; + size_t k = 0; int ret = -1; int value; - value = VIR_STRDUP(array[i++], "hello"); + value = VIR_STRDUP(array[i++], testStrdupLookup1(j++)); if (value != 1) { fprintf(stderr, "unexpected strdup result %d, expected 1\n", value); goto cleanup; @@ -124,13 +150,18 @@ testStrdup(const void *data ATTRIBUTE_UNUSED) fprintf(stderr, "unexpected side effects i=%zu, expected 1\n", i); goto cleanup; } + if (j != 1) { + fprintf(stderr, "unexpected side effects j=%zu, expected 1\n", j); + goto cleanup; + } if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { fprintf(stderr, "incorrect array contents '%s' '%s'\n", NULLSTR(array[0]), NULLSTR(array[1])); goto cleanup; } - value = VIR_STRNDUP(array[i++], NULL, 5); + value = VIR_STRNDUP(array[i++], testStrdupLookup1(j++), + testStrdupLookup2(k++)); if (value != 0) { fprintf(stderr, "unexpected strdup result %d, expected 0\n", value); goto cleanup; @@ -139,12 +170,25 @@ testStrdup(const void *data ATTRIBUTE_UNUSED) fprintf(stderr, "unexpected side effects i=%zu, expected 2\n", i); goto cleanup; } + if (j != 2) { + fprintf(stderr, "unexpected side effects j=%zu, expected 2\n", j); + goto cleanup; + } + if (k != 1) { + fprintf(stderr, "unexpected side effects k=%zu, expected 1\n", k); + goto cleanup; + } if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { fprintf(stderr, "incorrect array contents '%s' '%s'\n", NULLSTR(array[0]), NULLSTR(array[1])); goto cleanup; } + if (fail) { + fprintf(stderr, "side effects failed\n"); + goto cleanup; + } + ret = 0; cleanup: for (i = 0; i < ARRAY_CARDINALITY(array); i++) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org