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

[Jim Meyering]

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