From: Jim Meyering <meyering(a)redhat.com&gt; Among some here, there is a strong aversion to use of "assert", yet some others think it is essential (when applied judiciously) even -- perhaps "especially" -- at the heart of libraries and core hypervisor- related code. Here is a compromise that lets us make assertions about the code (e.g., to tell static analyzers about invariants) without even a hint of risk of an abort. * src/internal.h [STATIC_ANALYSIS]: Include <assert.h>. (sa_assert): Define. A no-op most of the time, but equivalent to classical assert when STATIC_ANALYSIS is nonzero.

--- src/internal.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/src/internal.h b/src/internal.h index 2e73210..4be17d8 100644 --- a/src/internal.h +++ b/src/internal.h @@ -9,6 +9,13 @@ # include <limits.h> # include <verify.h> +# if STATIC_ANALYSIS

+# include <assert.h> +# define sa_assert(expr) assert (expr) +# else +# define sa_assert(expr) /* empty */ +# endif

From: Jim Meyering <meyering(a)redhat.com&gt; Among some here, there is a strong aversion to use of "assert", yet some others think it is essential (when applied judiciously) even -- perhaps "especially" -- at the heart of libraries and core hypervisor- related code. Here is a compromise that lets us make assertions about the code (e.g., to tell static analyzers about invariants) without even a hint of risk of an abort. * src/internal.h [STATIC_ANALYSIS]: Include <assert.h>. (sa_assert): Define. A no-op most of the time, but equivalent to classical assert when STATIC_ANALYSIS is nonzero. --- src/internal.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/src/internal.h b/src/internal.h index 2e73210..4be17d8 100644 --- a/src/internal.h +++ b/src/internal.h @@ -9,6 +9,13 @@ # include <limits.h> # include <verify.h> +# if STATIC_ANALYSIS +# include <assert.h> +# define sa_assert(expr) assert (expr) +# else +# define sa_assert(expr) /* empty */ +# endif + # ifdef HAVE_SYS_SYSLIMITS_H # include <sys/syslimits.h> # endif

From: Jim Meyering <meyering(a)redhat.com&gt; * configure.ac (STATIC_ANALYSIS): Define when run via clang's scan-build or coverity-prevent's cov-build. Use the CLANG_CC and COVERITY_BUILD_COMMAND envvars as witnesses. --- configure.ac | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/configure.ac b/configure.ac index 3505b4a..664eaf0 100644 --- a/configure.ac +++ b/configure.ac @@ -2000,6 +2000,12 @@ AM_CONDITIONAL([WITH_MACVTAP], [test "$with_macvtap" = "yes"]) test "x$srcdir" = x. && ! test -f COPYING && cp -f COPYING.LIB COPYING +# Detect when running under the clang static analyzer's scan-build driver +# or Coverity-prevent's cov-build. Define STATIC_ANALYSIS accordingly. +test -n "$$CCC_ANALYZER_ANALYSIS$COVERITY_BUILD_COMMAND" && t=1 || t=0

+AC_DEFINE_UNQUOTED([STATIC_ANALYSIS], [$t], + [Define to 1 when performing static analysis.]) + AC_OUTPUT(Makefile src/Makefile include/Makefile docs/Makefile \ docs/schemas/Makefile \ gnulib/lib/Makefile \

From: Jim Meyering <meyering(a)redhat.com&gt; * configure.ac (STATIC_ANALYSIS): Define when run via clang's scan-build or coverity-prevent's cov-build. Use the CLANG_CC and COVERITY_BUILD_COMMAND envvars as witnesses. --- configure.ac | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/configure.ac b/configure.ac index 3505b4a..664eaf0 100644 --- a/configure.ac +++ b/configure.ac @@ -2000,6 +2000,12 @@ AM_CONDITIONAL([WITH_MACVTAP], [test "$with_macvtap" = "yes"]) test "x$srcdir" = x. && ! test -f COPYING && cp -f COPYING.LIB COPYING +# Detect when running under the clang static analyzer's scan-build driver +# or Coverity-prevent's cov-build. Define STATIC_ANALYSIS accordingly. +test -n "$$CCC_ANALYZER_ANALYSIS$COVERITY_BUILD_COMMAND" && t=1 || t=0 +AC_DEFINE_UNQUOTED([STATIC_ANALYSIS], [$t], + [Define to 1 when performing static analysis.]) + AC_OUTPUT(Makefile src/Makefile include/Makefile docs/Makefile \ docs/schemas/Makefile \ gnulib/lib/Makefile \

From: Jim Meyering <meyering(a)redhat.com&gt; * src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesApplyNewRules): Don't dereference a NULL or uninitialized pointer when given an empty list of rules. Add an sa_assert(inst) in each loop to tell clang that the uses of "inst[i]" are valid. --- src/nwfilter/nwfilter_ebiptables_driver.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index b481b4c..f54099f 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2834,11 +2834,11 @@ ebiptablesApplyNewRules(virConnectPtr conn ATTRIBUTE_UNUSED, bool haveIptables = false; bool haveIp6tables = false; - if (inst) - qsort(inst, nruleInstances, sizeof(inst[0]), - ebiptablesRuleOrderSort); + if (nruleInstances > 1 && inst) + qsort(inst, nruleInstances, sizeof(inst[0]), ebiptablesRuleOrderSort); for (i = 0; i < nruleInstances; i++) { + sa_assert (inst); if (inst[i]->ruleType == RT_EBTABLES) { if (inst[i]->chainprefix == CHAINPREFIX_HOST_IN_TEMP) chains_in |= (1 << inst[i]->neededProtocolChain); @@ -2881,6 +2881,7 @@ ebiptablesApplyNewRules(virConnectPtr conn ATTRIBUTE_UNUSED, goto tear_down_tmpebchains; for (i = 0; i < nruleInstances; i++) + sa_assert (inst); switch (inst[i]->ruleType) { case RT_EBTABLES: ebiptablesInstCommand(&buf, @@ -2918,6 +2919,7 @@ ebiptablesApplyNewRules(virConnectPtr conn ATTRIBUTE_UNUSED, goto tear_down_tmpiptchains; for (i = 0; i < nruleInstances; i++) { + sa_assert (inst); if (inst[i]->ruleType == RT_IPTABLES) iptablesInstCommand(&buf, inst[i]->commandTemplate,

libvir-list-bounces(a)redhat.com wrote on 04/14/2010 01:40:17 PM: > Please respond to "Daniel P. Berrange" > > On Wed, Apr 14, 2010 at 06:02:32PM +0200, Jim Meyering wrote: > > From: Jim Meyering <meyering(a)redhat.com&gt; > > > > * src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesApplyNewRules): > > Don't dereference a NULL or uninitialized pointer when given > > an empty list of rules.  Add an sa_assert(inst) in each loop to > > tell clang that the uses of "inst[i]" are valid. > > --- > >  src/nwfilter/nwfilter_ebiptables_driver.c |    8 +++++--- > >  1 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/ > nwfilter/nwfilter_ebiptables_driver.c > > index b481b4c..f54099f 100644 > > --- a/src/nwfilter/nwfilter_ebiptables_driver.c > > +++ b/src/nwfilter/nwfilter_ebiptables_driver.c > > @@ -2834,11 +2834,11 @@ ebiptablesApplyNewRules(virConnectPtr conn > ATTRIBUTE_UNUSED, > >      bool haveIptables = false; > >      bool haveIp6tables = false; > > > > -    if (inst) > > -        qsort(inst, nruleInstances, sizeof(inst[0]), > > -              ebiptablesRuleOrderSort); > > +    if (nruleInstances > 1 && inst) > > +        qsort(inst, nruleInstances, sizeof(inst[0]), > ebiptablesRuleOrderSort); > > > >      for (i = 0; i < nruleInstances; i++) { > > +        sa_assert (inst); > >          if (inst[i]->ruleType == RT_EBTABLES) { > >              if (inst[i]->chainprefix == CHAINPREFIX_HOST_IN_TEMP) > >                  chains_in  |= (1 << inst[i]->neededProtocolChain); > > @@ -2881,6 +2881,7 @@ ebiptablesApplyNewRules(virConnectPtr conn > ATTRIBUTE_UNUSED, > >          goto tear_down_tmpebchains; > > > >      for (i = 0; i < nruleInstances; i++) > > +        sa_assert (inst); Due to this statement here I get segmentation faults for which there is no reason. I have no idea why that is but I have to deactivate this line for it to work again. The same is not true for the statement further above...  So strange.    Stefan

From: Jim Meyering <meyering(a)redhat.com&gt; * src/util/util.c (virGetHostnameLocalhost): Add an sa_assert to tell clang it's ok to dereference "info" after a non-failing getaddrinfo call. --- src/util/util.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 405c5f3..268944d 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2394,6 +2394,9 @@ char *virGetHostnameLocalhost(int allow_localhost) return NULL; } + /* Tell static analyzers about getaddrinfo semantics. */ + sa_assert (info); +

From: Jim Meyering <meyering(a)redhat.com&gt; * src/qemu/qemu_driver.c (qemudDomainAttachSCSIDisk): Initialize "cont" to NULL, so clang knows it's set. Add an sa_assert so it knows it's non-NULL when dereferenced. --- src/qemu/qemu_driver.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df1d435..f5cf1f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6851,7 +6851,7 @@ static int qemudDomainAttachSCSIDisk(struct qemud_driver *driver, { int i; qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainControllerDefPtr cont; + virDomainControllerDefPtr cont = NULL; char *drivestr = NULL; char *devstr = NULL; int ret = -1; @@ -6894,6 +6894,11 @@ static int qemudDomainAttachSCSIDisk(struct qemud_driver *driver, goto error; } + /* Tell clang that "cont" is non-NULL. + This is because disk->info.addr.driver.controller is unsigned, + and hence the above loop must iterate at least once. */ + sa_assert (cont); + if (cont->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("SCSI controller %d was missing its PCI address"), cont->idx);

From: Jim Meyering <meyering(a)redhat.com&gt; * src/xen/xend_internal.c (xend_parse_sexp_desc_char): Add three uses of sa_assert, each preceding a strchr(value,... to assure clang that "value" is non-NULL. --- src/xen/xend_internal.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index c4e73b7..950f1b5 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1284,6 +1284,7 @@ xend_parse_sexp_desc_char(virBufferPtr buf, virBufferVSprintf(buf, " <source path='%s'/>\n", value); } else if (STREQ(type, "tcp")) { + sa_assert (value); const char *offset = strchr(value, ':');

From: Jim Meyering <meyering(a)redhat.com&gt; * src/xen/xend_internal.c (xend_parse_sexp_desc_char): Add three uses of sa_assert, each preceding a strchr(value,... to assure clang that "value" is non-NULL. --- src/xen/xend_internal.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index c4e73b7..950f1b5 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1284,6 +1284,7 @@ xend_parse_sexp_desc_char(virBufferPtr buf, virBufferVSprintf(buf, " <source path='%s'/>\n", value); } else if (STREQ(type, "tcp")) { + sa_assert (value); const char *offset = strchr(value, ':'); const char *offset2; const char *mode, *protocol; @@ -1325,6 +1326,7 @@ xend_parse_sexp_desc_char(virBufferPtr buf, " <protocol type='%s'/>\n", protocol); } else if (STREQ(type, "udp")) { + sa_assert (value); const char *offset = strchr(value, ':'); const char *offset2, *offset3; @@ -1383,6 +1385,7 @@ xend_parse_sexp_desc_char(virBufferPtr buf, } } else if (STREQ(type, "unix")) { + sa_assert (value); const char *offset = strchr(value, ','); int dolisten = 0; if (offset)

From: Jim Meyering <meyering(a)redhat.com&gt; * src/util/conf.c (virConfParseValue): Add an sa_assert. --- src/util/conf.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-) diff --git a/src/util/conf.c b/src/util/conf.c index ae0459e..38eb163 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -1,7 +1,7 @@ /** * conf.c: parser for a subset of the Python encoded Xen configuration files * - * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008, 2009, 2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -473,6 +473,13 @@ virConfParseValue(virConfParserCtxtPtr ctxt) SKIP_BLANKS_AND_EOL; } while ((ctxt->cur < ctxt->end) && (CUR != ']')) { + + /* Tell Clang that when execution reaches this point + "lst" is guaranteed to be non-NULL. This stops it + from issuing an invalid NULL-dereference warning about + "prev = lst; while (prev->next..." below. */ + sa_assert (lst); + if (CUR != ',') { virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting a separator in list"));