7:59 a.m.
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(a)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
8:03 a.m.
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.
6:51 a.m.
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(a)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 :|
5:30 a.m.
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(a)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(a)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(a)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
10:54 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5401
days inactive
5406
days old
4 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering