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

Fri Jan 8 11:13:24 UTC 2010

Daniel Veillard wrote:
...
>> 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"
>
>   I guess it's mostly a matter of coding style, I'm not sure it makes
> such a difference in practice, libvirt output will likely be burried
> in a log file, unless virsh or similar CLI tool is used.
>   We have only 4 asserts in the code currently, I think it shows that
> so far we are not relying on it. On one hand this mean calling exit()

Is "don't use assert" libvirt policy?  I hope not.

> and I prefer a good old core dump for debugging than a one line message,

Same here, but there are many reasons for which a reporter will be
unwilling or unable to provide a core dump.  No one hesitates to include
the output of a failed assertion in a bug report.

> on the other hand if you managed to catch that message, well this can
> help pinpoint if the person reporting has no debugging skills.
>
>   I think there is pros and cons and that the current status quo is
> that we don't use asserts but more defensing coding by erroring out
> when this happen. The best way is not to assert() when we may
> dereference a NULL pointer but check for NULL at the point where
> we get that pointer, that's closer to the current code practice of
> libvirt (or well I expect so :-) and allow useful reporting (we
> failed to do a given operation) and a graceful error handling without
> exit'ing. So in general if we think we need an assert somewhere that
> mean that we failed to do the check at the right place, and I would

No.  That is not how one should use assert, and certainly not
how I proposed to use it.  This is not about testing for a user-
or system-provokable condition, but rather about testing code invariants.
See below.

> rather not start to get into the practice of just asserting in a zillion
> place instead of checking at the correct place.
>
>   So in my opinion, I'm still not fond of assert(), and I would prefer
> to catch up problem earlier, like parameter checking on function entry
> points or checking return value for functions producing pointers.
>
>   In that case, res being NULL means that both answer and request
> parameters are null  after the retry: label, since the two pointers
> are not modified in the function this should be tested when entering
> the function,
>
>     if ((answer == NULL) && (request == NULL)) {
>         virProxyError (NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
>         return -1;
>     }
>
> you get the error location, as in the assert but propagate the error
> back in the stack cleanly instead of exit'ing

It is important to distinguish two types of errors:

  - conditions that may arise due to user input or environment
  (misbehaving client or server, malformed packet, I/O error, etc.)

  - internal API abuse, violation of an internal assumption or invariant

Using "assert" is appropriate only in the latter case, for detecting
conditions that typically are provoked only by developer-introduced errors.
Adding error-handling code like the above is appropriate only in the
first case.  Somewhat along these lines, we are starting to remove certain
types of tests, (e.g., conn == NULL), when "conn" is always non-NULL.
In the case of this patch, "request" is always non-null,
and should probably have the "nonnull" attribute.  New patch appended below.

One advantage of using assert is that it usually _reduces_ the
maintenance burden (i.e., when skimming the code, you may ignore the
assert statement).  However, if you add expressions like the above to
"ensure" a condition that will always be true (i.e., that should not
be possible to trigger via user input), you *increase* the maintenance
burden by adding a test of an always-false condition that is handled as
if it *can* sometimes be true.  That would increase the WTF/m rate for
certain readers. (http://www.osnews.com/story/19266/WTFs_m)
Such a test might even trigger new warnings from tools like clang
and coverity.

Whether we use an assertion in this particular case is not important,
but I hope that libvirt adopts a judicious policy on the use of "assert".

Here's the revised patch, followed by the tiny nonnull-one.
(also fixes typos s/ret/res/ in log message)


>From 2a7fed9238667ff75a6ecd662b663549bc8fb7b5 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 "res"

* src/xen/proxy_internal.c (xenProxyCommand): "res" is known to be
non-NULL at that point, so remove the "res == NULL" guard.
---
 src/xen/proxy_internal.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c
index ec4522b..ee4678a 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
  *
@@ -444,7 +444,7 @@ retry:
     /*
      * do more checks on the incoming packet.
      */
-    if ((res == NULL) || (res->version != PROXY_PROTO_VERSION) ||
+    if ((res->version != PROXY_PROTO_VERSION) ||
         (res->len < sizeof(virProxyPacket))) {
         virProxyError(conn, VIR_ERR_INTERNAL_ERROR, "%s",
                       _("Communication error with proxy: malformed packet\n"));
--
1.6.6.425.g4dc2d



>From eb6f7f0478ce510de8dc5fc9add4eaaca1c2bfc1 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at redhat.com>
Date: Thu, 7 Jan 2010 19:48:05 +0100
Subject: [PATCH] proxy_internal.c: mark "request" parameter as nonnull

* src/xen/proxy_internal.c (xenProxyCommand): Mark "request"
as an always-non-NULL parameter.
---
 src/xen/proxy_internal.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c
index ee4678a..73a0469 100644
--- a/src/xen/proxy_internal.c
+++ b/src/xen/proxy_internal.c
@@ -340,7 +340,7 @@ xenProxyClose(virConnectPtr conn)
     return 0;
 }

-static int
+static int ATTRIBUTE_NONNULL(2)
 xenProxyCommand(virConnectPtr conn, virProxyPacketPtr request,
                 virProxyFullPacketPtr answer, int quiet) {
     static int serial = 0;
--
1.6.6.425.g4dc2d


