8:27 a.m.
Hi,
attached patches backport the fixes for CVE-2013-6458 to v0.9.12-maint. I
decided to cherry-pick the introduction of VIR_STRDUP and virReportError
as well to ease backporting of future fixes. I'd be happy about any review.
Daniel P. Berrange (1):
Introduce virReportError macro for general error reporting
Eric Blake (2):
string: make VIR_STRDUP easier to use
string: test VIR_STRDUP
Jiri Denemark (4):
qemu: Do not access stale data in virDomainBlockStats
qemu: Avoid using stale data in virDomainGetBlockInfo
qemu: Fix job usage in qemuDomainBlockJobImpl
qemu: Fix job usage in virDomainGetBlockIoTune
Michal Privoznik (1):
virstring: Introduce VIR_STRDUP and VIR_STRNDUP
HACKING | 16 ++++++++
cfg.mk | 1 +
docs/hacking.html.in | 24 ++++++++++-
src/libvirt_private.syms | 2 +
src/qemu/qemu_driver.c | 68 ++++++++++++++++---------------
src/util/virprocess.c | 3 --
src/util/virstring.c | 78 ++++++++++++++++++++++++++++++++++++
src/util/virstring.h | 70 ++++++++++++++++++++++++++++++++
src/util/virterror_internal.h | 3 ++
tests/virstringtest.c | 93 ++++++++++++++++++++++++++++++++++++++++++-
10 files changed, 320 insertions(+), 38 deletions(-)
--
1.8.5.2
8:27 a.m.
New subject: [libvirt] [v0.9.12-maint 1/8] virstring: Introduce VIR_STRDUP and VIR_STRNDUP
From: Michal Privoznik <mprivozn(a)redhat.com>
The code adaptation is not done right now, but in subsequent patches.
Hence I am not implementing syntax-check rule as it would break
compilation. Developers are strongly advised to use these new macros.
They are similar to VIR_ALLOC() logic: VIR_STRDUP(dst, src) returns zero
on success, -1 otherwise. In case you don't want to report OOM error,
use the _QUIET variant of a macro.
Conflicts:
src/libvirt_private.syms
src/util/virstring.h
(cherry picked from commit c3abb5c45988a0d7583f059974513722d82e2c2b)
---
HACKING | 11 +++++++
docs/hacking.html.in | 14 +++++++++
src/libvirt_private.syms | 2 ++
src/util/virstring.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virstring.h | 60 +++++++++++++++++++++++++++++++++++++++
5 files changed, 161 insertions(+)
diff --git a/HACKING b/HACKING
index 69ea96b..5b5c34b 100644
--- a/HACKING
+++ b/HACKING
@@ -535,6 +535,17 @@ sizeof(dest) returns something meaningful). Note that this is a
macro, so
arguments could be evaluated more than once. This is equivalent to
virStrncpy(dest, src, strlen(src), sizeof(dest)).
+ VIR_STRDUP(char *dst, const char *src);
+ VIR_STRNDUP(char *dst, const char *src, size_t n);
+
+You should avoid using strdup or strndup directly as they do not report
+out-of-memory error. Use VIR_STRDUP or VIR_STRNDUP macros instead. Note, that
+these two behave similar to VIR_ALLOC: on success zero is returned, otherwise
+the result is -1 and dst is guaranteed to be NULL. In very specific cases,
+when you don't want to report the out-of-memory error, you can use
+VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and usually
+considered a flaw.
+
Variable length string buffer
=============================
diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index 89f9980..198afe7 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -641,6 +641,20 @@
virStrncpy(dest, src, strlen(src), sizeof(dest)).
</p>
+<pre>
+ VIR_STRDUP(char *dst, const char *src);
+ VIR_STRNDUP(char *dst, const char *src, size_t n);
+</pre>
+ <p>
+ You should avoid using strdup or strndup directly as they do not report
+ out-of-memory error. Use VIR_STRDUP or VIR_STRNDUP macros instead. Note,
+ that these two behave similar to VIR_ALLOC: on success zero is returned,
+ otherwise the result is -1 and dst is guaranteed to be NULL. In very
+ specific cases, when you don't want to report the out-of-memory error, you
+ can use VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare
+ and usually considered a flaw.
+ </p>
+
<h2><a name="strbuf">Variable length string
buffer</a></h2>
<p>
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f35fd63..21b593a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1495,9 +1495,11 @@ virStrerror;
# virstring.h
+virStrdup;
virStringSplit;
virStringJoin;
virStringFreeList;
+virStrndup;
# virtime.h
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 92289eb..21142b4 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -177,3 +177,77 @@ size_t virStringListLength(char **strings)
return i;
}
+
+/**
+ * virStrdup:
+ * @dest: where to store duplicated string
+ * @src: the source string to duplicate
+ * @report: whether to report OOM error, if there is one
+ * @domcode: error domain code
+ * @filename: caller's filename
+ * @funcname: caller's funcname
+ * @linenr: caller's line number
+ *
+ * Wrapper over strdup, which reports OOM error if told so,
+ * in which case callers wants to pass @domcode, @filename,
+ * @funcname and @linenr which should represent location in
+ * caller's body where virStrdup is called from. Consider
+ * using VIR_STRDUP which sets these automatically.
+ *
+ * Returns: 0 on success, -1 otherwise.
+ */
+int
+virStrdup(char **dest,
+ const char *src,
+ bool report,
+ int domcode,
+ const char *filename,
+ const char *funcname,
+ size_t linenr)
+{
+ if (!(*dest = strdup(src))) {
+ if (report)
+ virReportOOMErrorFull(domcode, filename, funcname, linenr);
+ return -1;
+ }
+
+ return 0;
+}
+
+/**
+ * virStrndup:
+ * @dest: where to store duplicated string
+ * @src: the source string to duplicate
+ * @n: how many bytes to copy
+ * @report: whether to report OOM error, if there is one
+ * @domcode: error domain code
+ * @filename: caller's filename
+ * @funcname: caller's funcname
+ * @linenr: caller's line number
+ *
+ * Wrapper over strndup, which reports OOM error if told so,
+ * in which case callers wants to pass @domcode, @filename,
+ * @funcname and @linenr which should represent location in
+ * caller's body where virStrndup is called from. Consider
+ * using VIR_STRNDUP which sets these automatically.
+ *
+ * Returns: 0 on success, -1 otherwise.
+ */
+int
+virStrndup(char **dest,
+ const char *src,
+ size_t n,
+ bool report,
+ int domcode,
+ const char *filename,
+ const char *funcname,
+ size_t linenr)
+{
+ if (!(*dest = strndup(src, n))) {
+ if (report)
+ virReportOOMErrorFull(domcode, filename, funcname, linenr);
+ return -1;
+ }
+
+ return 0;
+}
diff --git a/src/util/virstring.h b/src/util/virstring.h
index d68ed2f..cd5ccda 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -37,4 +37,64 @@ void virStringFreeList(char **strings);
size_t virStringListLength(char **strings);
+/* Don't call these directly - use the macros below */
+int virStrdup(char **dest, const char *src, bool report, int domcode,
+ const char *filename, const char *funcname, size_t linenr)
+ ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
+int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode,
+ const char *filename, const char *funcname, size_t linenr)
+ ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
+/**
+ * VIR_STRDUP:
+ * @dst: variable to hold result (char*, not char**)
+ * @src: string to duplicate
+ *
+ * Duplicate @src string and store it into @dst.
+ *
+ * Returns -1 on failure (with OOM error reported), 0 on success
+ */
+# define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \
+ __FILE__, __FUNCTION__, __LINE__)
+
+/**
+ * VIR_STRDUP_QUIET:
+ * @dst: variable to hold result (char*, not char**)
+ * @src: string to duplicate
+ *
+ * Duplicate @src string and store it into @dst.
+ *
+ * Returns -1 on failure, 0 on success
+ */
+# define VIR_STRDUP_QUIET(dst, src) virStrdup(&(dst), src, false, 0, NULL, NULL, 0)
+
+/**
+ * VIR_STRNDUP:
+ * @dst: variable to hold result (char*, not char**)
+ * @src: string to duplicate
+ * @n: the maximum number of bytes to copy
+ *
+ * Duplicate @src string and store it into @dst. If @src is longer than @n,
+ * only @n bytes are copied and terminating null byte '\0' is added.
+ *
+ * Returns -1 on failure (with OOM error reported), 0 on success
+ */
+# define VIR_STRNDUP(dst, src, n) virStrndup(&(dst), src, n, true, \
+ VIR_FROM_THIS, __FILE__, \
+ __FUNCTION__, __LINE__)
+
+/**
+ * VIR_STRNDUP_QUIET:
+ * @dst: variable to hold result (char*, not char**)
+ * @src: string to duplicate
+ * @n: the maximum number of bytes to copy
+ *
+ * Duplicate @src string and store it into @dst. If @src is longer than @n,
+ * only @n bytes are copied and terminating null byte '\0' is added.
+ *
+ * Returns -1 on failure, 0 on success
+ */
+# define VIR_STRNDUP_QUIET(dst, src, n) virStrndup(&(dst), src, n, false, \
+ 0, NULL, NULL, 0)
#endif /* __VIR_STRING_H__ */
--
1.8.5.2
8:27 a.m.
New subject: [libvirt] [v0.9.12-maint 2/8] string: make VIR_STRDUP easier to use
From: Eric Blake <eblake(a)redhat.com>
While reviewing proposed VIR_STRDUP conversions, I've already noticed
several places that do:
if (str && VIR_STRDUP(dest, str) < 0)
which can be simplified by allowing str to be NULL (something that
strdup() doesn't allow). Meanwhile, code that wants to ensure a
non-NULL dest regardless of the source can check for <= 0.
Also, make it part of the VIR_STRDUP contract that macro arguments
are evaluated exactly once.
* src/util/virstring.h (VIR_STRDUP, VIR_STRDUP_QUIET, VIR_STRNDUP)
(VIR_STRNDUP_QUIET): Improve contract.
* src/util/virstring.c (virStrdup, virStrndup): Change return
conventions.
* docs/hacking.html.in: Document this.
* HACKING: Regenerate.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
Conflicts:
HACKING
docs/hacking.html.in
(cherry picked from commit 6b74a9f5d98e066f8dfdf5d5ccda68230b516246)
---
HACKING | 17 +++++++++++------
docs/hacking.html.in | 16 ++++++++++++----
src/util/virstring.c | 12 ++++++++----
src/util/virstring.h | 22 ++++++++++++++++------
4 files changed, 47 insertions(+), 20 deletions(-)
diff --git a/HACKING b/HACKING
index 5b5c34b..758d327 100644
--- a/HACKING
+++ b/HACKING
@@ -234,6 +234,11 @@ But if negating a complex condition is too ugly, then at least add
braces:
Preprocessor
============
+Macros defined with an ALL_CAPS name should generally be assumed to be unsafe
+with regards to arguments with side-effects (that is, MAX(a++, b--) might
+increment a or decrement b too many or too few times). Exceptions to this rule
+are explicitly documented for macros in viralloc.h and virstring.h.
+
For variadic macros, stick with C99 syntax:
#define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__)
@@ -539,12 +544,12 @@ virStrncpy(dest, src, strlen(src), sizeof(dest)).
VIR_STRNDUP(char *dst, const char *src, size_t n);
You should avoid using strdup or strndup directly as they do not report
-out-of-memory error. Use VIR_STRDUP or VIR_STRNDUP macros instead. Note, that
-these two behave similar to VIR_ALLOC: on success zero is returned, otherwise
-the result is -1 and dst is guaranteed to be NULL. In very specific cases,
-when you don't want to report the out-of-memory error, you can use
-VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and usually
-considered a flaw.
+out-of-memory error, and do not allow a NULL source. Use VIR_STRDUP or
+VIR_STRNDUP macros instead, which return 0 for NULL source, 1 for successful
+copy, and -1 for allocation failure with the error already reported. In very
+specific cases, when you don't want to report the out-of-memory error, you can
+use VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and
+usually considered a flaw.
Variable length string buffer
diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index 198afe7..d553f42 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -303,7 +303,14 @@
}
</pre>
- <h2><a href="types">Preprocessor</a></h2>
+ <h2><a name="preprocessor">Preprocessor</a></h2>
+
+ <p>Macros defined with an ALL_CAPS name should generally be
+ assumed to be unsafe with regards to arguments with side-effects
+ (that is, MAX(a++, b--) might increment a or decrement b too
+ many or too few times). Exceptions to this rule are explicitly
+ documented for macros in viralloc.h and virstring.h.
+ </p>
<p>
For variadic macros, stick with C99 syntax:
@@ -647,9 +654,10 @@
</pre>
<p>
You should avoid using strdup or strndup directly as they do not report
- out-of-memory error. Use VIR_STRDUP or VIR_STRNDUP macros instead. Note,
- that these two behave similar to VIR_ALLOC: on success zero is returned,
- otherwise the result is -1 and dst is guaranteed to be NULL. In very
+ out-of-memory error, and do not allow a NULL source. Use
+ VIR_STRDUP or VIR_STRNDUP macros instead, which return 0 for
+ NULL source, 1 for successful copy, and -1 for allocation
+ failure with the error already reported. In very
specific cases, when you don't want to report the out-of-memory error, you
can use VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare
and usually considered a flaw.
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 21142b4..915c771 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -194,7 +194,7 @@ size_t virStringListLength(char **strings)
* caller's body where virStrdup is called from. Consider
* using VIR_STRDUP which sets these automatically.
*
- * Returns: 0 on success, -1 otherwise.
+ * Returns: 0 for NULL src, 1 on successful copy, -1 otherwise.
*/
int
virStrdup(char **dest,
@@ -205,13 +205,15 @@ virStrdup(char **dest,
const char *funcname,
size_t linenr)
{
+ if (!src)
+ return 0;
if (!(*dest = strdup(src))) {
if (report)
virReportOOMErrorFull(domcode, filename, funcname, linenr);
return -1;
}
- return 0;
+ return 1;
}
/**
@@ -231,7 +233,7 @@ virStrdup(char **dest,
* caller's body where virStrndup is called from. Consider
* using VIR_STRNDUP which sets these automatically.
*
- * Returns: 0 on success, -1 otherwise.
+ * Returns: 0 for NULL src, 1 on successful copy, -1 otherwise.
*/
int
virStrndup(char **dest,
@@ -243,11 +245,13 @@ virStrndup(char **dest,
const char *funcname,
size_t linenr)
{
+ if (!src)
+ return 0;
if (!(*dest = strndup(src, n))) {
if (report)
virReportOOMErrorFull(domcode, filename, funcname, linenr);
return -1;
}
- return 0;
+ return 1;
}
diff --git a/src/util/virstring.h b/src/util/virstring.h
index cd5ccda..74164bf 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -40,11 +40,11 @@ size_t virStringListLength(char **strings);
/* Don't call these directly - use the macros below */
int virStrdup(char **dest, const char *src, bool report, int domcode,
const char *filename, const char *funcname, size_t linenr)
- ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+ ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1);
int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode,
const char *filename, const char *funcname, size_t linenr)
- ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+ ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1);
/**
* VIR_STRDUP:
@@ -53,7 +53,10 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int
domcode,
*
* Duplicate @src string and store it into @dst.
*
- * Returns -1 on failure (with OOM error reported), 0 on success
+ * This macro is safe to use on arguments with side effects.
+ *
+ * Returns -1 on failure (with OOM error reported), 0 if @src was NULL,
+ * 1 if @src was copied
*/
# define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \
__FILE__, __FUNCTION__, __LINE__)
@@ -65,7 +68,9 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int
domcode,
*
* Duplicate @src string and store it into @dst.
*
- * Returns -1 on failure, 0 on success
+ * This macro is safe to use on arguments with side effects.
+ *
+ * Returns -1 on failure, 0 if @src was NULL, 1 if @src was copied
*/
# define VIR_STRDUP_QUIET(dst, src) virStrdup(&(dst), src, false, 0, NULL, NULL, 0)
@@ -78,7 +83,10 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int
domcode,
* Duplicate @src string and store it into @dst. If @src is longer than @n,
* only @n bytes are copied and terminating null byte '\0' is added.
*
- * Returns -1 on failure (with OOM error reported), 0 on success
+ * This macro is safe to use on arguments with side effects.
+ *
+ * Returns -1 on failure (with OOM error reported), 0 if @src was NULL,
+ * 1 if @src was copied
*/
# define VIR_STRNDUP(dst, src, n) virStrndup(&(dst), src, n, true, \
VIR_FROM_THIS, __FILE__, \
@@ -93,7 +101,9 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int
domcode,
* Duplicate @src string and store it into @dst. If @src is longer than @n,
* only @n bytes are copied and terminating null byte '\0' is added.
*
- * Returns -1 on failure, 0 on success
+ * This macro is safe to use on arguments with side effects.
+ *
+ * Returns -1 on failure, 0 if @src was NULL, 1 if @src was copied
*/
# define VIR_STRNDUP_QUIET(dst, src, n) virStrndup(&(dst), src, n, false, \
0, NULL, NULL, 0)
--
1.8.5.2
8:27 a.m.
New subject: [libvirt] [v0.9.12-maint 3/8] string: test VIR_STRDUP
From: Eric Blake <eblake(a)redhat.com>
The surest way to avoid regressions is to test documented behavior :)
* tests/virstringtest.c (testStrdup): New test case.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
(cherry picked from commit 504b4a8dae06330ba1735a28c316c1c68a32c471)
---
tests/virstringtest.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 92 insertions(+), 1 deletion(-)
diff --git a/tests/virstringtest.c b/tests/virstringtest.c
index 7e726c6..aca79cd 100644
--- a/tests/virstringtest.c
+++ b/tests/virstringtest.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2012 Red Hat, Inc.
+ * Copyright (C) 2012-2013 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -108,6 +108,95 @@ cleanup:
return ret;
}
+static bool fail;
+
+static const char *
+testStrdupLookup1(size_t i)
+{
+ switch (i) {
+ case 0:
+ return "hello";
+ case 1:
+ return NULL;
+ default:
+ fail = true;
+ return "oops";
+ }
+}
+
+static size_t
+testStrdupLookup2(size_t i)
+{
+ if (i)
+ fail = true;
+ return 5;
+}
+
+static int
+testStrdup(const void *data ATTRIBUTE_UNUSED)
+{
+ char *array[] = { NULL, NULL };
+ size_t i = 0;
+ size_t j = 0;
+ size_t k = 0;
+ int ret = -1;
+ int value;
+
+ value = VIR_STRDUP(array[i++], testStrdupLookup1(j++));
+ if (value != 1) {
+ fprintf(stderr, "unexpected strdup result %d, expected 1\n", value);
+ goto cleanup;
+ }
+ if (i != 1) {
+ fprintf(stderr, "unexpected side effects i=%zu, expected 1\n", i);
+ goto cleanup;
+ }
+ if (j != 1) {
+ fprintf(stderr, "unexpected side effects j=%zu, expected 1\n", j);
+ goto cleanup;
+ }
+ if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) {
+ fprintf(stderr, "incorrect array contents '%s'
'%s'\n",
+ NULLSTR(array[0]), NULLSTR(array[1]));
+ goto cleanup;
+ }
+
+ value = VIR_STRNDUP(array[i++], testStrdupLookup1(j++),
+ testStrdupLookup2(k++));
+ if (value != 0) {
+ fprintf(stderr, "unexpected strdup result %d, expected 0\n", value);
+ goto cleanup;
+ }
+ if (i != 2) {
+ fprintf(stderr, "unexpected side effects i=%zu, expected 2\n", i);
+ goto cleanup;
+ }
+ if (j != 2) {
+ fprintf(stderr, "unexpected side effects j=%zu, expected 2\n", j);
+ goto cleanup;
+ }
+ if (k != 1) {
+ fprintf(stderr, "unexpected side effects k=%zu, expected 1\n", k);
+ goto cleanup;
+ }
+ if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) {
+ fprintf(stderr, "incorrect array contents '%s'
'%s'\n",
+ NULLSTR(array[0]), NULLSTR(array[1]));
+ goto cleanup;
+ }
+
+ if (fail) {
+ fprintf(stderr, "side effects failed\n");
+ goto cleanup;
+ }
+
+ ret = 0;
+cleanup:
+ for (i = 0; i < ARRAY_CARDINALITY(array); i++)
+ VIR_FREE(array[i]);
+ return ret;
+}
+
static int
mymain(void)
@@ -154,6 +243,8 @@ mymain(void)
const char *tokens7[] = { "The", "quick", "brown",
"fox", "", NULL };
TEST_SPLIT("The quick brown fox ", " ", 0, tokens7);
+ if (virtTestRun("strdup", 1, testStrdup, NULL) < 0)
+ ret = -1;
return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE;
}
--
1.8.5.2
8:27 a.m.
New subject: [libvirt] [v0.9.12-maint 4/8] Introduce virReportError macro for general error reporting
From: "Daniel P. Berrange" <berrange(a)redhat.com>
Nearly every source file does something like
#define VIR_FROM_THIS VIR_FROM_FOO
#define virFooReportErorr(code, ...) \
virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \
__FUNCTION__, __LINE__, \
__VA_ARGS__)
This creates needless duplication and inconsistent error
reporting function names in each file. It is trivial to
just have virterror_internal.h provide a virReportError
macro that is equivalent
* src/util/virterror_internal.h: Define virReportError(code, ...)
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
(cherry picked from commit 7e94acd4fc470ea46ab94f21f7fe718d512ca3b1)
---
cfg.mk | 1 +
src/util/virprocess.c | 3 ---
src/util/virterror_internal.h | 3 +++
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 67141a9..80fbd14 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -560,6 +560,7 @@ msg_gen_function += virNetworkReportError
msg_gen_function += virNodeDeviceReportError
msg_gen_function += virNWFilterReportError
msg_gen_function += virRaiseError
+msg_gen_function += virReportError
msg_gen_function += virReportErrorHelper
msg_gen_function += virReportSystemError
msg_gen_function += virSecretReportError
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index f05f8f3..9f5e093 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -40,9 +40,6 @@
#include "util.h"
#include "virstring.h"
#include "ignore-value.h"
-#define virReportError(code, ...) \
- virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \
- __FUNCTION__, __LINE__, __VA_ARGS__)
#define VIR_FROM_THIS VIR_FROM_NONE
diff --git a/src/util/virterror_internal.h b/src/util/virterror_internal.h
index b8cb279..2e8aaf8 100644
--- a/src/util/virterror_internal.h
+++ b/src/util/virterror_internal.h
@@ -76,6 +76,9 @@ void virReportOOMErrorFull(int domcode,
# define virReportOOMError() \
virReportOOMErrorFull(VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__)
+# define virReportError(code, ...) \
+ virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \
+ __FUNCTION__, __LINE__, __VA_ARGS__)
int virSetError(virErrorPtr newerr);
void virDispatchError(virConnectPtr conn);
--
1.8.5.2
8:27 a.m.
New subject: [libvirt] [v0.9.12-maint 5/8] qemu: Do not access stale data in virDomainBlockStats
From: Jiri Denemark <jdenemar(a)redhat.com>
CVE-2013-6458
https://bugzilla.redhat.com/show_bug.cgi?id=1043069
When virDomainDetachDeviceFlags is called concurrently to
virDomainBlockStats: libvirtd may crash because qemuDomainBlockStats
finds a disk in vm->def before getting a job on a domain and uses the
disk pointer after getting the job. However, the domain in unlocked
while waiting on a job condition and thus data behind the disk pointer
may disappear. This happens when thread 1 runs
virDomainDetachDeviceFlags and enters monitor to actually remove the
disk. Then another thread starts running virDomainBlockStats, finds the
disk in vm->def, and while it's waiting on the job condition (owned by
the first thread), the first thread finishes the disk removal. When the
second thread gets the job, the memory pointed to be the disk pointer is
already gone.
That said, every API that is going to begin a job should do that before
fetching data from vm->def.
Conflicts:
src/qemu/qemu_driver.c
(cherry picked from commit db86da5ca2109e4006c286a09b6c75bfe10676ad)
---
src/qemu/qemu_driver.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c0b4707..3f46c10 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7604,34 +7604,29 @@ qemuDomainBlockStats(virDomainPtr dom,
goto cleanup;
}
- if (!virDomainObjIsActive(vm)) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
+
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto endjob;
}
if ((i = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
- qemuReportError(VIR_ERR_INVALID_ARG,
- _("invalid path: %s"), path);
- goto cleanup;
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("invalid path: %s"), path);
+ goto endjob;
}
disk = vm->def->disks[i];
if (!disk->info.alias) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("missing disk device alias name for %s"),
disk->dst);
- goto cleanup;
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("missing disk device alias name for %s"),
disk->dst);
+ goto endjob;
}
priv = vm->privateData;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
- goto cleanup;
-
- if (!virDomainObjIsActive(vm)) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
- goto endjob;
- }
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetBlockStatsInfo(priv->mon,
--
1.8.5.2
8:27 a.m.
New subject: [libvirt] [v0.9.12-maint 6/8] qemu: Avoid using stale data in virDomainGetBlockInfo
From: Jiri Denemark <jdenemar(a)redhat.com>
CVE-2013-6458
Generally, every API that is going to begin a job should do that before
fetching data from vm->def. However, qemuDomainGetBlockInfo does not
know whether it will have to start a job or not before checking vm->def.
To avoid using disk alias that might have been freed while we were
waiting for a job, we use its copy. In case the disk was removed in the
meantime, we will fail with "cannot find statistics for device '...'"
error message.
Conflicts:
src/qemu/qemu_driver.c
(cherry picked from commit b799259583bd65c0b2f5042e6c3ff19637ade881)
---
src/qemu/qemu_driver.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3f46c10..b17aa09 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -92,6 +92,7 @@
#include "virnodesuspend.h"
#include "virtime.h"
#include "virtypedparam.h"
+#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -8409,10 +8410,12 @@ cleanup:
}
-static int qemuDomainGetBlockInfo(virDomainPtr dom,
- const char *path,
- virDomainBlockInfoPtr info,
- unsigned int flags) {
+static int
+qemuDomainGetBlockInfo(virDomainPtr dom,
+ const char *path,
+ virDomainBlockInfoPtr info,
+ unsigned int flags)
+{
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm;
int ret = -1;
@@ -8423,6 +8426,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,
struct stat sb;
int i;
int format;
+ char *alias = NULL;
virCheckFlags(0, -1);
@@ -8545,13 +8549,16 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,
virDomainObjIsActive(vm)) {
qemuDomainObjPrivatePtr priv = vm->privateData;
+ if (VIR_STRDUP(alias, disk->info.alias) < 0)
+ goto cleanup;
+
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
if (virDomainObjIsActive(vm)) {
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetBlockExtent(priv->mon,
- disk->info.alias,
+ alias,
&info->allocation);
qemuDomainObjExitMonitor(driver, vm);
} else {
@@ -8565,6 +8572,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,
}
cleanup:
+ VIR_FREE(alias);
virStorageFileFreeMetadata(meta);
VIR_FORCE_CLOSE(fd);
if (vm)
--
1.8.5.2
8:27 a.m.
New subject: [libvirt] [v0.9.12-maint 7/8] qemu: Fix job usage in qemuDomainBlockJobImpl
From: Jiri Denemark <jdenemar(a)redhat.com>
CVE-2013-6458
Every API that is going to begin a job should do that before fetching
data from vm->def.
Conflicts:
src/qemu/qemu_driver.c
(cherry picked from commit f93d2caa070f6197ab50d372d286018b0ba6bbd8)
---
src/qemu/qemu_driver.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b17aa09..f810275 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11749,11 +11749,6 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const
char *base,
goto cleanup;
}
- device = qemuDiskPathToAlias(vm, path, &idx);
- if (!device)
- goto cleanup;
- disk = vm->def->disks[idx];
-
if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
@@ -11763,6 +11758,11 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const
char *base,
goto endjob;
}
+ device = qemuDiskPathToAlias(vm, path, &idx);
+ if (!device)
+ goto endjob;
+ disk = vm->def->disks[idx];
+
qemuDomainObjEnterMonitorWithDriver(driver, vm);
/* XXX - libvirt should really be tracking the backing file chain
* itself, and validating that base is on the chain, rather than
--
1.8.5.2
8:27 a.m.
New subject: [libvirt] [v0.9.12-maint 8/8] qemu: Fix job usage in virDomainGetBlockIoTune
From: Jiri Denemark <jdenemar(a)redhat.com>
CVE-2013-6458
Every API that is going to begin a job should do that before fetching
data from vm->def.
Conflicts:
src/qemu/qemu_driver.c
(cherry picked from commit 3b56425938e2f97208d5918263efa0d6439e4ecd)
---
src/qemu/qemu_driver.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f810275..30b703a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12157,12 +12157,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
goto cleanup;
}
- device = qemuDiskPathToAlias(vm, disk, NULL);
-
- if (!device) {
- goto cleanup;
- }
-
if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
@@ -12170,6 +12164,11 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
&persistentDef) < 0)
goto endjob;
+ device = qemuDiskPathToAlias(vm, disk, NULL);
+ if (!device) {
+ goto endjob;
+ }
+
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
priv = vm->privateData;
qemuDomainObjEnterMonitorWithDriver(driver, vm);
--
1.8.5.2
2:43 p.m.
On 01/11/2014 07:27 AM, Guido Günther wrote:
Hi,
attached patches backport the fixes for CVE-2013-6458 to v0.9.12-maint. I
decided to cherry-pick the introduction of VIR_STRDUP and virReportError
as well to ease backporting of future fixes. I'd be happy about any review.
Looks correct to me. I'll let you push to 0.9.12-maint since you
already did that work; I already pushed to all the branches 0.10.2 and
later. When porting to 0.10.2, I chose to just inline the call to
strdup() instead of backporting VIR_STRDUP, for fewer patches but more
conflict resolution; but either approach seems acceptable.
Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so
we could argue that we should either fix the 0.9.11 branch, or add
another commit to the branch that explicitly marks it as end-of-life
because no one appears to be relying on it. Fedora 18 is now
end-of-life, so from Fedora's perspective, I only care about 0.10.2
(RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide),
although I didn't mind touching all the intermediate branches on my way
down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we
don't have an upstream v0.8.2-maint branch (thank goodness!), that's
something for Red Hat to worry about.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:47 p.m.
On 01/15/2014 01:43 PM, Eric Blake wrote:
On 01/11/2014 07:27 AM, Guido Günther wrote:
> Hi,
> attached patches backport the fixes for CVE-2013-6458 to v0.9.12-maint. I
> decided to cherry-pick the introduction of VIR_STRDUP and virReportError
> as well to ease backporting of future fixes. I'd be happy about any review.
Looks correct to me. I'll let you push to 0.9.12-maint since you
already did that work; I already pushed to all the branches 0.10.2 and
later. When porting to 0.10.2, I chose to just inline the call to
strdup() instead of backporting VIR_STRDUP, for fewer patches but more
conflict resolution; but either approach seems acceptable.
Oh, and I also pushed the two patches for CVE-2014-1447 to all branches
back to 0.10.2. Since that also exists in 0.9.8, you'll want to include
those two patches in your push to 0.9.12. There's a conflict resolution
needed in the first of the two patches, if you want to borrow from
0.10.2-maint.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:18 a.m.
On Wed, Jan 15, 2014 at 01:47:48PM -0700, Eric Blake wrote:
On 01/15/2014 01:43 PM, Eric Blake wrote:
> On 01/11/2014 07:27 AM, Guido Günther wrote:
>> Hi,
>> attached patches backport the fixes for CVE-2013-6458 to v0.9.12-maint. I
>> decided to cherry-pick the introduction of VIR_STRDUP and virReportError
>> as well to ease backporting of future fixes. I'd be happy about any review.
>
> Looks correct to me. I'll let you push to 0.9.12-maint since you
> already did that work; I already pushed to all the branches 0.10.2 and
> later. When porting to 0.10.2, I chose to just inline the call to
> strdup() instead of backporting VIR_STRDUP, for fewer patches but more
> conflict resolution; but either approach seems acceptable.
Oh, and I also pushed the two patches for CVE-2014-1447 to all branches
back to 0.10.2. Since that also exists in 0.9.8, you'll want to include
those two patches in your push to 0.9.12. There's a conflict resolution
needed in the first of the two patches, if you want to borrow from
0.10.2-maint.
I've cherry-picked these too and will tag a 0.9.12.3 during the next
days. Thanks a lot!
-- Guido
1:49 a.m.
On Wed, Jan 15, 2014 at 01:43:54PM -0700, Eric Blake wrote:
On 01/11/2014 07:27 AM, Guido Günther wrote:
> Hi,
> attached patches backport the fixes for CVE-2013-6458 to v0.9.12-maint. I
> decided to cherry-pick the introduction of VIR_STRDUP and virReportError
> as well to ease backporting of future fixes. I'd be happy about any review.
Looks correct to me. I'll let you push to 0.9.12-maint since you
already did that work; I already pushed to all the branches 0.10.2 and
later. When porting to 0.10.2, I chose to just inline the call to
strdup() instead of backporting VIR_STRDUP, for fewer patches but more
conflict resolution; but either approach seems acceptable.
Thanks for the review!
Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so
we could argue that we should either fix the 0.9.11 branch, or add
another commit to the branch that explicitly marks it as end-of-life
because no one appears to be relying on it. Fedora 18 is now
end-of-life, so from Fedora's perspective, I only care about 0.10.2
(RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide),
although I didn't mind touching all the intermediate branches on my way
down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we
don't have an upstream v0.8.2-maint branch (thank goodness!), that's
something for Red Hat to worry about.
I'd say let's close 0.9.11. We have 0.8.3 in Debian oldstable but I'm
not going to open a maint branch for this but deal with it in the
package itself.
Cheers,
-- Guido
1:13 p.m.
New subject: [libvirt] closing old maint branches [was: [v0.9.12-maint 0/8] Backport changes for CVE-2013-6458 to v0.9.12-maint]
On 01/15/2014 01:43 PM, Eric Blake wrote:
Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so
we could argue that we should either fix the 0.9.11 branch, or add
another commit to the branch that explicitly marks it as end-of-life
because no one appears to be relying on it. Fedora 18 is now
end-of-life, so from Fedora's perspective, I only care about 0.10.2
(RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide),
although I didn't mind touching all the intermediate branches on my way
down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we
don't have an upstream v0.8.2-maint branch (thank goodness!), that's
something for Red Hat to worry about.
I've gone ahead and marked v0.8.3-maint and v0.9.11-maint as closed (I'm
not posting the actual patch here, but it was done by 'git rm -f \*'
followed by recreating .gitignore and a placeholder README that mentions
the death of the branch).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:26 a.m.
New subject: [libvirt] closing old maint branches [was: [v0.9.12-maint 0/8] Backport changes for CVE-2013-6458 to v0.9.12-maint]
On Wed, Jan 22, 2014 at 12:13:48PM -0700, Eric Blake wrote:
On 01/15/2014 01:43 PM, Eric Blake wrote:
>
> Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so
> we could argue that we should either fix the 0.9.11 branch, or add
> another commit to the branch that explicitly marks it as end-of-life
> because no one appears to be relying on it. Fedora 18 is now
> end-of-life, so from Fedora's perspective, I only care about 0.10.2
> (RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide),
> although I didn't mind touching all the intermediate branches on my way
> down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we
> don't have an upstream v0.8.2-maint branch (thank goodness!), that's
> something for Red Hat to worry about.
I've gone ahead and marked v0.8.3-maint and v0.9.11-maint as closed (I'm
not posting the actual patch here, but it was done by 'git rm -f \*'
followed by recreating .gitignore and a placeholder README that mentions
the death of the branch).
FYI for openstack I examined the current libvirt versions in some
major distros:
https://wiki.openstack.org/wiki/LibvirtDistroSupportMatrix
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
6:47 a.m.
New subject: [libvirt] closing old maint branches [was: [v0.9.12-maint 0/8] Backport changes for CVE-2013-6458 to v0.9.12-maint]
On 01/23/2014 12:26 PM, Daniel P. Berrange wrote:
On Wed, Jan 22, 2014 at 12:13:48PM -0700, Eric Blake wrote:
> On 01/15/2014 01:43 PM, Eric Blake wrote:
>
>> Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so
>> we could argue that we should either fix the 0.9.11 branch, or add
>> another commit to the branch that explicitly marks it as end-of-life
>> because no one appears to be relying on it. Fedora 18 is now
>> end-of-life, so from Fedora's perspective, I only care about 0.10.2
>> (RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide),
>> although I didn't mind touching all the intermediate branches on my way
>> down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we
>> don't have an upstream v0.8.2-maint branch (thank goodness!), that's
>> something for Red Hat to worry about.
> I've gone ahead and marked v0.8.3-maint and v0.9.11-maint as closed (I'm
> not posting the actual patch here, but it was done by 'git rm -f \*'
> followed by recreating .gitignore and a placeholder README that mentions
> the death of the branch).
FYI for openstack I examined the current libvirt versions in some
major distros:
https://wiki.openstack.org/wiki/LibvirtDistroSupportMatrix
After seeing that list, I thought an "end of life" column could be
interesing, but then realized the only bit I was interested in was how
long we will need to put of with the oldest version on the list. As far
as I can tell, Ubuntu 12.04 LTS is scheduled for EOL in April 2017 (date
from here: https://wiki.ubuntu.com/Releases ), so I guess *somebody* has
to care about libvirt-0.9.8 until 2017 (of course we don't have a
v0.9.8-maint branch anyway, so that's not likely going to happen within
upstream infrastructure)
2:46 p.m.
New subject: [libvirt] closing old maint branches [was: [v0.9.12-maint 0/8] Backport changes for CVE-2013-6458 to v0.9.12-maint]
On Thu, Jan 23, 2014 at 10:26:14AM +0000, Daniel P. Berrange wrote:
On Wed, Jan 22, 2014 at 12:13:48PM -0700, Eric Blake wrote:
> On 01/15/2014 01:43 PM, Eric Blake wrote:
>
> >
> > Is anyone still using v0.9.11-maint? The CVE extends back to 0.9.8, so
> > we could argue that we should either fix the 0.9.11 branch, or add
> > another commit to the branch that explicitly marks it as end-of-life
> > because no one appears to be relying on it. Fedora 18 is now
> > end-of-life, so from Fedora's perspective, I only care about 0.10.2
> > (RHEL and CentOS 6), 1.0.5 (F19), 1.1.3 (F20) and soon 1.2.1 (rawhide),
> > although I didn't mind touching all the intermediate branches on my way
> > down to 0.10.2. RHEL 5 is also vulnerable to CVE-2013-6458, but as we
> > don't have an upstream v0.8.2-maint branch (thank goodness!), that's
> > something for Red Hat to worry about.
>
> I've gone ahead and marked v0.8.3-maint and v0.9.11-maint as closed (I'm
> not posting the actual patch here, but it was done by 'git rm -f \*'
> followed by recreating .gitignore and a placeholder README that mentions
> the death of the branch).
FYI for openstack I examined the current libvirt versions in some
major distros:
https://wiki.openstack.org/wiki/LibvirtDistroSupportMatrix
Great. I took the libverty to add wheezy-backports as well since we ship
updated versions from there and it's nowadays integrated into the main
Debian archive.
Cheers,
-- Guido
3928
days inactive
3940
days old
16 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Guido Günther -
Laine Stump