[libvirt] [PATCH 0/2] Resolve Coverity issues
The following patches resolve a couple of Coverity issues. The 'lxc' change is merely a work-around by adding a Coverity tag to avoid what I consider to be a false positive. Another option to resolving is to change the code in lxcContainerPrepareRoot() to assign 'tmp = root->src' and then VIR_FREE(tmp) instead. I have submitted a bug to Coverity for this. The issue first showed up earlier this month and I've tried various ways to resolve without much luck. The 'virsh' change is from a more recent change where the ':' option processing was changed in such a way that Coverity's analysis found a missing break. The loop in question should find a match with optopt, but if it didn't it would have fallen into the '?' code. John Ferlan (2): lxc: Coverity false positive USE_AFTER_FREE virsh: Resolve Coverity 'MISSING_BREAK' src/lxc/lxc_container.c | 5 +++++ tools/virsh.c | 10 +++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) -- 1.8.1.4
--- src/lxc/lxc_container.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index ac0f69c..d082a06 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1772,6 +1772,11 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, /* Some versions of Linux kernel don't let you overmount * the selinux filesystem, so make sure we kill it first */ + /* Filed coverity bug for false positive 'USE_AFTER_FREE' due to swap + * of root->src with root->dst and the VIR_FREE(root->src) prior to the + * reset of root->src in lxcContainerPrepareRoot() + */ + /* coverity[deref_arg] */ if (STREQ(root->src, "/") && lxcContainerUnmountSubtree(SELINUX_MOUNT, false) < 0) goto cleanup; -- 1.8.1.4
On 05/01/2013 11:53 AM, John Ferlan wrote:
--- src/lxc/lxc_container.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index ac0f69c..d082a06 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1772,6 +1772,11 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, /* Some versions of Linux kernel don't let you overmount * the selinux filesystem, so make sure we kill it first */ + /* Filed coverity bug for false positive 'USE_AFTER_FREE' due to swap + * of root->src with root->dst and the VIR_FREE(root->src) prior to the + * reset of root->src in lxcContainerPrepareRoot() + */ + /* coverity[deref_arg] */ if (STREQ(root->src, "/") && lxcContainerUnmountSubtree(SELINUX_MOUNT, false) < 0) goto cleanup;
ping? Any thoughts on this - either way? There is a Coverity bug filed, but it'll take some time to work through their system. Tks, John
On Wed, May 08, 2013 at 06:51:19AM -0400, John Ferlan wrote:
On 05/01/2013 11:53 AM, John Ferlan wrote:
--- src/lxc/lxc_container.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index ac0f69c..d082a06 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1772,6 +1772,11 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, /* Some versions of Linux kernel don't let you overmount * the selinux filesystem, so make sure we kill it first */ + /* Filed coverity bug for false positive 'USE_AFTER_FREE' due to swap + * of root->src with root->dst and the VIR_FREE(root->src) prior to the + * reset of root->src in lxcContainerPrepareRoot() + */ + /* coverity[deref_arg] */ if (STREQ(root->src, "/") && lxcContainerUnmountSubtree(SELINUX_MOUNT, false) < 0) goto cleanup;
ping?
Any thoughts on this - either way? There is a Coverity bug filed, but it'll take some time to work through their system.
ACK 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 :|
Recent commit '53531e16' resulted in a new Coverity warning regarding a missing break in the ':' options processing. Adjust the commit to avoid the issue. --- tools/virsh.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index ac86608..6ffa0ba 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3054,11 +3054,15 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) case ':': for (i = 0; opt[i].name != NULL; i++) { if (opt[i].val == optopt) { - vshError(ctl, _("option '-%c'/'--%s' requires an argument"), - optopt, opt[i].name); - exit(EXIT_FAILURE); + break; } } + if (opt[i].name) + vshError(ctl, _("option '-%c'/'--%s' requires an argument"), + optopt, opt[i].name); + else + vshError(ctl, _("option '-%c' requires an argument"), optopt); + exit(EXIT_FAILURE); case '?': if (optopt) vshError(ctl, _("unsupported option '-%c'. See --help."), optopt); -- 1.8.1.4
On 05/01/2013 05:53 PM, John Ferlan wrote:
Recent commit '53531e16' resulted in a new Coverity warning regarding a missing break in the ':' options processing. Adjust the commit to avoid the issue. --- tools/virsh.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index ac86608..6ffa0ba 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3054,11 +3054,15 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) case ':': for (i = 0; opt[i].name != NULL; i++) { if (opt[i].val == optopt) { - vshError(ctl, _("option '-%c'/'--%s' requires an argument"), - optopt, opt[i].name); - exit(EXIT_FAILURE); + break; }
You might want to remove the redundant braces.
} + if (opt[i].name) + vshError(ctl, _("option '-%c'/'--%s' requires an argument"), + optopt, opt[i].name); + else + vshError(ctl, _("option '-%c' requires an argument"), optopt); + exit(EXIT_FAILURE); case '?': if (optopt) vshError(ctl, _("unsupported option '-%c'. See --help."), optopt);
ACK. Jan
On 05/01/2013 11:53 AM, John Ferlan wrote:
The following patches resolve a couple of Coverity issues. The 'lxc' change is merely a work-around by adding a Coverity tag to avoid what I consider to be a false positive. Another option to resolving is to change the code in lxcContainerPrepareRoot() to assign 'tmp = root->src' and then VIR_FREE(tmp) instead. I have submitted a bug to Coverity for this. The issue first showed up earlier this month and I've tried various ways to resolve without much luck. The 'virsh' change is from a more recent change where the ':' option processing was changed in such a way that Coverity's analysis found a missing break. The loop in question should find a match with optopt, but if it didn't it would have fallen into the '?' code.
John Ferlan (2): lxc: Coverity false positive USE_AFTER_FREE virsh: Resolve Coverity 'MISSING_BREAK'
src/lxc/lxc_container.c | 5 +++++ tools/virsh.c | 10 +++++++--- 2 files changed, 12 insertions(+), 3 deletions(-)
Pushed w/ adj to 1/2 to remove extraneous {} Thanks, John
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Ján Tomko