[libvirt] [PATCH 0/5] Clean up after recent Coverity scan
This is the last round of cleanup with a few Coverity errors from the tests trees and a couple that just showed up in my recent scans within security and virsh. The commandtest still has Valgrind errors, but errors are not new to this change. After this round of Coverity changes, the only remaining Coverity warnings reported are within the gnulib tree. A few have been there all along within vasnprintf.c, while others are new with the recent gnulib update (regexec.c, regex_internal.c, and regcomp.c). Once these are submitted I will have a baseline from which I can make periodic comparisons. Since Coverity will sometimes "find" issues unrelated to recent changes, using git bisect to point the fickle finger of fate at a recent submission may not be possible. John Ferlan (5): drivermodule: Ignore coverity warning about leaked_storage commandtest: Resolve some coverity resource leaks vircommand: Remove unnecessary sa_assert virsh: Resolve possible NULL dereference security: Remove unnecessary checks for mgr == NULL src/security/security_dac.c | 6 ------ src/security/security_selinux.c | 6 ------ src/util/vircommand.c | 3 --- tests/commandtest.c | 17 ++++++++++++----- tests/virdrivermoduletest.c | 2 ++ tools/virsh.c | 4 ++-- 6 files changed, 16 insertions(+), 22 deletions(-) -- 1.7.11.7
--- tests/virdrivermoduletest.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/virdrivermoduletest.c b/tests/virdrivermoduletest.c index de8cf70..bbceb96 100644 --- a/tests/virdrivermoduletest.c +++ b/tests/virdrivermoduletest.c @@ -39,10 +39,12 @@ static int testDriverModule(const void *args) { const struct testDriverData *data = args; + /* coverity[leaked_storage] */ if (data->dep1 && !virDriverLoadModule(data->dep1)) return -1; + /* coverity[leaked_storage] */ if (!virDriverLoadModule(data->name)) return -1; -- 1.7.11.7
On 02/14/13 17:42, John Ferlan wrote:
--- tests/virdrivermoduletest.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tests/virdrivermoduletest.c b/tests/virdrivermoduletest.c index de8cf70..bbceb96 100644 --- a/tests/virdrivermoduletest.c +++ b/tests/virdrivermoduletest.c @@ -39,10 +39,12 @@ static int testDriverModule(const void *args) { const struct testDriverData *data = args;
+ /* coverity[leaked_storage] */
I presume the leaked storage is from the one shot initializers.
if (data->dep1 && !virDriverLoadModule(data->dep1)) return -1;
+ /* coverity[leaked_storage] */ if (!virDriverLoadModule(data->name)) return -1;
ACK. Peter
--- tests/commandtest.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 93c6333..3bfc358 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -680,7 +680,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - if (!outbuf || *outbuf) { + if (*outbuf) { puts("output buffer is not an allocated empty string"); goto cleanup; } @@ -702,7 +702,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } - if (!outbuf || *outbuf || !errbuf || *errbuf) { + if (*outbuf || *errbuf) { puts("output buffers are not allocated empty strings"); goto cleanup; } @@ -936,6 +936,7 @@ mymain(void) int fd; virCommandTestDataPtr test = NULL; int timer = -1; + int virinitret; if (virThreadInitialize() < 0) return EXIT_FAILURE; @@ -963,16 +964,19 @@ mymain(void) dup2(fd, 6) < 0 || dup2(fd, 7) < 0 || dup2(fd, 8) < 0 || - (fd > 8 && VIR_CLOSE(fd) < 0)) + (fd > 8 && VIR_CLOSE(fd) < 0)) { + VIR_FORCE_CLOSE(fd); return EXIT_FAILURE; + } + sa_assert(fd == -1); /* Prime the debug/verbose settings from the env vars, * since we're about to reset 'environ' */ ignore_value(virTestGetDebug()); ignore_value(virTestGetVerbose()); - if (virInitialize() < 0) - return EXIT_FAILURE; + /* Make sure to not leak fd's */ + virinitret = virInitialize(); /* Phase two of killing interfering fds; see above. */ fd = 3; @@ -988,6 +992,9 @@ mymain(void) fd = 8; VIR_FORCE_CLOSE(fd); + if (virinitret < 0) + return EXIT_FAILURE; + virEventRegisterDefaultImpl(); if (VIR_ALLOC(test) < 0) { virReportOOMError(); -- 1.7.11.7
On 02/14/13 17:42, John Ferlan wrote:
--- tests/commandtest.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c index 93c6333..3bfc358 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c
...
@@ -963,16 +964,19 @@ mymain(void) dup2(fd, 6) < 0 || dup2(fd, 7) < 0 || dup2(fd, 8) < 0 || - (fd > 8 && VIR_CLOSE(fd) < 0)) + (fd > 8 && VIR_CLOSE(fd) < 0)) { + VIR_FORCE_CLOSE(fd); return EXIT_FAILURE; + } + sa_assert(fd == -1);
according to man of dup2(): * If oldfd is a valid file descriptor, and newfd has the same value as oldfd, then dup2() does nothing, and returns newfd. It is possible that open returns fd < 8, dup2() on that does nothing and afterwards this assertion won't be true.
/* Prime the debug/verbose settings from the env vars, * since we're about to reset 'environ' */ ignore_value(virTestGetDebug()); ignore_value(virTestGetVerbose());
- if (virInitialize() < 0) - return EXIT_FAILURE; + /* Make sure to not leak fd's */ + virinitret = virInitialize();
/* Phase two of killing interfering fds; see above. */ fd = 3;
ACK with the assertion removed or a sufficient explanation provided. Peter
On 02/14/2013 10:15 AM, Peter Krempa wrote:
On 02/14/13 17:42, John Ferlan wrote:
--- tests/commandtest.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c index 93c6333..3bfc358 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c
...
@@ -963,16 +964,19 @@ mymain(void) dup2(fd, 6) < 0 || dup2(fd, 7) < 0 || dup2(fd, 8) < 0 || - (fd > 8 && VIR_CLOSE(fd) < 0)) + (fd > 8 && VIR_CLOSE(fd) < 0)) { + VIR_FORCE_CLOSE(fd); return EXIT_FAILURE; + } + sa_assert(fd == -1);
according to man of dup2(): * If oldfd is a valid file descriptor, and newfd has the same value as oldfd, then dup2() does nothing, and returns newfd.
It is possible that open returns fd < 8, dup2() on that does nothing and afterwards this assertion won't be true.
I gave my ACK too soon - Peter is right that fd _might_ not be -1 at this point. There are two cases to consider: 1. start life with fds 3-8 already opened by inheritance (rare). Opening /dev/null gets fd 9, which we then dup to 3-8 (overwriting whatever was inherited from the environment), then we close 9. If we fail to dup2, we still close 9, but none of the other fds which we forcefully overwrote. In this case, fd==-1 is true when you get to the sa_assert. 2. start life with at least one of fd 3-8 closed (most common). Opening /dev/null then gets the first open fd, which we then dup to all the remaining fds between 3-8. We don't close fd in this case, because we want fds 3-8 opened on /dev/null; so in this case, fd==-1 is false. Does the assert actually fix anything? I think it is best to remove it.
/* Prime the debug/verbose settings from the env vars, * since we're about to reset 'environ' */ ignore_value(virTestGetDebug()); ignore_value(virTestGetVerbose());
- if (virInitialize() < 0) - return EXIT_FAILURE; + /* Make sure to not leak fd's */ + virinitret = virInitialize();
/* Phase two of killing interfering fds; see above. */ fd = 3;
ACK with the assertion removed or a sufficient explanation provided.
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/14/2013 12:43 PM, Eric Blake wrote:
On 02/14/2013 10:15 AM, Peter Krempa wrote:
On 02/14/13 17:42, John Ferlan wrote:
--- tests/commandtest.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c index 93c6333..3bfc358 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c
...
@@ -963,16 +964,19 @@ mymain(void) dup2(fd, 6) < 0 || dup2(fd, 7) < 0 || dup2(fd, 8) < 0 || - (fd > 8 && VIR_CLOSE(fd) < 0)) + (fd > 8 && VIR_CLOSE(fd) < 0)) { + VIR_FORCE_CLOSE(fd); return EXIT_FAILURE; + } + sa_assert(fd == -1);
according to man of dup2(): * If oldfd is a valid file descriptor, and newfd has the same value as oldfd, then dup2() does nothing, and returns newfd.
It is possible that open returns fd < 8, dup2() on that does nothing and afterwards this assertion won't be true.
I gave my ACK too soon - Peter is right that fd _might_ not be -1 at this point. There are two cases to consider:
1. start life with fds 3-8 already opened by inheritance (rare). Opening /dev/null gets fd 9, which we then dup to 3-8 (overwriting whatever was inherited from the environment), then we close 9. If we fail to dup2, we still close 9, but none of the other fds which we forcefully overwrote. In this case, fd==-1 is true when you get to the sa_assert.
2. start life with at least one of fd 3-8 closed (most common). Opening /dev/null then gets the first open fd, which we then dup to all the remaining fds between 3-8. We don't close fd in this case, because we want fds 3-8 opened on /dev/null; so in this case, fd==-1 is false.
Does the assert actually fix anything? I think it is best to remove it.
Without the sa_assert, the following is coughed up by Coverity: 979 980 /* Phase two of killing interfering fds; see above. */ (20) Event overwrite_var: Overwriting handle "fd" in "fd = 3" leaks the handle. Also see events: [open_fn][var_assign][noescape][noescape][noescape][noescape][noescape][noescape] 981 fd = 3; 982 VIR_FORCE_CLOSE(fd); When I first modified this code it was still fd 3->5 being dup2'd and the sa_assert() worked for at least shutting coverity up, but I see the point about the last if condition. So, one could logically believe the check could change to: sa_assert(fd == -1 || (fd >= 3 && fd <= 8)); but that too triggers Coverity to complain that we're overwriting the fd. So without creating a new variable and adding more busy work, adding the following prior to the initialization removes the warning. /* coverity[overwrite_var] - silence the obvious */ fd = 3; Also, yes, it's strange on the error case of the if condition that one doesn't have to do the same close 3->8 game even though the VIR_FORCE_CLOSE(fd) was needed.
/* Prime the debug/verbose settings from the env vars, * since we're about to reset 'environ' */ ignore_value(virTestGetDebug()); ignore_value(virTestGetVerbose());
- if (virInitialize() < 0) - return EXIT_FAILURE; + /* Make sure to not leak fd's */ + virinitret = virInitialize();
/* Phase two of killing interfering fds; see above. */ fd = 3;
ACK with the assertion removed or a sufficient explanation provided.
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/14/13 20:00, John Ferlan wrote:
On 02/14/2013 12:43 PM, Eric Blake wrote:
On 02/14/2013 10:15 AM, Peter Krempa wrote:
On 02/14/13 17:42, John Ferlan wrote:
--- tests/commandtest.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c index 93c6333..3bfc358 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c
...
@@ -963,16 +964,19 @@ mymain(void) dup2(fd, 6) < 0 || dup2(fd, 7) < 0 || dup2(fd, 8) < 0 || - (fd > 8 && VIR_CLOSE(fd) < 0)) + (fd > 8 && VIR_CLOSE(fd) < 0)) { + VIR_FORCE_CLOSE(fd); return EXIT_FAILURE; + } + sa_assert(fd == -1);
according to man of dup2(): * If oldfd is a valid file descriptor, and newfd has the same value as oldfd, then dup2() does nothing, and returns newfd.
It is possible that open returns fd < 8, dup2() on that does nothing and afterwards this assertion won't be true.
I gave my ACK too soon - Peter is right that fd _might_ not be -1 at this point. There are two cases to consider:
1. start life with fds 3-8 already opened by inheritance (rare). Opening /dev/null gets fd 9, which we then dup to 3-8 (overwriting whatever was inherited from the environment), then we close 9. If we fail to dup2, we still close 9, but none of the other fds which we forcefully overwrote. In this case, fd==-1 is true when you get to the sa_assert.
2. start life with at least one of fd 3-8 closed (most common). Opening /dev/null then gets the first open fd, which we then dup to all the remaining fds between 3-8. We don't close fd in this case, because we want fds 3-8 opened on /dev/null; so in this case, fd==-1 is false.
Does the assert actually fix anything? I think it is best to remove it.
Without the sa_assert, the following is coughed up by Coverity:
979 980 /* Phase two of killing interfering fds; see above. */
(20) Event overwrite_var: Overwriting handle "fd" in "fd = 3" leaks the handle.
From a human point of view this is a false positive: 1) In case fd <= 8, it gets dup2()-ed to itself and we close it later. 2) In case fd > we close it anyway But the analyzer has hard time following our manual closing of the handles afterwards.
Also see events: [open_fn][var_assign][noescape][noescape][noescape][noescape][noescape][noescape]
981 fd = 3; 982 VIR_FORCE_CLOSE(fd);
When I first modified this code it was still fd 3->5 being dup2'd and the sa_assert() worked for at least shutting coverity up, but I see the point about the last if condition.
So, one could logically believe the check could change to:
sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
Hm, I think I know why this shuts coverity up: the assertion statement forces coverity to think that the last part of the if statement was true and thus fd was explicitly closed. In this case, I think it's okay to use it to shut coverity up as long as "sa_assert" doesn't have any effect outside of static analysis. We know that we're doing the right thing with the fds so no need for coverity to whine about that. I don't know if this is worth bothering any more and even filing a bug against coverity. We're doing some pretty ugly stuff here from the POV of static analysis.
but that too triggers Coverity to complain that we're overwriting the fd. So without creating a new variable and adding more busy work, adding the following prior to the initialization removes the warning.
/* coverity[overwrite_var] - silence the obvious */ fd = 3;
Also, yes, it's strange on the error case of the if condition that one doesn't have to do the same close 3->8 game even though the VIR_FORCE_CLOSE(fd) was needed.
/* Prime the debug/verbose settings from the env vars, * since we're about to reset 'environ' */ ignore_value(virTestGetDebug()); ignore_value(virTestGetVerbose());
- if (virInitialize() < 0) - return EXIT_FAILURE; + /* Make sure to not leak fd's */ + virinitret = virInitialize();
/* Phase two of killing interfering fds; see above. */ fd = 3;
ACK with the assertion removed or a sufficient explanation provided.
I think this is a sufficient explanation, so ACK (as long as sa_assert has no side effects normally).
Peter
On 02/15/2013 02:01 AM, Peter Krempa wrote:
So, one could logically believe the check could change to:
sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
Yes, I like that.
ACK with the assertion removed or a sufficient explanation provided.
I think this is a sufficient explanation, so ACK (as long as sa_assert has no side effects normally).
sa_assert is a no-op when static analysis is not being performed. That's why we prefer it over raw assert(), because sa_assert() does not change based on NDEBUG, only based on toolchain. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/15/13 14:17, Eric Blake wrote:
On 02/15/2013 02:01 AM, Peter Krempa wrote:
So, one could logically believe the check could change to:
sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
Yes, I like that.
If I understood it correctly, the above condition won't shut up coverity, only sa_assert(fd == -1) does as coverity then thinks that fd was > 8 and thus closed. Otherwise it does not detect the magic we're doing later. Peter
On 02/15/2013 08:54 AM, Peter Krempa wrote:
On 02/15/13 14:17, Eric Blake wrote:
On 02/15/2013 02:01 AM, Peter Krempa wrote:
So, one could logically believe the check could change to:
sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
Yes, I like that.
If I understood it correctly, the above condition won't shut up coverity, only sa_assert(fd == -1) does as coverity then thinks that fd was > 8 and thus closed. Otherwise it does not detect the magic we're doing later.
Yes, correct, hence the need for the "/* coverity[overwrite_var] */ tag. Coverity will try 2 paths and only tell you when it fails. I figured that out when going back through this exercise based on the review. Paying close attention to trail of messages it leaves is the key - when I first made changes to this code - I'm not sure I picked that up. John
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/15/2013 08:12 AM, John Ferlan wrote:
On 02/15/2013 08:54 AM, Peter Krempa wrote:
On 02/15/13 14:17, Eric Blake wrote:
On 02/15/2013 02:01 AM, Peter Krempa wrote:
So, one could logically believe the check could change to:
sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
Yes, I like that.
If I understood it correctly, the above condition won't shut up coverity, only sa_assert(fd == -1) does as coverity then thinks that fd was > 8 and thus closed. Otherwise it does not detect the magic we're doing later.
Yes, correct, hence the need for the "/* coverity[overwrite_var] */ tag.
If you're using the /* coverity[overwrite_var] */ tag, then do we still need the sa_assert? This is one case where leaving comments to shut up coverity is fair game, because it is a test program, and because we already know we are doing some unusual games with fds to get into a known state. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/15/2013 10:44 AM, Eric Blake wrote:
On 02/15/2013 08:12 AM, John Ferlan wrote:
On 02/15/2013 08:54 AM, Peter Krempa wrote:
On 02/15/13 14:17, Eric Blake wrote:
On 02/15/2013 02:01 AM, Peter Krempa wrote:
So, one could logically believe the check could change to:
sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
Yes, I like that.
If I understood it correctly, the above condition won't shut up coverity, only sa_assert(fd == -1) does as coverity then thinks that fd was > 8 and thus closed. Otherwise it does not detect the magic we're doing later.
Yes, correct, hence the need for the "/* coverity[overwrite_var] */ tag.
If you're using the /* coverity[overwrite_var] */ tag, then do we still need the sa_assert? This is one case where leaving comments to shut up coverity is fair game, because it is a test program, and because we already know we are doing some unusual games with fds to get into a known state.
The sa_assert() would not be required. I think by setting to just -1, Coverity chose to not check fd >=3 && <=8. That is perhaps it "tells" Coverity that we know our inputs and we're guaranteeing that the VIR_CLOSE will happen. I will remove the sa_assert and keep the comment. John
On 02/14/2013 09:42 AM, John Ferlan wrote:
--- tests/commandtest.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c index 93c6333..3bfc358 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -680,7 +680,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED) goto cleanup; }
- if (!outbuf || *outbuf) { + if (*outbuf) { puts("output buffer is not an allocated empty string"); goto cleanup; } @@ -702,7 +702,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED) goto cleanup; }
- if (!outbuf || *outbuf || !errbuf || *errbuf) { + if (*outbuf || *errbuf) { puts("output buffers are not allocated empty strings"); goto cleanup;
This part seems reasonable.
} @@ -936,6 +936,7 @@ mymain(void) int fd; virCommandTestDataPtr test = NULL; int timer = -1; + int virinitret;
if (virThreadInitialize() < 0) return EXIT_FAILURE; @@ -963,16 +964,19 @@ mymain(void) dup2(fd, 6) < 0 || dup2(fd, 7) < 0 || dup2(fd, 8) < 0 || - (fd > 8 && VIR_CLOSE(fd) < 0)) + (fd > 8 && VIR_CLOSE(fd) < 0)) { + VIR_FORCE_CLOSE(fd); return EXIT_FAILURE;
Odd that Coverity complains about the leak of fd, but not about the leak of all the other dup2'd fds; and at any rate, the leak is inconsequential since we are about to exit. But if this shuts things up, I'm okay with it.
+ } + sa_assert(fd == -1);
Yes, this is true, and if it helps Coverity, I'm fine with it.
/* Prime the debug/verbose settings from the env vars, * since we're about to reset 'environ' */ ignore_value(virTestGetDebug()); ignore_value(virTestGetVerbose());
- if (virInitialize() < 0) - return EXIT_FAILURE; + /* Make sure to not leak fd's */ + virinitret = virInitialize();
/* Phase two of killing interfering fds; see above. */ fd = 3; @@ -988,6 +992,9 @@ mymain(void) fd = 8; VIR_FORCE_CLOSE(fd);
+ if (virinitret < 0) + return EXIT_FAILURE;
Seems okay to me. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Changes from commit '3178df9a' removed the need for the sa_assert(infd). --- src/util/vircommand.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 5837fee..aa3bce0 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1853,9 +1853,6 @@ virCommandProcessIO(virCommandPtr cmd) fds[i].fd == cmd->inpipe) { int done; - /* Coverity 5.3.0 can't see that we only get here if - * infd is in the set because it was non-negative. */ - sa_assert(infd != -1); done = write(cmd->inpipe, cmd->inbuf + inoff, inlen - inoff); if (done < 0) { -- 1.7.11.7
On 02/14/13 17:42, John Ferlan wrote:
Changes from commit '3178df9a' removed the need for the sa_assert(infd). --- src/util/vircommand.c | 3 --- 1 file changed, 3 deletions(-)
ACK. I'm not able to verify if it's not needed anymore but it doesn't break the code. Peter
Coverity found that commit '41b5e845' had a possible NULL dereference on arg->data processing --- tools/virsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 4f75e8e..f5a01b3 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1,7 +1,7 @@ /* * virsh.c: a shell to exercise the libvirt API * - * Copyright (C) 2005, 2007-2012 Red Hat, Inc. + * Copyright (C) 2005, 2007-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 @@ -1475,7 +1475,7 @@ vshCommandOptStringReq(vshControl *ctl, if (!arg->data) error = N_("Programming error: Requested option is a boolean"); - if (!*arg->data && !(arg->def->flags & VSH_OFLAG_EMPTY_OK)) + if (arg->data && !*arg->data && !(arg->def->flags & VSH_OFLAG_EMPTY_OK)) error = N_("Option argument is empty"); if (error) { -- 1.7.11.7
On 02/14/13 17:42, John Ferlan wrote:
Coverity found that commit '41b5e845' had a possible NULL dereference on arg->data processing --- tools/virsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 4f75e8e..f5a01b3 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1,7 +1,7 @@ /* * virsh.c: a shell to exercise the libvirt API * - * Copyright (C) 2005, 2007-2012 Red Hat, Inc. + * Copyright (C) 2005, 2007-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 @@ -1475,7 +1475,7 @@ vshCommandOptStringReq(vshControl *ctl, if (!arg->data) error = N_("Programming error: Requested option is a boolean");
- if (!*arg->data && !(arg->def->flags & VSH_OFLAG_EMPTY_OK)) + if (arg->data && !*arg->data && !(arg->def->flags & VSH_OFLAG_EMPTY_OK))
Ah, yeah, that assignment doesn't skip this condition. I missed that while writing it.
error = N_("Option argument is empty");
if (error) {
ACK. Peter
Coverity found the DACGenLabel was checking for mgr == NULL after a possible dereference; however, in order to get into the function the virSecurityManagerGenLabel would have already dereferenced sec_managers[i] so the check was unnecessary. Same check is made in SELinuxGenSecurityLabel. --- src/security/security_dac.c | 6 ------ src/security/security_selinux.c | 6 ------ 2 files changed, 12 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b115bb0..0b274b7 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -913,12 +913,6 @@ virSecurityDACGenLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr seclabel; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - if (mgr == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("invalid security driver")); - return rc; - } - seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (seclabel == NULL) { return rc; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index a61e0f0..a042b26 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -560,12 +560,6 @@ virSecuritySELinuxGenSecurityLabel(virSecurityManagerPtr mgr, virSecuritySELinuxDataPtr data; const char *baselabel; - if (mgr == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("invalid security driver")); - return rc; - } - seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (seclabel == NULL) { return rc; -- 1.7.11.7
On 02/14/13 17:42, John Ferlan wrote:
Coverity found the DACGenLabel was checking for mgr == NULL after a possible dereference; however, in order to get into the function the virSecurityManagerGenLabel would have already dereferenced sec_managers[i] so the check was unnecessary. Same check is made in SELinuxGenSecurityLabel. --- src/security/security_dac.c | 6 ------ src/security/security_selinux.c | 6 ------ 2 files changed, 12 deletions(-)
Yep, probably a relic since layered security drivers were added. ACK. Peter
On 02/14/2013 11:42 AM, John Ferlan wrote:
src/security/security_dac.c | 6 ------ src/security/security_selinux.c | 6 ------ src/util/vircommand.c | 3 --- tests/commandtest.c | 17 ++++++++++++----- tests/virdrivermoduletest.c | 2 ++ tools/virsh.c | 4 ++-- 6 files changed, 16 insertions(+), 22 deletions(-)
Thanks for the reviews, changes were pushed. John
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa