Jim Meyering <jim@meyering.net> wrote on 03/29/2010
02:26:47 PM:
Jim,
either of the patches below is fine with me...
with a comment on the assert to 'keep static analyzer quiet'?
Regards,
Stefan
>
> 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