[libvirt] with these, clang vs. libvirt reports no errors
I've been running clang regularly, and there have been a few pesky false-positives that just won't go away. It's not productive to reexamine them each time, so I've wanted a way to educate clang without polluting the code with work-arounds that we'll be stuck maintaining and asking questions about long after clang becomes smart enough that those work-arounds are no longer required. My solution is to mark the work-arounds with a new macro, sa_assert (for "static analysis assert"), which acts just like the classical "assert", but is only enabled when compiled by a static analyzer like clang or coverity. The advantage of using an assert-like macro is that people already know that it must have no side-effects and that will make it easy to remove later, when clang grows up. One question you may ask is why add a new symbol, when "assert" itself can already do this via NDEBUG (defined, any assertions are disabled, not defined, they are enabled). There are a few assertions in the code now, and I prefer not to touch them, and to make it clear that these are helping us cater to static analyzers. [PATCH 1/7] sa_assert: new assert-like macro, enabled only for use with static analyzers [PATCH 2/7] build: set STATIC_ANALYSIS when running via clang or coverity [PATCH 3/7] nwfilter_ebiptables_driver.c: avoid NULL dereference [PATCH 4/7] virGetHostnameLocalhost: avoid FP NULL-ptr-deref from clang [PATCH 5/7] qemudDomainAttachSCSIDisk: avoid FP NULL-ptr-deref from clang [PATCH 6/7] xend_internal.c: assure clang that we do not dereference NULL [PATCH 7/7] qemudDomainAttachSCSIDisk: avoid FP NULL-ptr-deref from clang
From: Jim Meyering <meyering@redhat.com> 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 -- 1.7.1.rc1.248.gcefbb
On 04/14/2010 10:02 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
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.
Personally, I like this compromise. But I'll let others who have been more vocal against assert() give the actual ACK.
--- 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
I think we should add a line here: # undef NDEBUG
+# include <assert.h> +# define sa_assert(expr) assert (expr) +# else +# define sa_assert(expr) /* empty */ +# endif
to guarantee that the STATIC_ANALYSIS always gets the real assert(), rather than the no-op variant required by POSIX when NDEBUG is present. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Apr 14, 2010 at 06:02:30PM +0200, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
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
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Apr 14, 2010 at 06:02:30PM +0200, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
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
So Clang defines STATIC_ANALYSIS ? IMHO that's just fine, ACK ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
From: Jim Meyering <meyering@redhat.com> * 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 \ -- 1.7.1.rc1.248.gcefbb
On 04/14/2010 10:02 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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
Typo. This isn't make, so you are blindly setting t=1. And to be robust to $COVERITY_BUILD starting with -, it might be better as: test "x$CCC_ANALYZER_ANALYSIS$COVERITY_BUILD_COMMAND" = x && t=0 || t=1
+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 \
But ACK to the concept, if 1/7 is independently ACK'd. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 04/14/2010 10:02 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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
Typo. This isn't make, so you are blindly setting t=1. And to be robust to $COVERITY_BUILD starting with -, it might be better as:
test "x$CCC_ANALYZER_ANALYSIS$COVERITY_BUILD_COMMAND" = x && t=0 || t=1
Good catch. Fixed. FYI, before this week, I was using $CLANG_CC$COVERITY_BUILD_COMMAND, and only changed it to use $CCC_ANALYZER_ANALYSIS (with the new typo) due to the fact that CLANG_CC is not defined via rawhide's scan-build.
+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 \
But ACK to the concept, if 1/7 is independently ACK'd.
On Wed, Apr 14, 2010 at 06:02:31PM +0200, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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 \
ACK with the changes already discussed Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Apr 14, 2010 at 06:02:31PM +0200, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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 \
Ah, that how the define gets there, sound fine ! ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
From: Jim Meyering <meyering@redhat.com> * 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, -- 1.7.1.rc1.248.gcefbb
On 04/14/2010 10:02 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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);
ACK, if 1/7 is approved. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Apr 14, 2010 at 06:02:32PM +0200, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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,
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
libvir-list-bounces@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@redhat.com>
* 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
2010/4/15 Stefan Berger <stefanb@us.ibm.com>:
libvir-list-bounces@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@redhat.com>
* 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
This one is obvious. The second for loop has no {} abound it's block. Before the addition of sa_assert the switch formed the block of the for loop and everything works as expected. Now the sa_assert line is block of the for loop and the switch is no longer inside the loop. Adding proper {} to the second for loop will fix the problem. Matthias
Matthias Bolte <matthias.bolte@googlemail.com> wrote on 04/14/2010 08:20:22 PM:
"Daniel P. Berrange", libvir-list
2010/4/15 Stefan Berger <stefanb@us.ibm.com>:
libvir-list-bounces@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@redhat.com>
* 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
This one is obvious. The second for loop has no {} abound it's block. Before the addition of sa_assert the switch formed the block of the for loop and everything works as expected. Now the sa_assert line is block of the for loop and the switch is no longer inside the loop.
Adding proper {} to the second for loop will fix the problem.
It has python-indentation :-) I didn't see it... and I'll fix this. Stefan
Matthias
Matthias Bolte wrote: ...
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
This one is obvious. The second for loop has no {} abound it's block. Before the addition of sa_assert the switch formed the block of the for loop and everything works as expected. Now the sa_assert line is block of the for loop and the switch is no longer inside the loop.
Adding proper {} to the second for loop will fix the problem.
D'oh!
Stefan Berger wrote:
libvir-list-bounces@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@redhat.com>
* 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.
How is STATIC_ANALYSIS defined in config.h? $ grep STATIC_AN config.h #define STATIC_ANALYSIS 0 If it's not 0, then you must have one of these two envvars set: test -n "$CCC_ANALYZER_ANALYSIS$COVERITY_BUILD_COMMAND" && echo oops How is sa_assert defined for you? $ grep -C3 sa_assert src/internal.h # if STATIC_ANALYSIS # undef NDEBUG /* Don't let a prior NDEBUG definition cause trouble. */ # include <assert.h> # define sa_assert(expr) assert (expr) # else # define sa_assert(expr) /* empty */ # endif With those, the net result in your file should be that sa_assert is a no-op. If you're still convinced that the segfault is due to that use of sa_assert, please send me preprocessed output for that file, i.e., cd src f=nwfilter_ebiptables_driver touch nwfilter/$f.c la=libvirt_driver_nwfilter_la lo=$la-$f.lo make AM_CPPFLAGS='-E -dD' $lo mv .libs/$la-$f.o $f.i The cpp-preprocessed output is now in src/nwfilter_ebiptables_driver.i You should be able to see that sa_assert expands to nothing: $ grep sa_assert $f.i #define sa_assert(expr)
Jim Meyering <jim@meyering.net> wrote on 04/15/2010 01:36:21 AM:
"Daniel P. Berrange", libvir-list
Stefan Berger wrote:
libvir-list-bounces@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@redhat.com> [...]
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.
How is STATIC_ANALYSIS defined in config.h?
$ grep STATIC_AN config.h #define STATIC_ANALYSIS 0
Also 0 here.
If it's not 0, then you must have one of these two envvars set:
test -n "$CCC_ANALYZER_ANALYSIS$COVERITY_BUILD_COMMAND" && echo oops
got 'oops' here.
How is sa_assert defined for you?
$ grep -C3 sa_assert src/internal.h # if STATIC_ANALYSIS # undef NDEBUG /* Don't let a prior NDEBUG definition cause
trouble. */
# include <assert.h> # define sa_assert(expr) assert (expr) # else # define sa_assert(expr) /* empty */ # endif
With those, the net result in your file should be that sa_assert is a no-op.
Yes, I agree. My understanding also.
If you're still convinced that the segfault is due to that use of sa_assert, please send me preprocessed output for that file, i.e.,
cd src f=nwfilter_ebiptables_driver touch nwfilter/$f.c la=libvirt_driver_nwfilter_la lo=$la-$f.lo make AM_CPPFLAGS='-E -dD' $lo mv .libs/$la-$f.o $f.i
The cpp-preprocessed output is now in
src/nwfilter_ebiptables_driver.i
You should be able to see that sa_assert expands to nothing:
$ grep sa_assert $f.i #define sa_assert(expr)
Well, not quite true. I see the lonely semicolon there that's the remainder from sa_assert(); -> one would have to write sa_assert(), which isn't nice, either... if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) goto tear_down_tmpebchains; for (i = 0; i < nruleInstances; i++) ; switch (inst[i]->ruleType) { Regards, Stefan
Stefan Berger wrote:
If it's not 0, then you must have one of these two envvars set:
test -n "$CCC_ANALYZER_ANALYSIS$COVERITY_BUILD_COMMAND" && echo oops
got 'oops' here.
That is surprising (esp. since your definition of STATIC_ANALYSIS was 0). You get that on the command line? Which of those two variables is defined for you? And to what? echo "$CCC_ANALYZER_ANALYSIS" echo "$COVERITY_BUILD_COMMAND" If you confirm, I may have to adjust the configure-time test to be less prone to false positive.
How is sa_assert defined for you?
$ grep -C3 sa_assert src/internal.h # if STATIC_ANALYSIS # undef NDEBUG /* Don't let a prior NDEBUG definition cause trouble. */ # include <assert.h> # define sa_assert(expr) assert (expr) # else # define sa_assert(expr) /* empty */ # endif
With those, the net result in your file should be that sa_assert is a no-op.
Yes, I agree. My understanding also.
If you're still convinced that the segfault is due to that use of sa_assert, please send me preprocessed output for that file, i.e.,
cd src f=nwfilter_ebiptables_driver touch nwfilter/$f.c la=libvirt_driver_nwfilter_la lo=$la-$f.lo make AM_CPPFLAGS='-E -dD' $lo mv .libs/$la-$f.o $f.i
The cpp-preprocessed output is now in
src/nwfilter_ebiptables_driver.i
You should be able to see that sa_assert expands to nothing:
$ grep sa_assert $f.i #define sa_assert(expr)
Well, not quite true. I see the lonely semicolon there that's the remainder from sa_assert(); -> one would have to write sa_assert(), which isn't nice, either...
if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) goto tear_down_tmpebchains;
for (i = 0; i < nruleInstances; i++) ; switch (inst[i]->ruleType) {
Right. It's obvious, once you notice the missing curly brace.
Jim Meyering <jim@meyering.net> wrote on 04/15/2010 09:47:53 AM:
"Daniel P. Berrange", libvir-list
Stefan Berger wrote:
If it's not 0, then you must have one of these two envvars set:
test -n "$CCC_ANALYZER_ANALYSIS$COVERITY_BUILD_COMMAND" && echo oops
got 'oops' here.
That is surprising (esp. since your definition of STATIC_ANALYSIS was 0). You get that on the command line? Which of those two variables is defined for you? And to what?
echo "$CCC_ANALYZER_ANALYSIS" echo "$COVERITY_BUILD_COMMAND"
Was a typo on my side. Anyway, problem solved after adding the braces to the for statement. Stefan
From: Jim Meyering <meyering@redhat.com> * 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); + /* if we aren't allowing localhost, then we iterate through the * list and make sure none of the IPv4 addresses are 127.0.0.1 and * that none of the IPv6 addresses are ::1 -- 1.7.1.rc1.248.gcefbb
On 04/14/2010 10:02 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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); +
ACK if 1/7 goes through, and thanks for the comment explaining why. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Apr 14, 2010 at 06:02:33PM +0200, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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); + /* if we aren't allowing localhost, then we iterate through the * list and make sure none of the IPv4 addresses are 127.0.0.1 and * that none of the IPv6 addresses are ::1
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
From: Jim Meyering <meyering@redhat.com> * 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); -- 1.7.1.rc1.248.gcefbb
On 04/14/2010 10:02 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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. --- @@ -6894,6 +6894,11 @@ static int qemudDomainAttachSCSIDisk(struct qemud_driver *driver,
A bit more context would have made review a tad easier: int i; ... for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) { cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i, qemuCmdFl ags); if (!cont)
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);
At any rate, ACK, once 1/7 is approved. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Apr 14, 2010 at 06:02:34PM +0200, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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);
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
From: Jim Meyering <meyering@redhat.com> * 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) -- 1.7.1.rc1.248.gcefbb
On 04/14/2010 10:02 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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, ':');
This introduces an expression before a declaration, even when sa_assert is empty. I know that we already rely on several other C99 features (like __VA_ARGS__ from the preprocessor), but my understanding has been that we have been trying to stick with C89 declarations first still. Does this need to be refactored accordingly? Besides, xend_parse_sexp_desc_char already dereferences value at line 1224; would it be possible to fix this by marking the argument as NONNULL, rather than adding sa_assert()? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 04/14/2010 10:02 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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, ':');
This introduces an expression before a declaration, even when sa_assert is empty. I know that we already rely on several other C99 features (like __VA_ARGS__ from the preprocessor), but my understanding has been that we have been trying to stick with C89 declarations first still. Does this need to be refactored accordingly?
There have been others like this, and no one objected then. Prohibiting stmt-after-decl is detrimental. Take this example. Prohibiting it here would change perfectly usable/readable code into this longer and slightly inferior equivalent: (inferior because it's longer and hence detracts from overall readability, and because "offset" is now duplicated) const char *offset; sa_assert (value); offset = strchr(value, ':');
Besides, xend_parse_sexp_desc_char already dereferences value at line 1224;
True, but just after that, it may be set to NULL: if (value[0] == '/') { type = "dev"; } else if (STRPREFIX(value, "null")) { type = "null"; value = NULL; <<<<=================== } else if (STRPREFIX(value, "vc")) { ... and *that* is the case clang complains about.
would it be possible to fix this by marking the argument as NONNULL, rather than adding sa_assert()?
No.
On 04/14/2010 11:29 AM, Jim Meyering wrote:
} else if (STREQ(type, "tcp")) { + sa_assert (value); const char *offset = strchr(value, ':');
This introduces an expression before a declaration, even when sa_assert is empty. I know that we already rely on several other C99 features (like __VA_ARGS__ from the preprocessor), but my understanding has been that we have been trying to stick with C89 declarations first still. Does this need to be refactored accordingly?
There have been others like this, and no one objected then. Prohibiting stmt-after-decl is detrimental.
In re-reading my comment, I can see my position was not clear. I am very much in favor of relying on more C99 features, especially decl-after-stmt. I'm not opposed to your solution, so much as questioning whether others are still trying to stick to the C89 rules, as well as point out that sticking to C89 in this case would be anachronistic given that we already have rely on other C99 features like // for comments.
Besides, xend_parse_sexp_desc_char already dereferences value at line 1224;
True, but just after that, it may be set to NULL:
if (value[0] == '/') { type = "dev"; } else if (STRPREFIX(value, "null")) { type = "null"; value = NULL; <<<<===================
Ahh. My review was not thorough enough, then. Still, clang didn't complain about the first value[0] dereference? At any rate, your additional explanation, coupled with my second review of that function, is enough to warrant ACK, once 1/7 is resolved. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 04/14/2010 11:29 AM, Jim Meyering wrote:
} else if (STREQ(type, "tcp")) { + sa_assert (value); const char *offset = strchr(value, ':');
This introduces an expression before a declaration, even when sa_assert is empty. I know that we already rely on several other C99 features (like __VA_ARGS__ from the preprocessor), but my understanding has been that we have been trying to stick with C89 declarations first still. Does this need to be refactored accordingly?
There have been others like this, and no one objected then. Prohibiting stmt-after-decl is detrimental.
In re-reading my comment, I can see my position was not clear. I am very much in favor of relying on more C99 features, especially decl-after-stmt. I'm not opposed to your solution, so much as questioning whether others are still trying to stick to the C89 rules, as well as point out that sticking to C89 in this case would be anachronistic given that we already have rely on other C99 features like // for comments.
Besides, xend_parse_sexp_desc_char already dereferences value at line 1224;
True, but just after that, it may be set to NULL:
if (value[0] == '/') { type = "dev"; } else if (STRPREFIX(value, "null")) { type = "null"; value = NULL; <<<<===================
Ahh. My review was not thorough enough, then. Still, clang didn't complain about the first value[0] dereference?
Correct. I looked a little further, and have perhaps discovered why. That entire 230-line internal function is NEVER USED. Not anywhere. I've removed it, along with the decl in the .h file, and everything still links. For now, I'll simply fix the objection from clang. But the next step will be to remove it, unless someone says they have plans for it.
At any rate, your additional explanation, coupled with my second review of that function, is enough to warrant ACK, once 1/7 is resolved.
On Wed, Apr 14, 2010 at 06:02:35PM +0200, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
From: Jim Meyering <meyering@redhat.com> * 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")); -- 1.7.1.rc1.248.gcefbb
On 04/14/2010 10:02 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* src/util/conf.c (virConfParseValue): Add an sa_assert. --- src/util/conf.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)
A little more context makes this obvious: if ((ctxt->cur < ctxt->end) && (CUR != ']')) { if ((lst = virConfParseValue(ctxt)) == NULL) return(NULL);
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); +
Either we never enter the while loop, or the previous if must have been true and lst was assigned. So ACK, if 1/7 is approved. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 04/14/2010 10:02 AM, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* src/util/conf.c (virConfParseValue): Add an sa_assert. --- src/util/conf.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)
A little more context makes this obvious:
Good point. Applies to a couple others, too. I'll try to make things easier to review next time. Thanks for the prompt reviews.
if ((ctxt->cur < ctxt->end) && (CUR != ']')) { if ((lst = virConfParseValue(ctxt)) == NULL) return(NULL);
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); +
Either we never enter the while loop, or the previous if must have been true and lst was assigned. So ACK, if 1/7 is approved.
On Wed, Apr 14, 2010 at 06:02:36PM +0200, Jim Meyering wrote:
From: Jim Meyering <meyering@redhat.com>
* 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"));
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Apr 14, 2010 at 06:02:29PM +0200, Jim Meyering wrote:
I've been running clang regularly, and there have been a few pesky false-positives that just won't go away. It's not productive to reexamine them each time, so I've wanted a way to educate clang without polluting the code with work-arounds that we'll be stuck maintaining and asking questions about long after clang becomes smart enough that those work-arounds are no longer required.
My solution is to mark the work-arounds with a new macro, sa_assert (for "static analysis assert"), which acts just like the classical "assert", but is only enabled when compiled by a static analyzer like clang or coverity. The advantage of using an assert-like macro is that people already know that it must have no side-effects and that will make it easy to remove later, when clang grows up.
One question you may ask is why add a new symbol, when "assert" itself can already do this via NDEBUG (defined, any assertions are disabled, not defined, they are enabled). There are a few assertions in the code now, and I prefer not to touch them, and to make it clear that these are helping us cater to static analyzers.
This sounds like a good compromise solution to me Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Wed, Apr 14, 2010 at 06:02:29PM +0200, Jim Meyering wrote:
I've been running clang regularly, and there have been a few pesky false-positives that just won't go away. It's not productive to reexamine them each time, so I've wanted a way to educate clang without polluting the code with work-arounds that we'll be stuck maintaining and asking questions about long after clang becomes smart enough that those work-arounds are no longer required.
My solution is to mark the work-arounds with a new macro, sa_assert (for "static analysis assert"), which acts just like the classical "assert", but is only enabled when compiled by a static analyzer like clang or coverity. The advantage of using an assert-like macro is that people already know that it must have no side-effects and that will make it easy to remove later, when clang grows up.
One question you may ask is why add a new symbol, when "assert" itself can already do this via NDEBUG (defined, any assertions are disabled, not defined, they are enabled). There are a few assertions in the code now, and I prefer not to touch them, and to make it clear that these are helping us cater to static analyzers.
This sounds like a good compromise solution to me
Thanks. Adjusted per comments and pushed.
participants (6)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Jim Meyering -
Matthias Bolte -
Stefan Berger