[libvirt] [PATCH] (absolutePathFromBaseFile): fix up preceding commit
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@redhat.com> 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; } -- 1.7.0.rc1.204.gb96e
Jim Meyering wrote: ...
+ ignore_value(virAsprintf(&res, "%.*s/%s", (int) d_len, base_file, path));
Regarding this, I will be removing the warn_unused_result attribute from virAsprintf (and this use of ignore_value), since it *is* ok to ignore its return value, since it guarantees the pointer arg is set to NULL whenever it fails.
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@redhat.com> 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 and any use of assert(). If the function can conceivably fail the assertion, then we need to return an error code, not abort(). 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 :|
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@redhat.com> 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@redhat.com> 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@redhat.com> 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
On Wed, Feb 10, 2010 at 12:30:26PM +0100, Jim Meyering wrote:
Daniel P. Berrange wrote:
- 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
[...]
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.
It's a bit of an unwritten policy, but really we should try to avoid assert() in libvirt code, even if in that case right it's look unlikely to get there, but it's better to handle things gracefully, as long as it doesn't make the code really really worse. Point is if we start to add assert() calls, then new people looking at the code and submitting new patches may thing it's generally acceptable, so let's avoid this as much as possible, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering