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

[Daniel Veillard]

On Fri, Feb 05, 2010 at 10:59:23AM +0100, Jim Meyering wrote:
> Daniel P. Berrange wrote:
> > 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.
> 
> Good point.
> In spite of having to use "%.*s" in the format,
> using virAsprintf is both more readable and more concise.
> Note that there is no need to test the return value of virAsprintf here.
> Here's the incremental patch, followed by the amended one:
> 
> Note also that I've done it this way:
> 
>     virAsprintf(&res, "%.*s/%s", base_file, d_len, path);
> 
> but I could also have omitted the "/" from the format,
> since there's guaranteed to be one at base_file[d_len],
> but that is not obvious, which makes this much less readable:
> 
>     virAsprintf(&res, "%.*s%s", base_file, d_len+1, path);
> 
> Likewise, I could have avoided one of those two stpcpy calls.
>
> diff --git a/bootstrap b/bootstrap
> index 5cc43c5..113cc0f 100755
> --- a/bootstrap
> +++ b/bootstrap
> @@ -94,7 +94,6 @@ send
>  setsockopt
>  socket
>  stpcpy
> -stpncpy
>  strchrnul
>  strndup
>  strerror
> diff --git a/src/util/storage_file.c b/src/util/storage_file.c
> index 8c53fba..2c79fa9 100644
> --- a/src/util/storage_file.c
> +++ b/src/util/storage_file.c
> @@ -255,10 +255,7 @@ absolutePathFromBaseFile(const char *base_file, const char *path)
>      if (*path == '/' || d_len == 0)
>          return strdup(path);
> 
> -    if (VIR_ALLOC_N(res, d_len + 1 + strlen(path) + 1) < 0)
> -        return NULL;
> -
> -    stpcpy(stpcpy(stpncpy(res, base_file, d_len), "/"), path);
> +    virAsprintf(&res, "%.*s/%s", base_file, d_len, path);
>      return res;
>  }
> 
> 
> >From 8653369953741ceb0451db998e8c766220dd9ac4 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.
> * .gnulib: Update submodule to the latest.
> ---
>  .gnulib                 |    2 +-
>  bootstrap               |    1 +
>  src/util/storage_file.c |   26 ++++++++------------------
>  3 files changed, 10 insertions(+), 19 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..113cc0f 100755
> --- a/bootstrap
> +++ b/bootstrap
> @@ -71,6 +71,7 @@ c-ctype
>  canonicalize-lgpl
>  close
>  connect
> +dirname-lgpl
>  getaddrinfo
>  gethostname
>  getpass
> diff --git a/src/util/storage_file.c b/src/util/storage_file.c
> index 44057d2..2c79fa9 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,15 @@ 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)
> -        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 */
> -    }
> +    virAsprintf(&res, "%.*s/%s", base_file, d_len, path);
>      return res;
>  }
> 

  ACK,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/