[libvirt] [PATCH 2/7] util: make allocation functions abort on OOM

[John Ferlan]

On 8/29/19 2:02 PM, Daniel P. Berrangé wrote:
> The functions are left returning an "int" to avoid an immediate
> big-bang cleanup. They'll simply never return anything other
> than 0, except for virInsertN which can still return an error
> if the requested insertion index is out of range. Interestingly
> in that case, the _QUIET function would none the less report
> an error.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/util/viralloc.c | 201 +++++++++++---------------------------------
>  src/util/viralloc.h | 145 +++++++++++---------------------
>  2 files changed, 97 insertions(+), 249 deletions(-)
> 


FWIW: I applied the series to my Coverity branch to see what (if
anything) gets shaken there.  Good news is - not much. There were a
couple of things I already reported on that were fixed for 5.7.0 (commit
ed7e342b0 and be9d259eb).

There's one more false positive in qemuDomainGetNumaParameters that
probably would be fixed by AUTOFREE stuff (nodeset gets set to NULL
after virTypedParameterAssign).

Beyond that the only thing is two CHECKED_RETURN's (e.g. N callers check
the return value, but these 2 don't).  I consider both false positives;
however, it could also be argued that unless any of the functions where
either 0 or abort occurs, then make them just void, but that's a sh*load
more work...

[...]

>  /**
> @@ -204,50 +143,35 @@ int virExpandN(void *ptrptr,
>   * @allocptr: pointer to number of elements allocated in array
>   * @count: number of elements currently used in array
>   * @add: minimum number of additional elements to support in array
> - * @report: whether to report OOM error, if there is one
> - * @domcode: error domain code
> - * @filename: caller's filename
> - * @funcname: caller's funcname
> - * @linenr: caller's line number
>   *
>   * If 'count' + 'add' is larger than '*allocptr', then resize the
>   * block of memory in 'ptrptr' to be an array of at least 'count' +
>   * 'add' elements, each 'size' bytes in length. Update 'ptrptr' and
>   * 'allocptr' with the details of the newly allocated memory. On
>   * failure, 'ptrptr' and 'allocptr' are not changed. Any newly
> - * allocated memory in 'ptrptr' is zero-filled. If @report is true,
> - * OOM errors are reported automatically.
> - *
> + * allocated memory in 'ptrptr' is zero-filled.
>   *
> - * Returns -1 on failure to allocate, zero on success
> + * Returns zero on success, aborts on OOM
>   */
>  int virResizeN(void *ptrptr,
>                 size_t size,
>                 size_t *allocptr,
>                 size_t count,
> -               size_t add,
> -               bool report,
> -               int domcode,
> -               const char *filename,
> -               const char *funcname,
> -               size_t linenr)
> +               size_t add)
>  {
>      size_t delta;
>  
> -    if (count + add < count) {
> -        if (report)
> -            virReportOOMErrorFull(domcode, filename, funcname, linenr);
> -        errno = ENOMEM;
> -        return -1;
> -    }
> +    if (count + add < count)
> +        abort();
> +
>      if (count + add <= *allocptr)
>          return 0;
>  
>      delta = count + add - *allocptr;
>      if (delta < *allocptr / 2)
>          delta = *allocptr / 2;
> -    return virExpandN(ptrptr, size, allocptr, delta, report,
> -                      domcode, filename, funcname, linenr);
> +    virExpandN(ptrptr, size, allocptr, delta);
> +    return 0;

Could just return virExpandN here...

>  }

[...]

> @@ -312,12 +229,7 @@ int
>  virInsertElementsN(void *ptrptr, size_t size, size_t at,
>                     size_t *countptr,
>                     size_t add, void *newelems,
> -                   bool clearOriginal, bool inPlace,
> -                   bool report,
> -                   int domcode,
> -                   const char *filename,
> -                   const char *funcname,
> -                   size_t linenr)
> +                   bool clearOriginal, bool inPlace)
>  {
>      if (at == -1) {
>          at = *countptr;
> @@ -330,9 +242,8 @@ virInsertElementsN(void *ptrptr, size_t size, size_t at,
>  
>      if (inPlace) {
>          *countptr += add;
> -    } else if (virExpandN(ptrptr, size, countptr, add, report,
> -                          domcode, filename, funcname, linenr) < 0) {
> -        return -1;
> +    } else {
> +        virExpandN(ptrptr, size, countptr, add);
>      }
>  

This one's more painful, with using ignore_value() wrapper to just
pacify Coverity.

[...]

I'm not saying anything has to be done, but I figured it could be a
useful data point for you -

John