[libvirt] [PATCH] Fix misc bugs in virCommandPtr
The virCommandNewArgs() method would free the virCommandPtr if it failed to add the args. This meant errors reported in virCommandAddArgSet() were lost. Simply removing the check for errors from the constructor means they can be reported correctly later The virCommandAddEnvPassCommon() method failed to check for errors before reallocating the cmd->env array, causing a potential SEGV if cmd was NULL The virCommandAddArgSet() method needs to validate that at least 1 element in 'val's parameter is non-NULL, otherwise code like cmd = virCommandNew(binary) virCommandAddAtg(cmd, "foo") Would end up trying todo execve("foo"), if binary was NULL. --- src/util/command.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index ff2bd46..22f350b 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -109,11 +109,6 @@ virCommandNewArgs(const char *const*args) virCommandAddArgSet(cmd, args); - if (cmd->has_error) { - virCommandFree(cmd); - return NULL; - } - return cmd; } @@ -362,6 +357,9 @@ virCommandAddEnvPass(virCommandPtr cmd, const char *name) void virCommandAddEnvPassCommon(virCommandPtr cmd) { + if (!cmd || cmd->has_error) + return; + /* Attempt to Pre-allocate; allocation failure will be detected * later during virCommandAdd*. */ ignore_value(VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 9)); @@ -478,6 +476,11 @@ virCommandAddArgSet(virCommandPtr cmd, const char *const*vals) if (!cmd || cmd->has_error) return; + if (vals[0] == NULL) { + cmd->has_error = EINVAL; + return; + } + while (vals[narg] != NULL) narg++; -- 1.7.4
On 03/15/2011 08:32 AM, Daniel P. Berrange wrote:
The virCommandNewArgs() method would free the virCommandPtr if it failed to add the args. This meant errors reported in virCommandAddArgSet() were lost. Simply removing the check for errors from the constructor means they can be reported correctly later
The virCommandAddEnvPassCommon() method failed to check for errors before reallocating the cmd->env array, causing a potential SEGV if cmd was NULL
The virCommandAddArgSet() method needs to validate that at least 1 element in 'val's parameter is non-NULL, otherwise code like
cmd = virCommandNew(binary) virCommandAddAtg(cmd, "foo")
Would end up trying todo execve("foo"), if binary was NULL. --- src/util/command.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-)
Those all make sense to me. ACK.
On 03/15/2011 06:32 AM, Daniel P. Berrange wrote:
The virCommandNewArgs() method would free the virCommandPtr if it failed to add the args. This meant errors reported in virCommandAddArgSet() were lost. Simply removing the check for errors from the constructor means they can be reported correctly later
The virCommandAddEnvPassCommon() method failed to check for errors before reallocating the cmd->env array, causing a potential SEGV if cmd was NULL
The virCommandAddArgSet() method needs to validate that at least 1 element in 'val's parameter is non-NULL, otherwise code like
cmd = virCommandNew(binary) virCommandAddAtg(cmd, "foo")
Would end up trying todo execve("foo"), if binary was NULL.
Well, technically virCommandNew is ATTRIBUTE_NONNULL(1), so we would have caught this via clang (gcc's not quite as smart as clang at enforcing that parameter). But it doesn't hurt to be safe.
--- src/util/command.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-)
ACK to all three cleanups. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Mar 15, 2011 at 08:03:09AM -0600, Eric Blake wrote:
On 03/15/2011 06:32 AM, Daniel P. Berrange wrote:
The virCommandNewArgs() method would free the virCommandPtr if it failed to add the args. This meant errors reported in virCommandAddArgSet() were lost. Simply removing the check for errors from the constructor means they can be reported correctly later
The virCommandAddEnvPassCommon() method failed to check for errors before reallocating the cmd->env array, causing a potential SEGV if cmd was NULL
The virCommandAddArgSet() method needs to validate that at least 1 element in 'val's parameter is non-NULL, otherwise code like
cmd = virCommandNew(binary) virCommandAddAtg(cmd, "foo")
Would end up trying todo execve("foo"), if binary was NULL.
Well, technically virCommandNew is ATTRIBUTE_NONNULL(1), so we would have caught this via clang (gcc's not quite as smart as clang at enforcing that parameter). But it doesn't hurt to be safe.
Yep, ATTRIBUTE_NONNULL is not really good enough as the only line of defense - I did just hit this in one of my other patch series. 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 :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump