[libvirt] [PATCH] don't test "res == NULL" after we've already dereferenced it

[Daniel P. Berrange]

On Wed, Jan 06, 2010 at 09:55:23PM +0100, Jim Meyering wrote:
> Daniel Veillard wrote:
> 
> > On Wed, Jan 06, 2010 at 12:46:11PM +0100, Jim Meyering wrote:
> >> As the log says, once we've dereferenced it,
> >> there's no point in comparing to NULL.
> >>
> >> >From 463eaf1027a154e71839a67eca85b3ada8b817ff Mon Sep 17 00:00:00 2001
> >> From: Jim Meyering <meyering at redhat.com>
> >> Date: Wed, 6 Jan 2010 12:45:07 +0100
> >> Subject: [PATCH] don't test "res == NULL" after we've already dereferenced it
> >>
> >> * src/xen/proxy_internal.c (xenProxyCommand): It is known to be
> >> non-NULL at that point, so remove the "ret == NULL" guard, and
> >> instead add an assert(ret != NULL), in case future code changes
> >> cause the condition to becomes false.
> >> Include <assert.h>.
> >> ---
> >>  src/xen/proxy_internal.c |    6 ++++--
> >>  1 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c
> >> index ec4522b..42b6e91 100644
> >> --- a/src/xen/proxy_internal.c
> >> +++ b/src/xen/proxy_internal.c
> >> @@ -1,7 +1,7 @@
> >>  /*
> >>   * proxy_client.c: client side of the communication with the libvirt proxy.
> >>   *
> >> - * Copyright (C) 2006, 2008, 2009 Red Hat, Inc.
> >> + * Copyright (C) 2006, 2008, 2009, 2010 Red Hat, Inc.
> >>   *
> >>   * See COPYING.LIB for the License of this software
> >>   *
> >> @@ -11,6 +11,7 @@
> >>  #include <config.h>
> >>
> >>  #include <stdio.h>
> >> +#include <assert.h>
> >>  #include <stdlib.h>
> >>  #include <unistd.h>
> >>  #include <errno.h>
> >> @@ -444,7 +445,8 @@ retry:
> >>      /*
> >>       * do more checks on the incoming packet.
> >>       */
> >> -    if ((res == NULL) || (res->version != PROXY_PROTO_VERSION) ||
> >> +    assert (res != NULL);
> >> +    if ((res->version != PROXY_PROTO_VERSION) ||
> >>          (res->len < sizeof(virProxyPacket))) {
> >>          virProxyError(conn, VIR_ERR_INTERNAL_ERROR, "%s",
> >>                        _("Communication error with proxy: malformed packet\n"));
> >
> >   I'm not too fond of adding an assert, res should not be null there or
> > we should already have crashed.
> >
> >   ACK to the change without the new assert line.
> 
> Considering that this is in the daemon and that "bad things"
> have been known to happen via NULL derefs, some would
> argue that an assertion failure is preferable.

No, this code is the client side of the Xen proxy implementation, that is
never used within daemon context.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|