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@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 :|