4:28 p.m.
* src/util/network.c (virSocketAddrMask): Zero out port, so that
iptables can initialize just the netmask, then call
virSocketFormatAddr without an uninitialized read in getnameinfo.
---
I'm not sure if this is the best patch; an alternative would be
to call memset(network,0,sizeof network) in iptablesFormatNetwork
prior to virSocketAddrMaskByPrefix. But with this patch in place,
valgrind no longer complained about:
==31478== Use of uninitialised value of size 8
==31478== at 0x3021643DAB: _itoa_word (in /lib64/libc-2.12.so)
==31478== by 0x3021644E74: vfprintf (in /lib64/libc-2.12.so)
==31478== by 0x302166EFB1: vsnprintf (in /lib64/libc-2.12.so)
==31478== by 0x302164F022: snprintf (in /lib64/libc-2.12.so)
==31478== by 0x3021705135: getnameinfo (in /lib64/libc-2.12.so)
==31478== by 0x4E4FE00: virSocketFormatAddrFull (network.c:194)
==31478== by 0x4E4FCAE: virSocketFormatAddr (network.c:152)
==31478== by 0x4E43688: iptablesFormatNetwork (iptables.c:307)
==31478== by 0x4E43FDE: iptablesForwardMasquerade (iptables.c:761)
==31478== by 0x4E44392: iptablesRemoveForwardMasquerade (iptables.c:863)
==31478== by 0x4A8BEF: networkRemoveMasqueradingIptablesRules (bridge_driver.c:893)
==31478== by 0x4A9BCA: networkRemoveIpSpecificIptablesRules (bridge_driver.c:1244)
==31478== Uninitialised value was created by a stack allocation
==31478== at 0x4E43570: iptablesFormatNetwork (iptables.c:289)
So I'm assuming that the uninitialized read was due to gethostname
looking at the port field which was still filled with uninitialized
stack data.
src/util/network.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/network.c b/src/util/network.c
index a7e7423..33028aa 100644
--- a/src/util/network.c
+++ b/src/util/network.c
@@ -1,7 +1,7 @@
/*
* network.c: network helper APIs for libvirt
*
- * Copyright (C) 2009-2010 Red Hat, Inc.
+ * Copyright (C) 2009-2011 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -291,6 +291,7 @@ int virSocketAddrIsNetmask(virSocketAddrPtr netmask) {
* virSocketAddrMask:
* @addr: address that needs to be masked
* @netmask: the netmask address
+ * @network: where to store the result, can be same as @addr
*
* Mask off the host bits of @addr according to @netmask, turning it
* into a network address.
@@ -311,6 +312,7 @@ virSocketAddrMask(const virSocketAddrPtr addr,
network->data.inet4.sin_addr.s_addr
= (addr->data.inet4.sin_addr.s_addr
& netmask->data.inet4.sin_addr.s_addr);
+ network->data.inet4.sin_port = 0;
network->data.stor.ss_family = AF_INET;
network->len = addr->len;
return 0;
@@ -322,6 +324,7 @@ virSocketAddrMask(const virSocketAddrPtr addr,
= (addr->data.inet6.sin6_addr.s6_addr[ii]
& netmask->data.inet6.sin6_addr.s6_addr[ii]);
}
+ network->data.inet6.sin6_port = 0;
network->data.stor.ss_family = AF_INET6;
network->len = addr->len;
return 0;
@@ -334,6 +337,7 @@ virSocketAddrMask(const virSocketAddrPtr addr,
* virSocketAddrMaskByPrefix:
* @addr: address that needs to be masked
* @prefix: prefix (# of 1 bits) of netmask to apply
+ * @network: where to store the result, can be same as @addr
*
* Mask off the host bits of @addr according to @prefix, turning it
* into a network address.
--
1.7.3.4
7:54 p.m.
On 01/10/2011 05:28 PM, Eric Blake wrote:
* src/util/network.c (virSocketAddrMask): Zero out port, so that
iptables can initialize just the netmask, then call
virSocketFormatAddr without an uninitialized read in getnameinfo.
---
I'm not sure if this is the best patch; an alternative would be
to call memset(network,0,sizeof network) in iptablesFormatNetwork
prior to virSocketAddrMaskByPrefix.
(Sigh. I had visited this exact piece of the code due to an error during
getnameinfo, and thought that setting network->len was the only thing
missing.)
I think this is the right place to fix it; A caller to
virSocketAddrMaskByPrefix (or virSocketAddrMask) shouldn't need to
initialize the virSocketAddr (I think it really should be an opaque type
outside of network.c, although it isn't always treated that way, even by
me).
ACK.
But with this patch in place,
valgrind no longer complained about:
==31478== Use of uninitialised value of size 8
==31478== at 0x3021643DAB: _itoa_word (in /lib64/libc-2.12.so)
==31478== by 0x3021644E74: vfprintf (in /lib64/libc-2.12.so)
==31478== by 0x302166EFB1: vsnprintf (in /lib64/libc-2.12.so)
==31478== by 0x302164F022: snprintf (in /lib64/libc-2.12.so)
==31478== by 0x3021705135: getnameinfo (in /lib64/libc-2.12.so)
==31478== by 0x4E4FE00: virSocketFormatAddrFull (network.c:194)
==31478== by 0x4E4FCAE: virSocketFormatAddr (network.c:152)
==31478== by 0x4E43688: iptablesFormatNetwork (iptables.c:307)
==31478== by 0x4E43FDE: iptablesForwardMasquerade (iptables.c:761)
==31478== by 0x4E44392: iptablesRemoveForwardMasquerade (iptables.c:863)
==31478== by 0x4A8BEF: networkRemoveMasqueradingIptablesRules (bridge_driver.c:893)
==31478== by 0x4A9BCA: networkRemoveIpSpecificIptablesRules (bridge_driver.c:1244)
==31478== Uninitialised value was created by a stack allocation
==31478== at 0x4E43570: iptablesFormatNetwork (iptables.c:289)
So I'm assuming that the uninitialized read was due to gethostname
looking at the port field which was still filled with uninitialized
stack data.
src/util/network.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/network.c b/src/util/network.c
index a7e7423..33028aa 100644
--- a/src/util/network.c
+++ b/src/util/network.c
@@ -1,7 +1,7 @@
/*
* network.c: network helper APIs for libvirt
*
- * Copyright (C) 2009-2010 Red Hat, Inc.
+ * Copyright (C) 2009-2011 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -291,6 +291,7 @@ int virSocketAddrIsNetmask(virSocketAddrPtr netmask) {
* virSocketAddrMask:
* @addr: address that needs to be masked
* @netmask: the netmask address
+ * @network: where to store the result, can be same as @addr
*
* Mask off the host bits of @addr according to @netmask, turning it
* into a network address.
@@ -311,6 +312,7 @@ virSocketAddrMask(const virSocketAddrPtr addr,
network->data.inet4.sin_addr.s_addr
= (addr->data.inet4.sin_addr.s_addr
& netmask->data.inet4.sin_addr.s_addr);
+ network->data.inet4.sin_port = 0;
network->data.stor.ss_family = AF_INET;
network->len = addr->len;
return 0;
@@ -322,6 +324,7 @@ virSocketAddrMask(const virSocketAddrPtr addr,
= (addr->data.inet6.sin6_addr.s6_addr[ii]
& netmask->data.inet6.sin6_addr.s6_addr[ii]);
}
+ network->data.inet6.sin6_port = 0;
network->data.stor.ss_family = AF_INET6;
network->len = addr->len;
return 0;
@@ -334,6 +337,7 @@ virSocketAddrMask(const virSocketAddrPtr addr,
* virSocketAddrMaskByPrefix:
* @addr: address that needs to be masked
* @prefix: prefix (# of 1 bits) of netmask to apply
+ * @network: where to store the result, can be same as @addr
*
* Mask off the host bits of @addr according to @prefix, turning it
* into a network address.
9:26 a.m.
On 01/10/2011 06:54 PM, Laine Stump wrote:
On 01/10/2011 05:28 PM, Eric Blake wrote:
> * src/util/network.c (virSocketAddrMask): Zero out port, so that
> iptables can initialize just the netmask, then call
> virSocketFormatAddr without an uninitialized read in getnameinfo.
> ---
>
> I'm not sure if this is the best patch; an alternative would be
> to call memset(network,0,sizeof network) in iptablesFormatNetwork
> prior to virSocketAddrMaskByPrefix.
(Sigh. I had visited this exact piece of the code due to an error during
getnameinfo, and thought that setting network->len was the only thing
missing.)
getnameinfo is documented as reading both addr and port, depending on
the flags. Yes, this approach still leaves other fields in the
virSocketAddrPtr uninitialized, but valgrind didn't catch any invalid
uses of those other fields.
I think this is the right place to fix it; A caller to
virSocketAddrMaskByPrefix (or virSocketAddrMask) shouldn't need to
initialize the virSocketAddr (I think it really should be an opaque type
outside of network.c, although it isn't always treated that way, even by
me).
And we can't blindly memset() inside virSocketAddrMask, since we
document (and at least one client uses) that the output addr argument
can be the same memory as the incoming argument being masked, so setting
used fields is the best we can do from within network.c to isolate the
clients from having to use memset() themselves.
ACK.
Thanks; pushed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:03 p.m.
On 01/10/2011 05:28 PM, Eric Blake wrote:
* src/util/network.c (virSocketAddrMask): Zero out port, so that
iptables can initialize just the netmask, then call
virSocketFormatAddr without an uninitialized read in getnameinfo.
---
I'm not sure if this is the best patch; an alternative would be
to call memset(network,0,sizeof network) in iptablesFormatNetwork
prior to virSocketAddrMaskByPrefix. But with this patch in place,
valgrind no longer complained about:
==31478== Use of uninitialised value of size 8
==31478== at 0x3021643DAB: _itoa_word (in /lib64/libc-2.12.so)
==31478== by 0x3021644E74: vfprintf (in /lib64/libc-2.12.so)
==31478== by 0x302166EFB1: vsnprintf (in /lib64/libc-2.12.so)
==31478== by 0x302164F022: snprintf (in /lib64/libc-2.12.so)
==31478== by 0x3021705135: getnameinfo (in /lib64/libc-2.12.so)
==31478== by 0x4E4FE00: virSocketFormatAddrFull (network.c:194)
==31478== by 0x4E4FCAE: virSocketFormatAddr (network.c:152)
==31478== by 0x4E43688: iptablesFormatNetwork (iptables.c:307)
==31478== by 0x4E43FDE: iptablesForwardMasquerade (iptables.c:761)
==31478== by 0x4E44392: iptablesRemoveForwardMasquerade (iptables.c:863)
==31478== by 0x4A8BEF: networkRemoveMasqueradingIptablesRules (bridge_driver.c:893)
==31478== by 0x4A9BCA: networkRemoveIpSpecificIptablesRules (bridge_driver.c:1244)
==31478== Uninitialised value was created by a stack allocation
==31478== at 0x4E43570: iptablesFormatNetwork (iptables.c:289)
So I'm assuming that the uninitialized read was due to gethostname
looking at the port field which was still filled with uninitialized
stack data.
src/util/network.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/network.c b/src/util/network.c
index a7e7423..33028aa 100644
--- a/src/util/network.c
+++ b/src/util/network.c
@@ -1,7 +1,7 @@
/*
* network.c: network helper APIs for libvirt
*
- * Copyright (C) 2009-2010 Red Hat, Inc.
+ * Copyright (C) 2009-2011 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -291,6 +291,7 @@ int virSocketAddrIsNetmask(virSocketAddrPtr netmask) {
* virSocketAddrMask:
* @addr: address that needs to be masked
* @netmask: the netmask address
+ * @network: where to store the result, can be same as @addr
*
* Mask off the host bits of @addr according to @netmask, turning it
* into a network address.
@@ -311,6 +312,7 @@ virSocketAddrMask(const virSocketAddrPtr addr,
network->data.inet4.sin_addr.s_addr
= (addr->data.inet4.sin_addr.s_addr
& netmask->data.inet4.sin_addr.s_addr);
+ network->data.inet4.sin_port = 0;
network->data.stor.ss_family = AF_INET;
network->len = addr->len;
return 0;
@@ -322,6 +324,7 @@ virSocketAddrMask(const virSocketAddrPtr addr,
= (addr->data.inet6.sin6_addr.s6_addr[ii]
& netmask->data.inet6.sin6_addr.s6_addr[ii]);
}
+ network->data.inet6.sin6_port = 0;
network->data.stor.ss_family = AF_INET6;
network->len = addr->len;
return 0;
@@ -334,6 +337,7 @@ virSocketAddrMask(const virSocketAddrPtr addr,
* virSocketAddrMaskByPrefix:
* @addr: address that needs to be masked
* @prefix: prefix (# of 1 bits) of netmask to apply
+ * @network: where to store the result, can be same as @addr
*
* Mask off the host bits of @addr according to @prefix, turning it
* into a network address.
ACK.
Stefan
5102
days inactive
5103
days old
3 comments
3 participants
participants (3)
-
Eric Blake -
Laine Stump -
Stefan Berger