<tt>Jim Meyering <jim@meyering.net> wrote on 03/29/2010 02:26:47 PM: Jim,</tt> <tt> either of the patches below is fine with me... with a comment on the assert to 'keep static analyzer quiet'?</tt> <tt>Regards,</tt> <tt> Stefan</tt> <tt>> > Stefan Berger wrote: > ... > >> > ACK that your patch is the minimal fix to avoid a segfault, but we > >> > should probably get Stefan's input on whether returning success on an > >> > empty input is the best course of behavior. > >> > >> Ok. I've Cc'd him. > > > > Actually the inst[n] accesses are protected by nInstances having > to be > 0 for > > code to try to read a inst[n]. So it should be fine the way it is.nInstances > > and inst belong together and nInstances indicates the number of > members in that > > array. No member of inst[] is expected to be NULL. > > I noticed that, of course. > However, static analyzers can't always deduce such intended invariants. > In this case, it's guaranteed to be true (but not at all easy to see) > only because a prior initialization of that pointer to NULL ensures > that it's defined even when the number of rules is 0. > > Since static analyzers like clang are very handy, I'd like a way > to tell it that these uses of inst[i] are valid. > > One way is the first patch below. > That has an unpleasant side effect of adding code that we would > then have to maintain, and that would probably raise eyebrows > of anyone reviewing it. (or increase their WTF/m rate ;-) > > Another way to give clang the info it needs is to add > an "assert (inst);" on the first line of each loop in question. > That has the added benefit of helping to document the code. > They would never be triggered (assuming no one violates the invariant). > But if someone violates an invariant like that, they might as well add > code like this: v = NULL; use (*v); > > > From 84c8cc5425f79f4892b15ae131a721d0a7a1e175 Mon Sep 17 00:00:00 2001 > From: Jim Meyering <meyering@redhat.com> > Date: Mon, 29 Mar 2010 18:27:26 +0200 > Subject: [PATCH] nwfilter_ebiptables_driver.c: avoid NULL dereference > > * src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesApplyNewRules): > Don't dereference a NULL or uninitialized pointer when given > an empty list of rules. Guard each use of "inst[i]", in case > inst is NULL in spite of nruleInstances larger than zero. > --- > src/nwfilter/nwfilter_ebiptables_driver.c | 11 +++++------ > 1 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/ > nwfilter/nwfilter_ebiptables_driver.c > index 7871926..f29d0c0 100644 > --- a/src/nwfilter/nwfilter_ebiptables_driver.c > +++ b/src/nwfilter/nwfilter_ebiptables_driver.c > @@ -2389,11 +2389,10 @@ ebiptablesApplyNewRules(virConnectPtr conn, > virBuffer buf = VIR_BUFFER_INITIALIZER; > int haveIptables = 0; > > - 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++) { > + for (i = 0; i < nruleInstances && inst; i++) { > if (inst[i]->ruleType == RT_EBTABLES) { > if (inst[i]->chainprefix == CHAINPREFIX_HOST_IN_TEMP) > chains_in |= (1 << inst[i]->neededProtocolChain); > @@ -2433,7 +2432,7 @@ ebiptablesApplyNewRules(virConnectPtr conn, > if (ebiptablesExecCLI(conn, &buf, &cli_status) || cli_status != 0) > goto tear_down_tmpebchains; > > - for (i = 0; i < nruleInstances; i++) > + for (i = 0; i < nruleInstances && inst; i++) > switch (inst[i]->ruleType) { > case RT_EBTABLES: > ebiptablesInstCommand(conn, &buf, > @@ -2469,7 +2468,7 @@ ebiptablesApplyNewRules(virConnectPtr conn, > if (ebiptablesExecCLI(conn, &buf, &cli_status) || cli_status != 0) > goto tear_down_tmpiptchains; > > - for (i = 0; i < nruleInstances; i++) { > + for (i = 0; i < nruleInstances && inst; i++) { > if (inst[i]->ruleType == RT_IPTABLES) > iptablesInstCommand(conn, &buf, > inst[i]->commandTemplate, > -- > 1.7.0.3.448.g82eeb > > > Here's the alternative patch: > > From 27367f86dbb0b9c33a77a75388a584e625bf4440 Mon Sep 17 00:00:00 2001 > From: Jim Meyering <meyering@redhat.com> > Date: Mon, 29 Mar 2010 18:27:26 +0200 > Subject: [PATCH] nwfilter_ebiptables_driver.c: avoid NULL dereference > > * src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesApplyNewRules): > Don't dereference a NULL or uninitialized pointer when given > an empty list of rules. Add an assert(inst) in each loop to > tell clang that the uses of "inst[i]" are valid. > --- > src/nwfilter/nwfilter_ebiptables_driver.c | 9 ++++++--- > 1 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/ > nwfilter/nwfilter_ebiptables_driver.c > index 7871926..05d1339 100644 > --- a/src/nwfilter/nwfilter_ebiptables_driver.c > +++ b/src/nwfilter/nwfilter_ebiptables_driver.c > @@ -24,6 +24,7 @@ > #include <config.h> > > #include <sys/stat.h> > +#include <assert.h> > > #include "internal.h" > > @@ -2389,11 +2390,11 @@ ebiptablesApplyNewRules(virConnectPtr conn, > virBuffer buf = VIR_BUFFER_INITIALIZER; > int haveIptables = 0; > > - 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++) { > + assert (inst); > if (inst[i]->ruleType == RT_EBTABLES) { > if (inst[i]->chainprefix == CHAINPREFIX_HOST_IN_TEMP) > chains_in |= (1 << inst[i]->neededProtocolChain); > @@ -2434,6 +2435,7 @@ ebiptablesApplyNewRules(virConnectPtr conn, > goto tear_down_tmpebchains; > > for (i = 0; i < nruleInstances; i++) > + assert (inst); > switch (inst[i]->ruleType) { > case RT_EBTABLES: > ebiptablesInstCommand(conn, &buf, > @@ -2470,6 +2472,7 @@ ebiptablesApplyNewRules(virConnectPtr conn, > goto tear_down_tmpiptchains; > > for (i = 0; i < nruleInstances; i++) { > + assert (inst); > if (inst[i]->ruleType == RT_IPTABLES) > iptablesInstCommand(conn, &buf, > inst[i]->commandTemplate, > -- > 1.7.0.3.448.g82eeb </tt>