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

Stefan Berger wrote: ... 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: Jim Meyering <meyering(a)redhat.com&gt; 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: Jim Meyering <meyering(a)redhat.com&gt; 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