libvir-list-bounces@redhat.com wrote on 03/29/2010 12:37:02 PM:

> [image removed]
>
> [libvirt] [PATCH] nwfilter_ebiptables_driver.c: avoid NULL dereference
>
> Jim Meyering
>
> to:
>
> Libvirt
>
> 03/29/2010 12:43 PM
>
> Sent by:
>
> libvir-list-bounces@redhat.com
>
> Another one caught by clang:
>
> Note the first test to see if "inst" may be NULL.
> Then, in the following loop, "inst" is unconditionally
> dereferenced via "inst[i]".  There are other unprotected
> used of "inst[i]" below, too.
>
> Rather than trying to protect all uses, one by one, I chose
> to return "success" when given an empty list of rules.
>
> In addition, not only does it appear to be possible to call
> this function with a NULL "inst" pointer, but it may even
> be undefined.  At least one caller is virNWFilterInstantiate,
> where "inst" maps to the caller's "ptrs" variable.  There,
> ptrs is initialized (or not, in some cases) by
> virNWFilterRuleInstancesToArray.  Fortunately, at least
> this one caller (virNWFilterRuleInstancesToArray) does
> initialize "ptrs" to NULL, so in actuality, it cannot currently
> be used undefined.  But the fact that a function like
> virNWFilterRuleInstancesToArray can return "successfully"
> without defining that output parameter is a little risky.
>

Thanks again for looking at the code.

I'd like to fix this differently. Rather than return with 0 early I'd like the qsort to be skipped. If you have zero entries that would be like clearing the ebtables/iptables rules, which then should really be the only code that's executed.

Regards,

   Stefan


>
> >From f2d1a49095ed6f7caa3d5ee67409ac561c55ba77 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
> ---
>  src/nwfilter/nwfilter_ebiptables_driver.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/
> nwfilter/nwfilter_ebiptables_driver.c
> index 7871926..3932b44 100644
> --- a/src/nwfilter/nwfilter_ebiptables_driver.c
> +++ b/src/nwfilter/nwfilter_ebiptables_driver.c
> @@ -2378,31 +2378,32 @@ ebiptablesRuleOrderSort(const void *a, const void *b)
>
>  static int
>  ebiptablesApplyNewRules(virConnectPtr conn,
>                          const char *ifname,
>                          int nruleInstances,
>                          void **_inst)
>  {
>      int i;
>      int cli_status;
>      ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst;
>      int chains_in = 0, chains_out = 0;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      int haveIptables = 0;
>
> -    if (inst)
> -        qsort(inst, nruleInstances, sizeof(inst[0]),
> -              ebiptablesRuleOrderSort);
> +    if (nruleInstances == 0 || inst == NULL)
> +        return 0;
> +
> +    qsort(inst, nruleInstances, sizeof(inst[0]), ebiptablesRuleOrderSort);
>
>      for (i = 0; i < nruleInstances; i++) {
>          if (inst[i]->ruleType == RT_EBTABLES) {
>              if (inst[i]->chainprefix == CHAINPREFIX_HOST_IN_TEMP)
>                  chains_in  |= (1 << inst[i]->neededProtocolChain);
>              else
>                  chains_out |= (1 << inst[i]->neededProtocolChain);
>          }
>      }
>
>      ebtablesUnlinkTmpRootChain(conn, &buf, 1, ifname);
>      ebtablesUnlinkTmpRootChain(conn, &buf, 0, ifname);
>      ebtablesRemoveTmpSubChains(conn, &buf, ifname);
>      ebtablesRemoveTmpRootChain(conn, &buf, 1, ifname);
> --
> 1.7.0.3.448.g82eeb
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list