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

[Stefan Berger]

Jim Meyering <jim at 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 at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100329/f6604a19/attachment-0001.htm>