[libvirt] [PATCH] Fix a misuse of virAsprintf in qemudDomainMemoryPeek
The code specifies driver->cacheDir as the format string, but it usually doesn't contain '%s', so the subsequent argument, "/qemu.mem.XXXXXX", is always ignored. The patch fixes the misuse. --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bb1079e..843f827 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9033,7 +9033,7 @@ qemudDomainMemoryPeek (virDomainPtr dom, goto endjob; } - if (virAsprintf(&tmp, driver->cacheDir, "/qemu.mem.XXXXXX") < 0) { + if (virAsprintf(&tmp, "%s/qemu.mem.XXXXXX", driver->cacheDir) < 0) { virReportOOMError(); goto endjob; } -- 1.6.5.2
On 05/14/2010 04:08 AM, Ryota Ozaki wrote:
The code specifies driver->cacheDir as the format string, but it usually doesn't contain '%s', so the subsequent argument, "/qemu.mem.XXXXXX", is always ignored.
The patch fixes the misuse. --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bb1079e..843f827 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9033,7 +9033,7 @@ qemudDomainMemoryPeek (virDomainPtr dom, goto endjob; }
- if (virAsprintf(&tmp, driver->cacheDir, "/qemu.mem.XXXXXX") < 0) { + if (virAsprintf(&tmp, "%s/qemu.mem.XXXXXX", driver->cacheDir) < 0) {
ACK. Even worse, if driver->cacheDir contains %n, we have an exploitable security hole. Why didn't gcc -Wformat catch this one? Oh, because it doesn't warn on non-literal formats. So why didn't -Wformat-nonliteral catch it? Oh, because we don't turn it on, since we have other (provably safe) non-literals that would trip it up. Maybe it's time to play with the appropriate '#pragma gcc' to temporarily disable -Wformat-nonliteral around just the places audited to be safe, if we detect at configure time that we have new-enough gcc? Pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/14/2010 10:58 AM, Eric Blake wrote:
Why didn't gcc -Wformat catch this one? Oh, because it doesn't warn on non-literal formats. So why didn't -Wformat-nonliteral catch it? Oh, because we don't turn it on, since we have other (provably safe) non-literals that would trip it up. Maybe it's time to play with the appropriate '#pragma gcc' to temporarily disable -Wformat-nonliteral around just the places audited to be safe, if we detect at configure time that we have new-enough gcc?
I'm not sure when #pragma GCC diagnostic ignored -Wformat-literal was first supported, but unfortunately, gcc documents that it is an all-or-nothing choice that must be made at the front of each compilation unit. In other words, it's not something we can temporarily enable around just the functions that need it. Then again, since automake doesn't (yet) support per-file CFLAGS granularity (only per-target, where target is program or library), and creating convenience libraries around one file just to get per-target support to disable a diagnostic for that file seems painful. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Ryota Ozaki