[libvirt] [PATCH] nwfilter_ebiptables_driver.c: avoid NULL dereference
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.
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
On 03/29/2010 10:37 AM, Jim Meyering wrote:
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.
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. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 03/29/2010 10:37 AM, Jim Meyering wrote:
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.
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.
Jim Meyering <jim@meyering.net> wrote on 03/29/2010 01:22:56 PM:
[image removed]
Re: [libvirt] [PATCH] nwfilter_ebiptables_driver.c: avoid NULL dereference
Jim Meyering
to:
Eric Blake
03/29/2010 01:23 PM
Cc:
Stefan Berger, Libvirt
Eric Blake wrote:
On 03/29/2010 10:37 AM, Jim Meyering wrote:
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.
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. 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
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
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 !=
an 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
On Mon, Mar 29, 2010 at 02:32:20PM -0400, Stefan Berger wrote:
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'?
As stated previously, I really prefer to avoid assert(), thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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
participants (4)
-
Daniel Veillard -
Eric Blake -
Jim Meyering -
Stefan Berger