Re: [libvirt] [PATCH] (absolutePathFromBaseFile): fix up preceding commit

Daniel P. Berrange wrote: FYI, the patch you NACK'd had already been pushed, since it fixed compilation warning/errors and a real bug. I'm not sure how a user or even a malicious admin could cause base_file to have a length of 2GiB and pass it to this function, but it's easy to avoid the assertion this time, so I've done so. As already mentioned, virAsprintf should not have the warn_unused_result and the second patch removes it, along with the use of ignore_value that I'd been forced to add in absolutePathFromBaseFile. Neither changes how libvirt works. I've pushed both. From: Jim Meyering <meyering(a)redhat.com&gt; Date: Wed, 10 Feb 2010 11:54:24 +0100 Subject: [PATCH 1/2] absolutePathFromBaseFile: avoid an unnecessary use of assert * src/util/storage_file.c (absolutePathFromBaseFile): While this use of virAsprintf is slightly cleaner than using stpncpy(stpcpy(..., it does impose an artificial limitation on the length of the base_file name. Rather than asserting that it does not exceed INT_MAX, return NULL when it does. --- src/util/storage_file.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 3b69210..f8e528d 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -26,7 +26,6 @@ #include <unistd.h> #include <fcntl.h> -#include <assert.h> #include "dirname.h" #include "ignore-value.h" #include "memory.h" @@ -251,7 +250,8 @@ absolutePathFromBaseFile(const char *base_file, const char *path) return strdup(path); /* Ensure that the following cast-to-int is valid. */ - assert (d_len <= INT_MAX); + if (d_len > INT_MAX) + return NULL; ignore_value(virAsprintf(&res, "%.*s/%s", (int) d_len, base_file, path)); return res; -- 1.7.0.rc2.170.gbc565 From: Jim Meyering <meyering(a)redhat.com&gt; Date: Wed, 10 Feb 2010 12:00:38 +0100 Subject: [PATCH 2/2] virAsprintf: remove its warn_unused_result attribute * src/util/util.h (virAsprintf): Remove ATTRIBUTE_RETURN_CHECK, since it is perfectly fine to ignore the return value, now that the pointer is guaranteed to be set to NULL upon failure. * src/util/storage_file.c (absolutePathFromBaseFile): Remove now- unnecessary use of ignore_value. --- src/util/storage_file.c | 3 +-- src/util/util.h | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/storage_file.c b/src/util/storage_file.c index f8e528d..76ebb98 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -27,7 +27,6 @@ #include <unistd.h> #include <fcntl.h> #include "dirname.h" -#include "ignore-value.h" #include "memory.h" #include "virterror_internal.h" @@ -253,7 +252,7 @@ absolutePathFromBaseFile(const char *base_file, const char *path) if (d_len > INT_MAX) return NULL; - ignore_value(virAsprintf(&res, "%.*s/%s", (int) d_len, base_file, path)); + virAsprintf(&res, "%.*s/%s", (int) d_len, base_file, path); return res; } diff --git a/src/util/util.h b/src/util/util.h index bb545ac..4207508 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -177,8 +177,7 @@ int virMacAddrCompare (const char *mac1, const char *mac2); void virSkipSpaces(const char **str); int virParseNumber(const char **str); -int virAsprintf(char **strp, const char *fmt, ...) - ATTRIBUTE_FMT_PRINTF(2, 3) ATTRIBUTE_RETURN_CHECK; +int virAsprintf(char **strp, const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(2, 3); char *virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) ATTRIBUTE_RETURN_CHECK; char *virStrcpy(char *dest, const char *src, size_t destbytes) -- 1.7.0.rc2.170.gbc565