Daniel P. Berrange wrote:
> On Fri, Feb 05, 2010 at 02:59:17PM +0100, Jim Meyering wrote:
>> To my chagrin, I saw that my most recent commit introduced
>> compilation errors. Sorry about that.
>> Here's how I propose to fix it.
>>
>> >From 2d948a373ecebec6c06274f61b31d1ae9c40ae41 Mon Sep 17 00:00:00 2001
>> From: Jim Meyering <meyering@xxxxxxxxxx>
>> Date: Fri, 5 Feb 2010 14:57:35 +0100
>> Subject: [PATCH] (absolutePathFromBaseFile): fix up preceding commit
>>
>> * src/util/storage_file.c: Include <assert.h>.
>> (absolutePathFromBaseFile): Assert that converting size_t to int is valid.
>> Reverse length/string args to match "%.*s".
>> Explicitly ignore the return value of virAsprintf.
>> ---
>> src/util/storage_file.c | 7 ++++++-
>> 1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/util/storage_file.c b/src/util/storage_file.c
>> index 2c79fa9..135acec 100644
>> --- a/src/util/storage_file.c
>> +++ b/src/util/storage_file.c
>> @@ -26,7 +26,9 @@
>>
>> #include <unistd.h>
>> #include <fcntl.h>
>> +#include <assert.h>
>> #include "dirname.h"
>> +#include "ignore-value.h"
>> #include "memory.h"
>> #include "virterror_internal.h"
>>
>> @@ -255,7 +257,10 @@ absolutePathFromBaseFile(const char *base_file, const char *path)
>> if (*path == '/' || d_len == 0)
>> return strdup(path);
>>
>> - virAsprintf(&res, "%.*s/%s", base_file, d_len, path);
>> + /* Ensure that the following cast-to-int is valid. */
>> + assert (d_len <= INT_MAX);
>> +
>> + ignore_value(virAsprintf(&res, "%.*s/%s", (int) d_len, base_file, path));
>> return res;
>> }
>
> NACK to this
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 aea80d174596b48de3be09c323b6f721d8598b82 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@xxxxxxxxxx>
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 f88903a3706f9af2d63edfb2e9c8280e314668a0 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@xxxxxxxxxx>
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list