[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/7] nwfilter_ebiptables_driver.c: avoid NULL dereference



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)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]