[libvirt] [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"

[Daniel P. Berrange]

On Thu, Feb 04, 2010 at 05:14:40PM +0100, Jim Meyering wrote:
> Not only did this function leak(p), but it would also over-allocate
> (by the length of basename(base_file)), and then later, re-alloc
> to compensate, so I rewrote it:
> 
> >From 1dc52930daa000b407d8a8f18588a19cf4e8b7f5 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering at redhat.com>
> Date: Thu, 4 Feb 2010 16:55:57 +0100
> Subject: [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"
> 
> * src/util/storage_file.c: Include "dirname.h".
> (absolutePathFromBaseFile): Rewrite not to leak, and to require
> fewer allocations.
> * bootstrap (modules): Add dirname-lgpl and stpncpy.
> * .gnulib: Update submodule to the latest.
> ---
>  .gnulib                 |    2 +-
>  bootstrap               |    2 ++
>  src/util/storage_file.c |   27 ++++++++++-----------------
>  3 files changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/.gnulib b/.gnulib
> index 146d914..9d0ad65 160000
> --- a/.gnulib
> +++ b/.gnulib
> @@ -1 +1 @@
> -Subproject commit 146d9145073e62a2096a2d6b33f75e93908fedf3
> +Subproject commit 9d0ad652de159d08e5f679842f8a2a5658196361
> diff --git a/bootstrap b/bootstrap
> index cc3c6ef..5cc43c5 100755
> --- a/bootstrap
> +++ b/bootstrap
> @@ -71,6 +71,7 @@ c-ctype
>  canonicalize-lgpl
>  close
>  connect
> +dirname-lgpl
>  getaddrinfo
>  gethostname
>  getpass
> @@ -93,6 +94,7 @@ send
>  setsockopt
>  socket
>  stpcpy
> +stpncpy
>  strchrnul
>  strndup
>  strerror
> diff --git a/src/util/storage_file.c b/src/util/storage_file.c
> index 44057d2..8c53fba 100644
> --- a/src/util/storage_file.c
> +++ b/src/util/storage_file.c
> @@ -1,7 +1,7 @@
>  /*
>   * storage_file.c: file utility functions for FS storage backend
>   *
> - * Copyright (C) 2007-2009 Red Hat, Inc.
> + * Copyright (C) 2007-2010 Red Hat, Inc.
>   * Copyright (C) 2007-2008 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -26,6 +26,7 @@
> 
>  #include <unistd.h>
>  #include <fcntl.h>
> +#include "dirname.h"
>  #include "memory.h"
>  #include "virterror_internal.h"
> 
> @@ -246,26 +247,18 @@ vmdk4GetBackingStore(virConnectPtr conn,
>  static char *
>  absolutePathFromBaseFile(const char *base_file, const char *path)
>  {
> -    size_t base_size, path_size;
> -    char *res, *p;
> +    char *res;
> +    size_t d_len = dir_len (base_file);
> 
> -    if (*path == '/')
> +    /* If path is already absolute, or if dirname(base_file) is ".",
> +       just return a copy of path.  */
> +    if (*path == '/' || d_len == 0)
>          return strdup(path);
> 
> -    base_size = strlen(base_file) + 1;
> -    path_size = strlen(path) + 1;
> -    if (VIR_ALLOC_N(res, base_size - 1 + path_size) < 0)
> +    if (VIR_ALLOC_N(res, d_len + 1 + strlen(path) + 1) < 0)
>          return NULL;
> -    memcpy(res, base_file, base_size);
> -    p = strrchr(res, '/');
> -    if (p != NULL)
> -        p++;
> -    else
> -        p = res;
> -    memcpy(p, path, path_size);
> -    if (VIR_REALLOC_N(res, (p + path_size) - res) < 0) {
> -        /* Ignore failure */
> -    }
> +
> +    stpcpy(stpcpy(stpncpy(res, base_file, d_len), "/"), path);
>      return res;
>  }

Any code which uses a combination of VIR_ALLOC + strcat/strcpy/strpcpy/etc
really ought to be re-written to use virAsprintf(). The nested stpcpy is
a nice trick, but its not really helping clarity like asprintf would.

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