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@xxxxxxxxxx>
> 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;
> }
>
Okay, but while we're cleaning it up, I would suggest to do a
virReportOOMError(NULL) in case of allocation failure and remove
the one from virStorageFileGetMetadataFromFD() since it's used only
once. I think the conn arg is not useful anymore and it's better
to report the error when it occurs than at the caller level.
BTW I didn't know about stp(n)cpy ...
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list