[libvirt] [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"
Daniel P. Berrange
berrange at redhat.com
Thu Feb 4 18:18:07 UTC 2010
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 :|
More information about the libvir-list
mailing list