[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/4] memory: make it safer to expand arrays



On Sat, Aug 14, 2010 at 10:23:53AM -0600, Eric Blake wrote:
> Note - I did _not_ update HACKING.  Do we have a makefile rule
> for doing an auto-xsl translation from hacking.html.in yet, or
> is something I'll have to do manually before pushing this?
> 
> * 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.
> * docs/hacking.html.in: Prefer newer interfaces over
> VIR_REALLOC_N, since uninitialized memory can bite us.
> ---
>  docs/hacking.html.in |   24 +++++++++++--------
>  src/util/memory.c    |   59 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  src/util/memory.h    |   51 ++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 117 insertions(+), 17 deletions(-)
> 
> diff --git a/docs/hacking.html.in b/docs/hacking.html.in
> index deab530..4cc92f4 100644
> --- a/docs/hacking.html.in
> +++ b/docs/hacking.html.in
> @@ -332,11 +332,11 @@
>        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>eg to allocate  a single object:</p>
> +      <li><p>To allocate a single object:</p>
> 
>  <pre>
>        virDomainPtr domain;
> @@ -347,11 +347,11 @@
>        }
>  </pre></li>
> 
> -      <li><p>eg 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();
> @@ -359,7 +359,7 @@
>         }
>  </pre></li>
> 
> -      <li><p>eg to allocate an array of object pointers</p>
> +      <li><p>To allocate an array of object pointers</p>
> 
>  <pre>
>         virDomainPtr *domains;
> @@ -371,18 +371,22 @@
>         }
>  </pre></li>
> 
> -      <li><p>eg to re-allocate the array of domains to be longer</p>
> +      <li><p>To re-allocate the array of domains to be 10 elements longer</p>
> 
>  <pre>
> -       ndomains = 20
> -
> -       if (VIR_REALLOC_N(domains, ndomains) &lt; 0) {
> +       if (VIR_EXPAND_N(domains, ndomains, 10) &lt; 0) {
>             virReportOOMError();
>             return NULL;
>         }
>  </pre></li>
> 
> -      <li><p>eg 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);
> diff --git a/src/util/memory.c b/src/util/memory.c
> index dd1216b..e95aa47 100644
> --- a/src/util/memory.c
> +++ b/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
>   */
> @@ -165,6 +167,61 @@ int virReallocN(void *ptrptr, size_t size, size_t count)
>  }
> 
>  /**
> + * 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 res;
> +
> +    if (*countptr + add < *countptr) {
> +        errno = ENOMEM;
> +        return -1;
> +    }
> +    res = virReallocN(ptrptr, size, *countptr + add);
> +    if (res == 0) {
> +        memset(*(char **)ptrptr + (size * *countptr), 0, size * add);
> +        *countptr += add;
> +    }
> +    return res;
> +}
> +
> +/**
> + * 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
>   * @struct_size: size of initial struct
> diff --git a/src/util/memory.h b/src/util/memory.h
> index 60e2be6..98ac2b3 100644
> --- a/src/util/memory.h
> +++ b/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 virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
> -int virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
> +int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK
> +    ATTRIBUTE_NONNULL(1);
> +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;
> -void virFree(void *ptrptr);
> +                size_t count) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1);
> +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

ACK, I've wanted to add this for a long time now

Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]