Re: [libvirt] [PATCH 3/7] nwfilter_ebiptables_driver.c: avoid NULL dereference
Stefan Berger wrote:
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>
> >
> > * 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)