Re: [libvirt] [PATCH 3/7] nwfilter_ebiptables_driver.c: avoid NULL dereference
2010/4/15 Stefan Berger <stefanb(a)us.ibm.com>:
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.
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