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

[Jim Meyering]

Daniel P. Berrange wrote:

> 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.

Oh, right.  Thanks.

However, the point is still valid, so I'll wait for confirmation.
This is still about defensive coding, i.e., ensuring that
maintenance doesn't violate invariants in harder-to-diagnose ways.
If you get a bug report, which would you rather hear?
"libvirt sometimes segfaults" or
"I get an assertion failure on file.c:999"